Skip to content
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

change(release): Update release script and check it in CI #7128

Merged
merged 28 commits into from
Jul 4, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 3, 2023

Motivation

The script in our release checklist doesn't actually work.

Specifications

cargo release command reference:
https://github.com/crate-ci/cargo-release/blob/master/docs/reference.md

Complex Code or Requirements

This PR works around a cargo publish unpublished version resolution bug, by skipping the failing Zebra crates:
crate-ci/cargo-release#691

This bug only happens in dry runs, publishing crates works fine.

Solution

Release Checklist Fixes:

  • Update the release script so it works
    • Fix doc version replacements and add them to the script
  • Add and remove do-not-merge tags at the right times
  • Fix a command that's easy to accidentally run on the wrong version

Release Config Fixes:

  • Add a release config to simplify release commands and apply default new crate settings
  • Fix a release warning about an outdated default-features spelling
  • Remove a redundant elasticsearch feature dependency that caused release errors

CI:

  • Check that the release script works by running it on each PR (but skip crates that fail dry run checks)
  • Add special arguments that are needed for non-interactive release dry runs

Release v1.0.1 Cleanups

  • Update release date
  • Add a missing changelog entry
  • Do missing doc version replacements

Admin Actions

  • Admin: Add the new check as a branch protection rule

Review

This is a routine fix.

Reviewer Checklist

  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

We could move the script into a file
We could automate releases and do them from GitHub Actions

@teor2345 teor2345 added C-bug Category: This is a bug P-Medium ⚡ C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG A-release Area: Zebra releases and release management labels Jul 3, 2023
@teor2345 teor2345 requested a review from a team as a code owner July 3, 2023 01:51
@teor2345 teor2345 self-assigned this Jul 3, 2023
@teor2345 teor2345 requested review from a team as code owners July 3, 2023 01:51
@teor2345 teor2345 requested review from gustavovalverde and arya2 and removed request for a team July 3, 2023 01:51
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Jul 3, 2023
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #7128 (f6329b4) into main (4713994) will increase coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7128      +/-   ##
==========================================
+ Coverage   77.33%   77.41%   +0.08%     
==========================================
  Files         310      310              
  Lines       41828    41828              
==========================================
+ Hits        32347    32383      +36     
+ Misses       9481     9445      -36     

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you for getting started on automating some release steps!

I'm not sure about the workaround for cargo publish, I think it would have to be applied to every crate that depends on another crate in the workspace. It may be better to wait until there's more support for this use case, or to try an ephemeral private registry (ktra should mirror crates.io for Zebra's external deps).

.github/workflows/release-crates-io.yml Outdated Show resolved Hide resolved
zebra-consensus/Cargo.toml Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Jul 3, 2023

I'm not sure about the workaround for cargo publish, I think it would have to be applied to every crate that depends on another crate in the workspace.

This is actually a specific bug that's triggered by Zebra's dependency tree:
crate-ci/cargo-release#691

Normally, cargo would ignore missing crate versions that it's just about to publish. But Zebra triggers a bug in that code.

@teor2345
Copy link
Contributor Author

teor2345 commented Jul 4, 2023

Ok, the new job works in CI:

Packaging zebra-rpc v1.0.0-beta.28 (/home/runner/work/zebra/zebra/zebra-rpc)
Packaged 138 files, 451.8KiB (93.7KiB compressed)
Uploading zebra-rpc v1.0.0-beta.28 (/home/runner/work/zebra/zebra/zebra-rpc)
warning: aborting upload due to dry run

https://github.com/ZcashFoundation/zebra/actions/runs/5450302581/jobs/9915420110?pr=7128#step:7:235

mergify bot added a commit that referenced this pull request Jul 4, 2023
@mergify mergify bot merged commit 9b32ab7 into main Jul 4, 2023
@mergify mergify bot deleted the fix-release-script branch July 4, 2023 19:01
@teor2345
Copy link
Contributor Author

teor2345 commented Jul 9, 2023

I just added this check to the branch protection rules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-release Area: Zebra releases and release management C-bug Category: This is a bug C-enhancement Category: This is an improvement C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants