-
Notifications
You must be signed in to change notification settings - Fork 16
chore(ci): perform a dry-run release on each PR #244
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.
one question
.github/workflows/publish.yml
Outdated
|
||
jobs: | ||
publish: | ||
name: Publish in order | ||
runs-on: ubuntu-latest | ||
env: | ||
DRY_RUN_FLAG: ${{ !inputs.publish && "--dry-run" }} |
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 hate to keep bearing bad news, but the run failed at https://github.com/noir-lang/acvm/actions/runs/5268520673/workflow
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.
Yep, no worries. This PR has been sitting at the back of my mind along with #325, I was going to address this this evening but got sidetracked with brillig foreign call things.
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.
Will try to get this working before we push out #355
Something I overlooked when making this PR is that we're not testing the exact scenario as in a proper release (as shown in https://github.com/noir-lang/acvm/actions/runs/5274305702/jobs/9538639231#step:8:131). Each package is currently using the versions of its workspace dependencies from the previous "real" release on crates.io rather than the outputs of the dry-run releases from within the same workflow. We would need something like rust-lang/cargo#1169 in order to be able to do a proper apples-to-apples test for this. |
@phated my thoughts are we close this as we'd need to use a tool such as this in order to be able to dry-run the entire workspace, but the risk of including untrusted code into publishing pipeline is greater than the benefits. We can roll out dry-runs for any repositories which publish a single crate however. |
pull_request: | ||
branches: [master] |
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.
As @TomAFrench called out, this will need to become pull_request_target
before merge
Closing this PR as it's blocked by needing to support dry-run releases which rely on a dependency which is itself being dry-run released. |
Related issue(s)
Resolves #241
Description
Summary of changes
This PR runs the
publish
workflow on pushes to the branch which release-please uses to ensure that the release will be successful. To prevent these CI runs from publishing to crates.io we require apublish
flag to be set when calling the workflow to not runcargo publish --dry-run
I haven't performed any testing of this on a separate repo.
Dependency additions / changes
(If applicable.)
Test additions / changes
(If applicable.)
Checklist
cargo fmt
with default settings.Additional context
(If applicable.)