-
Notifications
You must be signed in to change notification settings - Fork 0
Add script to release the SDK along the OpenAPI spec #16
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.
Awesome! Really nice push towards further streamlining the release pipeline on LocalStack releases! 💯
I only added a bunch of nitpicks, nothing critical, so from my perspective this can be merged as is, or we can tackle these nitpicks right away ;)
.github/workflows/release.yml
Outdated
- name: "Generate code from spec" | ||
run: | | ||
make clean-generated | ||
./bin/generate.sh ${SPEC_URL} |
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.
nitpick (non-blocking): Maybe it would be good to have all the "spec url fiddling" in the script itself? I think it would be nicer / easier to use if the script just takes an optional version argument and in the script it either uses the latest spec or constructs the URL based on the version. What do you think?
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 the suggestion 👍 definitely cleaner, and we could also drop 2 steps from the workflow. Done in 892bf4d
|
||
- name: "Commit changed code" | ||
run: | | ||
if git status --porcelain packages/ | grep -q '^'; then |
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.
nitpick (non-blocking): That's an interesting condition, could you maybe add a comment here to explain what's going on in this line?
- name: "Install LocalStack" | ||
run: | | ||
pip install localstack==${{ env.release }} |
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.
nitpick (non-blocking): The installation of the right version of localstack
is just defined here in the pipeline. But it seems there is also a requirements-spec.txt
(which defines localstack as a dependency, but might not be used at all anymore?). I think it would be nice to:
- Clean up / remove the requirements file
- Either extend the readme explaining that one needs to start the right version of LocalStack manually, or integrate an automatic startup of the right version of LocalStack into the pytest conftest / setup.
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.
Good catch. The requirement file is just a leftover from previous iterations.
Co-authored-by: Alexander Rashed <2796604+alexrashed@users.noreply.github.com>
Co-authored-by: Alexander Rashed <2796604+alexrashed@users.noreply.github.com>
So far, we have manually released the Python SDK, generating the code from the latest OpenAPI spec.
This PR introduces a workflow to automate this process.
Given a LS version in input, we:
The workflow can be manually triggered on an arbitrary version of LS.
To properly integrate this into the release of LS, we can introduce a workflow triggered when a new OpenAPI spec is released that triggers this via
repository_dispatch
.