-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add workflow to dispatch the update of the OpenAPI spec #11709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow is looking good, but I would discuss the usage patterns for the client-payload config before merging this PR. I think this could be problematic, but could maybe also be useful?
token: ${{ secrets.PRO_ACCESS_TOKEN }} | ||
repository: localstack/openapi | ||
event-type: openapi-update | ||
client-payload: '{"ref": "${{ github.ref }}", "repo": "${{ github.repository }}"}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is this really about handing over the reg and repo here? This would mean that if you trigger this manually from another branch, the specs would be taken from another branch, updating the specs in the repo with a non-merged version.
Is this what we want? What would this be used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair observation. I added the ref to the branch and the workflow_dispatch
to be able to test the workflow from some spec changes sitting in a branch.
In my view, this workflow should be dispatched only when a spec change is pushed on master. Mostly for testing purposes, it could be handy to have the chance to trigger it from another branch, but I can't come up with concrete examples right now. The spec won't be automatically updated anyway: a PR would still have to be approved and merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right that makes sense! The additional barrier with the PR is great and should definitely be enough!
Would you mind adding a comment to the client-payload
? I feel like this isn't obvious and might make sense to add a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for answering the question! I think it would be great to add a small comment on the line discussed, but that's just a nitpick / not blocking a merge! 🚀
Motivation
We are building a separate repo to store LocalStack's OpenAPI specs.
This workflow is triggered when the spec changes and it dispatches a workflow on the destination repo that generates a new
latest
spec.See https://github.com/localstack/localstack-ext/pull/3554 for more context.