-
Notifications
You must be signed in to change notification settings - Fork 717
Update release workflow #6697
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
base: develop
Are you sure you want to change the base?
Update release workflow #6697
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6697 +/- ##
============================================
+ Coverage 44.77% 79.78% +35.01%
============================================
Files 577 577
Lines 357352 357795 +443
============================================
+ Hits 159992 285475 +125483
+ Misses 197360 72320 -125040 see 494 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
moving to ghcr - one thing i don't like is how all the layers (tagged or not) are visible on the packages page (see link above). a few things to follow up on:
|
|
there is also a question of if we should be re-running all the same tests (which should have been passing prior to release in order for the changes to merge), i.e. bitcoin tests. |
|
update - one thing i notice that i'm not sure about (but shoudln't be a blocker to merge) is that the artifacts are being attested 2x for the same archive. this may be a result of the the archive name being identical for stacks-core and the stacks-signer release. Edit: it is indeed because of the archive file naming being a duplicate for the release. so stacks-core & stacks-signer both having the same archive name like |
if you have a look to the sbtc repo I solved this problem! |
just to clear my understanding: does it means we don't run the "ci" tests for the hotfix release? |
no - just a note that we currently re-run all tests that are run as part of the PR process when doing a release, and perhaps there's an option to not do that in the future (with the expectation that tests would have already run. there are some caveats today, but i think it's possible). for a hotfix release, we'd still need to run all the normal required steps. but, i think there may be a case to be made where we can ensure all tests are passing before a release workflow is triggered - not addressed in this PR, but something to think about for a future change. |
…-test workflow - update ci.yml to no longer call atlas, slow workflows on release - remove test from sbtc and epoch workflows that no longer exist - moved last remaining test from slow_test to bitcoin_test workflow
aaronb-stacks
left a comment
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.
This LGTM, just a comment on some of the dockerfiles' CMDs
jacinta-stacks
left a comment
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.
LGTM.
|
|
||
| FROM alpine | ||
| COPY --from=builder /out/* /bin/ | ||
| CMD ["/bin/stacks-signer run --config /signer-config.toml"] |
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.
sanity check: shouldn't this be CMD ["/bin/stacks-signer", "run", "--config", "/signer-config.toml"] ?
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.
both are correct, but the list is more correct.i'll adjust (same with later comment)
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.
|
|
||
| FROM debian:stable-slim | ||
| COPY --from=builder /out/* /bin/ | ||
| CMD ["/bin/stacks-node mainnet"] |
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.
same q: CMD ["/bin/stacks-node", "mainnet"] ?
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.
|
|
||
| FROM debian:stable-slim | ||
| COPY --from=builder /out/* /bin/ | ||
| CMD ["/bin/stacks-node mainnet"] |
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.
same 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.
|
|
||
| FROM debian:stable-slim | ||
| COPY --from=builder /out/* /bin/ | ||
| CMD ["/bin/stacks-signer run --config /signer-config.toml"] |
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.
same 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.
|
|
||
| FROM alpine | ||
| COPY --from=builder /out/* /bin/ | ||
| CMD ["/bin/stacks-node mainnet"] |
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.
same 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.
| shell: bash | ||
| run: | | ||
| var_docker_tag_rc=false | ||
| if [[ "${docker_tag}" =~ -rc[0-9]*$ ]]; 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.
should the variable docker_tag be uppercase? if [[ "${DOCKER_TAG}" =~ -rc[0-9]*$ ]]; 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.
i treated this as a global/local syntax, where var_docker_tag_rc (and others only used in the step) are lowercase, and the global vars are set to uppercase. this may not be the same in older workflows though, but in this PR i tried to treat them this way consistently.
there's no clear guideline on what's acceptable in workflows that i could find, but if you think uppercase makes more sense i can adjust and push.
| context: /tmp | ||
| file: ./.github/actions/dockerfiles/debian/Dockerfile |
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.
sanity check: does the file relative path works even if context is pointing to /tmp instead of the current dir . ?
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.
I have this doubt because the documentation says that the default value of file is relative to context. (default {context}/Dockerfile)
https://github.com/docker/build-push-action?tab=readme-ov-file#inputs
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.
i read the same docs and had some really strange issues (i suspect the docs are al little obtuse).
when using a context of ., the dockerfile was not able to find the artifacts downloaded to ., which is why i chose a generic absolute of /tmp.
now, since we are setting both the context and the file inputs, i don't think the defaults in the docs apply here (and in practice, it works) - since we're setting the context specifically to /tmp, unless we also did not set the file input, it would look for /tmp/Dockerfile as a default.
but, since we're being verbose about which dockerfile to use, the default {context}/Dockerfile doesn't apply.
56b5ee7
Note: relies on composite PR stacks-network/actions#91 due to the changes in registry.
This change refactors the release workflow as follows:
release-*.ymletc.Overall, the performance is much better than using docker - what was taking ~30 minutes (discounting timeouts and failures) is now reliably down to ~12 minutes per build (macos being the outlier, since it was tested on a 7GB machine with the release-lite profile).
a full test release workflow: https://github.com/wileyj/stacks-core/actions/runs/19481419227
and a specific run of just the release workflow to show the expected timing of the changes proposed here: https://github.com/wileyj/stacks-core/actions/runs/19475022815
and the ghcr images produced-
signer: https://github.com/wileyj/stacks-core/pkgs/container/stacks-signer/versions?filters%5Bversion_type%5D=tagged
stacks-core: https://github.com/wileyj/stacks-core/pkgs/container/stacks-core/versions?filters%5Bversion_type%5D=tagged
published test release: https://github.com/wileyj/stacks-core/releases/tag/4.0.0.0.6
published singer test release: https://github.com/wileyj/stacks-core/releases/tag/signer-4.0.0.0.6.0
finally - the builds are set to use the
releaseprofile, which willl require at least the macos-latest-large runner, with the linux runners currently set to use the default ubuntu-latest (this may be changed, but the timing on default runners was acceptable).the binaries/images need more verification, but initial testing of produced artifacts looks good.