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
Release via Github Actions workflow #999
Release via Github Actions workflow #999
Conversation
18f2543
to
8a89d68
Compare
Thanks for working on this @nicholasbishop , I like the idea. Once this is merged, I update #568 (if not obsolete now) and then we close this one was well. |
xtask/src/util.rs
Outdated
@@ -44,6 +44,19 @@ pub fn run_cmd(mut cmd: Command) -> Result<()> { | |||
} | |||
} | |||
|
|||
/// Print a `Command` and run it, then check that it completes | |||
/// successfully. Return the command's stdout. | |||
pub fn get_cmd_stdout(mut cmd: Command) -> Result<Vec<u8>> { |
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.
nit: pub fn run_command(mut cmd: Command) -> Result<String>
- From
cmd.output()
it is not obvious that the command is executed here. I had to double-check that in the docs - I think we can already check here whether we have a string, right? Anything non-utf8 should be a failure anyway, right?
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.
Renamed to run_cmd_get_stdout
-- we already have a run_cmd
above, which is useful when you want the stdout/stderr to not be hidden (like when running most cargo commands).
Changed to convert to a String
as suggested.
@@ -24,5 +26,5 @@ sha2 = "0.10.6" | |||
syn = { version = "2.0.0", features = ["full"] } | |||
tar = "0.4.38" | |||
tempfile = "3.6.0" | |||
ureq = { version = "2.8.0", features = ["http-interop"] } |
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.
nit: any reason why you prefer ureq
over hyper
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.
No super strong reason, but generally I agree with this section of the ureq docs: https://docs.rs/ureq/2.8.0/ureq/#blocking-io-for-simplicity
Basically, async IO (which hyper is built around) doesn't add much for a very low-traffic client like this, and would also significantly expand the dep tree.
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.
Got it. I don't have a strong opinion here either.
xtask/src/opt.rs
Outdated
@@ -202,3 +203,9 @@ pub struct FmtOpt { | |||
#[clap(long, action)] | |||
pub check: bool, | |||
} | |||
|
|||
/// Run the auto-release process. |
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.
Please add something like This is tied to the GitHub CI. or similar. We should also ensure that no unexpected things happen, when people run this locally.
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.
Updated to:
/// Run the auto-release process (should only be run by Github Action).
///
/// This is run by the `release.yml` workflow. It is not intended to be run
/// locally, and will exit immediately if `GITHUB_SHA` is not set.
So unless someone goes out of their way to set GITHUB_SHA
it won't do anything when run locally, and even then it won't do publish anything unless that user has permissions to push to the repo/crates.io.
.github/workflows/release.yml
Outdated
@@ -0,0 +1,20 @@ | |||
# This workflow runs when commits are pushed to main or a branch starting with | |||
# "version-". It checks if any packages require a new release, and if so, | |||
# creates the crates.io release and git tag. |
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.
tag
-> tags
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.
done
- uses: Swatinem/rust-cache@v2 | ||
- run: cargo xtask auto-release | ||
env: | ||
CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }} |
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.
Is this token bound to your account? How did you create it?
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.
Yes, this token is part of my account, although there's nothing really specific to my account -- any of the three of us with owner access to the uefi crates can create a token at https://crates.io/settings/tokens/new with the publish-update
scope.
A potential alternative would be to create a service account instead of using a personal account, but then we'd need some way to manage credentials for that, so probably more trouble than it's worth for now.
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.
Yeah, I agree.
Generally speaking, I like the idea. Thanks for working on it! However, I'm concerned regarding the additional complexity in |
8a89d68
to
0d1d4d6
Compare
Definitely a fair point. I think what I'd really like is to have this new code in its own library on crates.io for easy re-use, then we wouldn't need to maintain it in this repo. $dayjob makes it easy for me to contribute to existing open source repos, but there's significant overhead in me creating a new one myself, so I'm trying to avoid doing that ;) It is possible that we could replace most of the code in |
haha, I can relate 😄 Fine with me. No strong opinion regarding the approach we take. If it works, it's good. IMHO, I'd prefer if we merge this but also update #568 in a way that it first discusses WHAT we want (git tags with specific name) and then ways of HOW we achive this (approach A: github flow, approach B: manual flow) |
This new workflow will simplify our release process and make it more reviewable:
release:
, the rest of the message is arbitrary.Since all the changes are now part of one PR that is created before the release actually occurs, we can now review our release changes just like any other PR. And with the release happening in a github action, it avoids a lot of easy-to-make mistakes such as releasing from the wrong commit or forgetting to push tags.
Obviously it's a little tricky to test release flows, so while I've tested the git-tag portion and a dry-run of the cargo-publish step, there could certainly be some bugs to resolve.
Closes #325
Checklist