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 #235

Closed
wants to merge 6 commits into from

Conversation

galargh
Copy link

@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 Cargo.toml files across the repo. The tags will be prefixed with the directory in which the Cargo.toml file is. For example, changing the version in dispatch_examples/greeter/Cargo.toml will create dispatch_examples/greeter/${VERSION} tag.

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 workflow with Cargo.toml.

Note, that the automation doesn't take care of cargo publishing currently.

@galargh galargh requested review from BigLep and rjan90 July 23, 2024 14:31
@galargh galargh marked this pull request as ready for review July 23, 2024 14:31
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.

Conceptually seems right but I'll leave to those who have done hands-on releasing here to approve like @rjan90 or @rvagg or @Stebalien.

Also, for the record this addresses some of #181 , but it isn't handling artifact creating/publishing (as called out in the PR description).

Comment on lines 6 to 24
"dispatch_examples/greeter/Cargo.toml",
"frc42_dispatch/Cargo.toml",
"frc42_dispatch/hasher/Cargo.toml",
"frc42_dispatch/macros/Cargo.toml",
"frc42_dispatch/macros/example/Cargo.toml",
"frc46_token/Cargo.toml",
"frc53_nft/Cargo.toml",
"fvm_actor_utils/Cargo.toml",
"fvm_dispatch_tools/Cargo.toml",
"testing/integration/Cargo.toml",
"testing/test_actors/Cargo.toml",
"testing/test_actors/actors/basic_nft_actor/Cargo.toml",
"testing/test_actors/actors/basic_receiving_actor/Cargo.toml",
"testing/test_actors/actors/basic_token_actor/Cargo.toml",
"testing/test_actors/actors/basic_transfer_actor/Cargo.toml",
"testing/test_actors/actors/frc46_factory_actor/Cargo.toml",
"testing/test_actors/actors/frc46_test_actor/Cargo.toml",
"testing/test_actors/actors/frc53_test_actor/Cargo.toml",
"testing/test_actors/actors/frc46_factory_token/token_impl/Cargo.toml"
Copy link
Member

Choose a reason for hiding this comment

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

This set of files is duplicated 4 times across 2 files. Is there a way to factor it out to one place?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, that isn't that straighforward.

Copy link
Member

Choose a reason for hiding this comment

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

Ack - bummer. I guess this in part because GitHub Actions doesn't support YML anchors?
Maybe for now just add a comment that any changes here should also be made in releaser.yml (and vice versa)

@BigLep BigLep requested review from Stebalien and rvagg July 23, 2024 20:08
@rvagg
Copy link
Member

rvagg commented Jul 24, 2024

frc42_dispatch/hasher and frc42_dispatch/macros would be new packages that haven't previously been tagged here if those Cargo.toml files are touched.

