chore: Separate api-docs PR and trigger another workflow for it#1718
chore: Separate api-docs PR and trigger another workflow for it#1718emincihangeri wants to merge 41 commits intomainfrom
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Separate API Docs PR from Release Notes WorkflowRefactor♻️ Decoupled the API documentation generation from the release notes workflow. Previously, API docs and release notes were bundled into a single PR during the release bump. They are now handled as two separate, independent PRs — improving modularity and allowing each to be triggered and merged independently. Changes
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
| inputs: | ||
| version: | ||
| description: 'Version to generate docs for (e.g., 2.9.0). Only the major version is used for the target folder (e.g., v2).' | ||
| description: 'Version to generate docs for (e.g., 2.9.0).' |
There was a problem hiding this comment.
[q] Why do we even have to pass a version? Can't we take what is in the package.json?
There was a problem hiding this comment.
This could be useful in very rare cases that there is something needs to be fixed in the API docs after a bump for a specific version but I am not sure if it is a must. :/
There was a problem hiding this comment.
Then this could be an optional field.
There was a problem hiding this comment.
[req] You need to handle the case if nothing was passed still. I think there is a script to get the version from package.json. If not, I think this can be very easily be done on the command line.
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
marikaner
left a comment
There was a problem hiding this comment.
We should remove the non-existent token + env variable, everything else is details, but would be nice to have before merging.
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
| inputs: | ||
| version: | ||
| description: 'Version to generate docs for (e.g., 2.9.0). Only the major version is used for the target folder (e.g., v2).' | ||
| description: 'Version to generate docs for (e.g., 2.9.0).' |
There was a problem hiding this comment.
[req] You need to handle the case if nothing was passed still. I think there is a script to get the version from package.json. If not, I think this can be very easily be done on the command line.
| inputs: | ||
| version: | ||
| description: 'Version to generate docs for (e.g., 2.9.0). Only the major version is used for the target folder (e.g., v2).' | ||
| description: 'Version to generate docs for (e.g., 2.9.0).' |
This reverts commit 4a92f92.
| inputs: | ||
| version: | ||
| description: 'Version to generate docs for (e.g., 2.9.0). Only the major version is used for the target folder (e.g., v2).' | ||
| description: 'Version to generate docs for (e.g., 2.9.0).' | ||
| required: true | ||
| type: string |
There was a problem hiding this comment.
[req] After reviewing this once more, I noticed that you don't use the version for anything different than the branch name and title of the PR and commit. As discussed, you fixed this for the workflow_dispatch, but not the workflow_call.
When you call the workflow from bump as is right now, yes, you are not running on the bumped version, but on the state of main pre bump (https://docs.github.com/en/enterprise-server@3.19/actions/how-tos/reuse-automations/reuse-workflows?utm_source=chatgpt.com#calling-a-reusable-workflow). But changing the title of the api-docs commit, won't change that.
See my comments in bump for suggestions.
| generate-api-docs: | ||
| name: Generate and Push API Documentation | ||
| needs: [bump] | ||
| uses: ./.github/workflows/api-docs.yml | ||
| secrets: inherit | ||
| with: | ||
| version: ${{ needs.bump.outputs.version }} |
There was a problem hiding this comment.
[req] AFAIK you have 2 options:
- Call the workflow using the gh cli.
- trigger the workflow on push to the tag: https://docs.github.com/en/enterprise-server@3.19/actions/reference/workflows-and-actions/workflow-syntax?utm_source=chatgpt.com&versionId=enterprise-server%403.19&productId=actions&restPage=how-tos%2Creuse-automations%2Creuse-workflows#example-including-branches-and-tags
There was a problem hiding this comment.
I tried to implement with the 2nd option now. Looks more clean.
marikaner
left a comment
There was a problem hiding this comment.
one more thought: I think it would be even better, if you made a reusable create-api-docs-pr action. Then you could use that directly in the api-docs.yml and bump.yml
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
So the bump workflow calling the api-docs.yml will no longer be the logic? but we still want to have the api-docs.yml for manual triggering then, right? |
marikaner
left a comment
There was a problem hiding this comment.
I still have some requests. I think it was not necessary to move the api-docs implementation to an action (my bad) but it is a good idea, if we want to align this between Cloud SDK and AI SDK, so let's keep it as is.
| if git commit -m 'Update SAP AI SDK for JavaScript API documentation v${{ steps.version.outputs.major-version }}'; then | ||
| git push -u origin "$BRANCH_NAME" | ||
| echo "pushed=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "No changes to commit. Skipping push." | ||
| echo "pushed=false" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
[req] I think the if else conditions here are redundant. We already have the check whether there are changes. If there are no changes, we never enter this step. So it would suffice to do the if block and not create a GITHUB_OUTPUT.
| fi | ||
|
|
||
| - name: Create pull request | ||
| if: steps.check-changes.outputs.docs == 'true' && steps.commit-push.outputs.pushed == 'true' |
There was a problem hiding this comment.
| if: steps.check-changes.outputs.docs == 'true' && steps.commit-push.outputs.pushed == 'true' | |
| if: steps.check-changes.outputs.docs == 'true' |
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Context
Closes SAP/ai-sdk-js-backlog#453.
What this PR does and why it is needed