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

ci: automate creation of GitHub releases and tags #2027

Closed
wants to merge 8 commits into from

Conversation

galargh
Copy link
Contributor

@galargh galargh commented Jul 23, 2024

This automates the creation of GitHub releases and associated GitHub tags based on the changes to the version in the selected Cargo.toml files.

The automation uses the same principles as the one you might know from Go projects where we control the current version via version.json file (https://github.com/ipdxco/unified-github-workflows?tab=readme-ov-file#versioning). This is the exact same workflow just updated to work with Cargo.toml.

Note, that the automation doesn't take care of cargo publishing currently. It doesn't create crate_name@crate_version tags either. To handle these, I'd suggest creating another workflow triggered on tag creation or github release publishing, which creates additional tags and performs cargo publishing.

The automation takes care of publishing crate_name@crate_version packages. It also creates GitHub releases for each tag.

@galargh galargh changed the title Ipdx/unified GitHub workflows ci: automate creation of GitHub releases and tags Jul 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.63%. Comparing base (844d34a) to head (423e826).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2027   +/-   ##
=======================================
  Coverage   75.63%   75.63%           
=======================================
  Files         155      155           
  Lines       15671    15676    +5     
=======================================
+ Hits        11852    11857    +5     
  Misses       3819     3819           

see 3 files with indirect coverage changes

@galargh galargh requested review from Stebalien and BigLep July 23, 2024 14:33
@galargh galargh marked this pull request as ready for review July 23, 2024 14:33
@Stebalien
Copy link
Member

Does this do a dry-run publish to prevent cutting a release if the eventual publish will fail (e.g., because we failed to release some dependency in the workspace)?

@rvagg
Copy link
Member

rvagg commented Jul 24, 2024

dry-run publish

I imagine this will just do a github release draft publish like version.json workflow? I think @Stebalien is asking about doing a cargo publish --dry-run, which I don't imagine to be part of this and that we probably need a second workflow to do with the same trigger as this? Or can we just add a new step in .github/workflows/releaser.yml to do this?

@galargh
Copy link
Contributor Author

galargh commented Jul 24, 2024

I imagine this will just do a github release draft publish like version.json workflow? I think @Stebalien is asking about doing a cargo publish --dry-run, which I don't imagine to be part of this and that we probably need a second workflow to do with the same trigger as this? Or can we just add a new step in .github/workflows/releaser.yml to do this?

Yes, we can add this as a new job in the releaser.yml and/or release-check.yml, web3-bot won't overwrite it. I'll propose that in a comment to show you, how this could look like.

@Stebalien
Copy link
Member

When you say this "automates the creation of tags" but doesn't automate the creation of crate_name@crate_version tags, what do you mean? We use the latter because we can and do release crates at separate times with separate versions.

E.g., what tags will I get if I update the version of fvm_ipld_hamt?

@galargh
Copy link
Contributor Author

galargh commented Jul 28, 2024

Now, the introduced workflows will release packages defined by any of these Cargo.tomls:

fvm/Cargo.toml
testing/integration/Cargo.toml
ipld/amt/Cargo.toml
ipld/bitfield/Cargo.toml
ipld/blockstore/Cargo.toml
ipld/car/Cargo.toml
ipld/encoding/Cargo.toml
ipld/hamt/Cargo.toml
ipld/kamt/Cargo.toml
sdk/Cargo.toml
shared/Cargo.toml

The workflows will also be triggered on changes to the root Cargo.toml since some of the packages use the workspace version.

This is how we'll work out a version for a specific package:

  1. If there is workspace.package.version or package.version or version defined in the config file, use it.
  2. Otherwise, check Cargo.toml in the parent directory unless we were already checking the root directory.

As for the name of the package to use, we'll infer it as follows:

  1. If there is package.name or name defined in the config file, use it.
  2. Otherwise, use the relative path from root as the package name.

Now, let's say, we're trying to release package defined by fvm/Cargo.toml.

  1. We modify the version root Cargo.toml
  2. We create a PR
  3. release-check.yml is triggered
  4. The workflow finds the name of the package in fvm/Cargo.toml and the version to use in Cargo.toml
  5. It creates a draft Release
  6. It comments on the PR with a link to the draft Release
  7. It recognizes that a draft for fvm/Cargo.toml was created so it proceeds to execute cargo publish --dry-run in the fvm directory.
  8. We merge the PR
  9. release.yml is triggered
  10. The workflow, similarly, finds the name of the package in fvm/Cargo.toml and the version in Cargo.toml
  11. It finds the draft Release that was previously created
  12. It publishes the draft Release

I also added a separator input to the release workflows and specified it here as @. This means that in the example above, the package that would be released would be [email protected].

Let me know if this meets your expectations now.

@Stebalien
Copy link
Member

I'm going to wait for us to resolve all the conversations on filecoin-project/actors-utils#235 first to avoid duplicating any work/discussions.

@galargh
Copy link
Contributor Author

galargh commented Aug 5, 2024

I limited duplication here in 50273e3, similarly to changes requested in actors-utils. Let me know if there's anything else beyond this that you'd like to see in this PR.

CONTRIBUTING.md Outdated
@@ -78,7 +78,7 @@ Once the release is prepared, it'll go through a review:

Finally, an [FVM "owner"](https://github.com/orgs/filecoin-project/teams/fvm-crate-owners/members) will:

1. Merge the release PR to master.
1. Merge the release PR to master (this will trigger GitHub release/tag creation if the workspace version in the root `Cargo.toml` was modified).
Copy link
Member

Choose a reason for hiding this comment

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

Is this complete? Are there steps from #2027 (comment) we need to include? I don't see discussion about opening a PR which creates a draft Release and that when we merge the PR we get a published Release.

And are the comments about an FVM owner still needing to run cargo publish accurate? (or are we only doing a dryrun as part of CI?)

@BigLep
Copy link
Member

BigLep commented Aug 5, 2024

@Stebalien : I believe @galargh believes this is ready for review again.

I am personally confused about whether crate publishing has been automated or not but that might be my own current mental state. I'm hopeful we get the readme updated to the new reality.

@Stebalien
Copy link
Member

#2031 should check the root crate. I think we need to match /Cargo.toml in addition to **/Cargo.toml.

source: ${{ matrix.source }}
run: |
pushd "$(dirname $source)"
cargo publish --dry-run
Copy link
Member

Choose a reason for hiding this comment

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

I think this will fail because it won't publish in the right order.

Copy link
Member

Choose a reason for hiding this comment

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

I.e., if we cut a release of the sdk + shared at the same time (which we always do), we'd need to push a release of shared to some fake registry for the release of the sdk to pass a cargo publish dry run (I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean now! Yes, that would definitely show up as a failure in the proposed process. Would it make sense to split the release in that case, as in, you first release shared and only follow up with sdk release after shared is fully out?

Copy link
Member

Choose a reason for hiding this comment

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

That's going to make releases take even longer, which defeats the point of having automation.

I think the answer here is manual dry-run and it's not going to be pretty.

  1. Figure out which crates need to be published.
  2. Either determine the correct order, or hard-code it in a config.
  3. Run cargo vendor --versioned-dirs, creating a .cargo/config.toml file to respect the vendor directory.
  4. In publish order (reverse dependency order):
  5. Run cargo package.
  6. Copy the package (the extracted one) out of target/packages into vendor.
  7. Generate the .cargo-checksum.json file for that manually vendored package. This is the most annoying step, but shouldn't be hard with find + jq...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very helfpul, thank you. I included this in the improvements section of the release automation doc in a3fb5a9. I also described what happens inside the current state of the automation in more detail.

I think the big decision here is whether the partial automation would be useful here, or whether we should wait until we can fully handle cargo publishes with respect to the dependency graph.

cc @BigLep

@galargh
Copy link
Contributor Author

galargh commented Aug 12, 2024

#2031 should check the root crate. I think we need to match /Cargo.toml in addition to **/Cargo.toml.

**/Cargo.toml captures root Cargo.toml as well. It hasn't been triggered in your PR because we use the pull_request_target trigger which means that the workflow has to be defined in the PR target and we use that version of it.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Thanks for adding the docs. I'll leave for others to approve and make decisions here, but this still seems to me like it's moving the ball forward.

CONTRIBUTING.md Outdated Show resolved Hide resolved
2. Check if a git tag for the version, using the `crate_name@version` as the pattern, already exists. Continue only if it does not.
3. Create a draft GitHub release with the version as the tag.
4. Comment on the pull request with a link to the draft release.
5. Run `cargo publish --dry-run` for the crate for which the release is proposed.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was explicitly requested here - that's why it was added. But it's not clear to me whether we want it at all without being able to incorporate dependency graph knowledge into the dry run publish process.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@BigLep
Copy link
Member

BigLep commented Aug 15, 2024

Per verbal with @Stebalien, without crate publishing we don't think this workflow is good enough to move forward. The reason is that in the manual workflow that a releaser like @Stebalien follows involves:

  1. Merge the version changes into main/master
  2. Attempt to publish crates. There is some trial and error on publishing the crates at this point and sometimes discover that a dependency hasn't been published yet, which after fixing means bumping the version again.
  3. After publishing the crates successfully the release engineer then does the git tags.

The workflow proposed in this PR has git tagging happening before any publishing has occurred and there are reasons why a dryrun publish is not truly representative (I'm not up on these details). As a result, this workflow could cause more work than before because you could be untagging in some cases.

It's a bummer not to get this across the line, but I know IPDX's contract hours with FilOz are up at this point. As a result, I'm going to close this PR. For next network upgrade, nv24, if the situation hasn't improved, we'll have someone like @rjan90 or @BigLep watch @Stebalien and capture the manual steps that are covered so we at least document the current manual release process better. We'll also make sure that others have access to publish with filecoin-project/github-mgmt#58 landed.

@galargh
Copy link
Contributor Author

galargh commented Aug 23, 2024

Sad to hear we couldn't get it over the line in this iteration, but I think these are totally fair points. I hope we'll be able to work on these together in the future and get the full automation in place for both this repo and actors-utils. Thank you all for all the reviews and information sharing. Even though we didn't land this, I think that the back-and-forth that we did here is going to be very useful whenever one attempts to implement full release process automation.

@Stebalien
Copy link
Member

Yeah, the lack of a a viable cargo publish --workspace --dry-run makes all this really hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

5 participants