Same for frc42_dispatch/macros/example, dispatch_examples/greeter, testing/integration, testing/test_actors, and all of the testing/test_actors/actors/* packages.

Is this reasonable and OK? I have to defer to @Stebalien || @anorth on what we expect to be released from here.

@galargh
Copy link
Author

galargh commented Jul 24, 2024

@rvagg thanks for the note. I took the list of packages we might want handled from

members = [
If some should be omitted, please let me know.

Also, as per the description, this doesn't handle cargo publishing at the moment. The most straightforward way to handle that would be to follow a similar flow to what we proposed in - filecoin-project/builtin-actors#1571 (comment) There, the post-GitHub Release publishing step is to upload assets to the release; here, it could be to perform cargo publishing.

@BigLep
Copy link
Member

BigLep commented Jul 24, 2024

Looking at this more, I think we need @Stebalien input. I'll point him at this.

On the surface, I would assume we should just tag what has previously been tagged. Looking at https://github.com/filecoin-project/actors-utils/tags, that appears to be:
frc42_dispatch
frc42_hasher
frc42_macros
frc46_token
frc53_nft
fvm_actor_utils
fvm_dispatch_tools

I also see this repo hasn't traditionally created GitHub releases: https://github.com/filecoin-project/actors-utils/releases . I don't know if that's intentional or not.

@anorth
Copy link
Member

anorth commented Jul 25, 2024

On the surface, I would assume we should just tag what has previously been tagged

Yes, I think that would be the right place to start. Tagging/releasing/publishing crates only used for testing doesn't seem helpful.

@Stebalien
Copy link
Member

Stebalien commented Jul 26, 2024

Yeah, two bits of feedback:

  1. I'd much prefer to tag based on package names instead of directories.
  2. Let's not tag/publish crates marked as publish = false in the Cargo.toml file.

Note: the cargo metadata command (specifically, cargo metadata --no-deps is particularly useful (no parsing toml). For example, you don't even have to look at individual packages, just run cargo metadata on both the base branch and the head branch, and look for changed versions, publish = false, etc.

@rvagg

frc42_dispatch/hasher and frc42_dispatch/macros would be new packages that haven't previously been tagged here if those Cargo.toml files are touched.

We have been publishing those as frc42_hasher and frc42_macros.

@galargh
Copy link
Author

galargh commented Jul 28, 2024

I now limited the list of packages that are to be released with the automation and also updated the separtor to "@" so the packages will be tagged as crate_name@crate_version.

Here's the current list of config files that will be looked at:

      "dispatch_examples/greeter/Cargo.toml",
      "frc42_dispatch/Cargo.toml",
      "frc42_dispatch/hasher/Cargo.toml",
      "dispatch_examples/greeter/Cargo.toml",
      "frc42_dispatch/Cargo.toml",
      "frc42_dispatch/hasher/Cargo.toml",
      "frc42_dispatch/macros/Cargo.toml",
      "frc46_token/Cargo.toml",
      "frc53_nft/Cargo.toml",
      "fvm_actor_utils/Cargo.toml",
      "fvm_dispatch_tools/Cargo.toml"

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This works but it requires duplicating information already present in the Cargo.toml files, which is unfortunate. Is there any way to extract this information from those files?

If that's difficult, having a single list of "released" crates would likely be good enough, especially if we can code-gen it.

.github/workflows/release-check.yml Outdated Show resolved Hide resolved
.github/workflows/release-check.yml Show resolved Hide resolved
.github/workflows/releaser.yml Outdated Show resolved Hide resolved
.github/workflows/release-check.yml Outdated Show resolved Hide resolved
@BigLep
Copy link
Member

BigLep commented Aug 1, 2024

2024-08-01 conversation between IPDX and FilOz: @galargh will incorporate comments week of 2024-08-05 so we can get this merged.

@galargh
Copy link
Author

galargh commented Aug 5, 2024

I applied the requested changes in 58afb97. I'll now proceed to add a similar commit to the PR in ref-fvm. Let me know if there's anything else you think this needs.

@BigLep BigLep requested a review from Stebalien August 5, 2024 22:40
@BigLep
Copy link
Member

BigLep commented Aug 5, 2024

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

@galargh : can you add a README section about the release process like we have in some of the repos so it's clear to someone about the set of steps to take? Something like:

1. Open a PR adjusting `version` in any of the Cargo.toml files.  This in turn causes X to occur.
2. When the PR is is merged, then X happens.
3. Afterwards you need to manually need to upload assets until https://github.com/filecoin-project/actors-utils/issues/181 is addressed

(I don't know if the above is accurate.)

Or drop me a couple of notes and I can happily do it. I'm actually confused what how much of the release process is manual vs. automated, especially since this repo doesn't have GitHub Releases. I assume I'm just missing things given being tired and having confusion compared to the other recent Rust repos we've been touching.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

As far as this goes, it seems reasonable but won't actually be useful until we can:

  1. Publish on release.
  2. Perform a dry-run before releasing.

Just double checking: the tags are now crate_name@version? I see it is.

@galargh
Copy link
Author

galargh commented Aug 13, 2024

As far as this goes, it seems reasonable but won't actually be useful until we can:

  1. Publish on release.
  2. Perform a dry-run before releasing.

Just double checking: the tags are now crate_name@version? I see it is.

Yes, agreed 💯

I described the process proposed here and future improvements in e4daf65.

I think we should decide whether the partial automation is useful for this repo or we should wait until we can also fully automate the cargo publish steps.

@coveralls
Copy link

coveralls commented Aug 13, 2024

Pull Request Test Coverage Report for Build 10383821682

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.13%

Totals Coverage Status
Change from base Build 10259573166: 0.0%
Covered Lines: 3780
Relevant Lines: 4241

💛 - Coveralls

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 documenting. This seems better than what we had before in that have more docs and a few less manual steps. I'll let @Stebalien make the call whether this is worth it or not though.

RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.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.
2. **[MANUAL]** Run `cargo publish --dry-run` for each crate that is proposed to be released in the reverse dependency order.
Copy link
Member

Choose a reason for hiding this comment

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

How does someone find the reverse dependency order?

Copy link
Member

Choose a reason for hiding this comment

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

And how does this get done for the crate? we have a workspace here so do we need to -p to do this or do we cd into the directory to run this? I'm not clear on how publishing on workspaces works so this is a bit of a mystery to me.

RELEASE.md Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
Co-authored-by: Steve Loeppky <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
@BigLep
Copy link
Member

BigLep commented Aug 15, 2024

Closing this PR for identical reasons to filecoin-project/ref-fvm#2027 (comment) . Bummer :(

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 by adjusting https://crates.io/crates/fvm_actor_utils and getting filecoin-project/github-mgmt#58 landed.

@BigLep
Copy link
Member

BigLep commented Aug 15, 2024

(Turns out I don't permission to close this.)

@Stebalien Stebalien closed this Aug 15, 2024
@galargh galargh deleted the ipdx/unified-github-workflows branch August 23, 2024 09:51
@galargh
Copy link
Author

galargh commented Aug 23, 2024

Same as in filecoin-project/ref-fvm#2027 (comment), thank you all for your time and knowledge - truly appreciated 🙇

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.

6 participants