[WIP] Feature post release hook #438

Person-93 · 2022-03-29T18:25:16Z

Here's my first crack at it. It needs some work, but I'm submitting it for feedback.

Open Questions

What environment variables should be set for the hook? For now, I copied the environment variables from the pre-release hook and added RELEASE_SHA.
- Just RELEASE_TAG and DRY_RUN
What should happen if running git rev-parse HEAD to get the release commit fails? It should probably exit non-zero, but I see that you use specific exit codes. So the question is basically, what exit code?
- We won't do this, we'll give the tag instead.
What should happen if the post-release hook exits non-zero? I think it makes sense to say that the hook is responsible for rolling back any changes it made and any changes made in the pre-release hook. Maybe cargo-release should yank the new release if it was already made?
- Just exit with an error
Should the post release hook be run if the pre-release hook fails? If we consider the post-release hook to be responsible for rolling back changes, maybe it makes sense to run it even if the release was aborted?
- No
Should there be a separate on-abort hook so the post-release hook isn't responsible for cleaning up an aborted release?
- Maybe but now as part of this feature.
How should this be tested?
- Dummy project and trycmd or snapbox.

Todo

Test
Update documentation
- Add the new Config field post-release-hook to docs/reference.md
- Document environment variables that are set
- Document failure behaviour

epage · 2022-03-29T18:42:29Z

What should happen if running git rev-parse HEAD to get the release commit fails? It should probably exit non-zero, but I see that you use specific exit codes. So the question is basically, what exit code?

If the crate handles it, then its not our problem :)

Should the post release hook be run if the pre-release hook fails? If we consider the post-release hook to be responsible for rolling back changes, maybe it makes sense to run it even if the release was aborted?

Should there be a separate on-abort hook so the post-release hook isn't responsible for cleaning up an aborted release?

My thoughts

We shouldn't specialize behavior on pre-release failure
However, we could have an "abort" hook that runs if any step fails in releasing a crate
However, let's not worry about an abort hook until someone has a clear use case.

How should this be tested?

One option is to have a dummy project to release with hooks that print messages. We then use trycmd or snapbox to run a release and we then verify that the hook output came through.

Person-93 · 2022-03-29T19:03:22Z

What should happen if running git rev-parse HEAD to get the release commit fails? It should probably exit non-zero, but I see that you use specific exit codes. So the question is basically, what exit code?

If the crate handles it, then its not our problem :)

The way I set it up is that cargo-release sets an environment variable with the release commit. If commits are not consolidated, it could be tricky for the hook to figure out which commit it's supposed to operate on.

My thoughts

* We shouldn't specialize behavior on pre-release failure

* However, we could have an "abort" hook that runs if any step fails in releasing a crate

* However, let's not worry about an abort hook until someone has a clear use case.

Sounds good. Does this also mean that the release shouldn't be yanked on failure?

How should this be tested?

One option is to have a dummy project to release with hooks that print messages. We then use trycmd or snapbox to run a release and we then verify that the hook output came through.

Sounds good.

epage · 2022-03-29T19:06:27Z

The way I set it up is that cargo-release sets an environment variable with the release commit. If commits are not consolidated, it could be tricky for the hook to figure out which commit it's supposed to operate on.

What if we provide the tag instead?

Sounds good. Does this also mean that the release shouldn't be yanked on failure?

Yes, let's leave all failure policy to the script

epage · 2022-03-29T19:07:09Z

Oh, have you looked at how other release processes from other ecosystems deal with this? I'm assuming there is similar for node and others. We should make a list of them to regularly consult for ideas.

Person-93 · 2022-03-29T19:42:58Z

What if we provide the tag instead?

I added this function which gets the head sha. I'm not sure what to do if it fails.

epage

Forgot to publish these

src/release.rs

epage · 2022-03-29T18:38:37Z

src/release.rs

+            if !cmd::call_with_env(post_rel_hook, envs, cwd, false)? {
+                todo!("handle non-zero exit from post-release hook")
+            }


What should happen if the post-release hook exits non-zero? I think it makes sense to say that the hook is responsible for rolling back any changes it made and any changes made in the pre-release hook. Maybe cargo-release should yank the new release if it was already made?

I think we should log the error as a warning and document it as no-fail

I think we should log the error as a warning and document it as no-fail

I'm not so sure about that. If that's the desired behavior, the hook can do that. I think it should be up to the hook to decide.

I'm torn on this one. For a high quality one, they will write it out. For what most people will probably write, the assistance could be useful.

What if we let it fail and add an error message stating that the hook should convert the error to a warning if that's the desired behavior?

Good point, the hook, if its handling errors, can decide whether to give an error exit code.

Let's go ahead and warn if the hook fails but we shouldn't be telling people what to do about the hook.

src/config.rs

Person-93 · 2022-03-29T20:05:25Z

Forgot to publish these

??

Person-93 · 2022-03-29T20:14:08Z

Oh, have you looked at how other release processes from other ecosystems deal with this? I'm assuming there is similar for node and others. We should make a list of them to regularly consult for ideas.

I looked briefly at npm. They have all the pre and post lifecycle scripts. If the postpublish script fails, it'll be reported as a failure. It won't automatically undo the release, but there's a separate unpublish command and there's nothing stopping the postpublish script from running it before exiting non-zero.

Person-93 · 2022-03-31T00:19:40Z

I've made the changes we discussed and added two failing tests. The hook does not run. I tried to copy the same logic that determines when to run the pre-release hook. I'm not sure where I went wrong.

Person-93 · 2022-03-31T00:19:54Z

I've made the changes we discussed and added two failing tests. The hook does not run. I tried to copy the same logic that determines when to run the pre-release hook. I'm not sure where I went wrong.

Person-93 · 2022-03-31T00:22:03Z

src/release.rs

-    // STEP 7: git push
+    // STEP 7: post-release hook
+    for pkg in pkgs {
+        if let (Some(post_rel_hook), Some(_)) =


@epage, I think this if statement isn't checking the right thing. I don't know what it should be checking. Can you advise?

The tests are not doing a version bump and most of actions are only on version bump.

epage · 2022-04-01T18:06:34Z

src/release.rs

+            let post_rel_hook = post_rel_hook.args();
+            log::debug!("Calling post-release hook: {:?}", post_rel_hook);
+            let envs = maplit::btreemap! {
+                OsStr::new("RELEASE_TAG") => OsStr::new(pkg.tag.as_ref().expect("post-release hook with no tag")),


I'd recommend we be consistent with the placholders: https://github.com/crate-ci/cargo-release/blob/master/docs/reference.md#placeholders

epage · 2022-04-01T18:06:52Z

src/release.rs

+            log::debug!("Calling post-release hook: {:?}", post_rel_hook);
+            let envs = maplit::btreemap! {
+                OsStr::new("RELEASE_TAG") => OsStr::new(pkg.tag.as_ref().expect("post-release hook with no tag")),
+                OsStr::new("DRY_RUN") => OsStr::new(if dry_run { "true" } else { "false" }),


How come a lot of the pre-release variables were removed?

epage · 2022-04-01T18:07:18Z

tests/post-release-hook/main.rs

@@ -0,0 +1,132 @@
+use assert_fs::prelude::*;


I'd prefer this to be tests/post-release-hook.rs

epage · 2022-04-01T18:07:34Z

tests/post-release-hook/workspace/.gitignore

@@ -0,0 +1 @@
+../crate/.gitignore


Please store all of these in the test/fixtures directory

epage · 2022-04-01T18:08:40Z

tests/post-release-hook/main.rs

+}
+
+fn git(dir: &Path, args: &[&str]) {
+    assert!(Command::new("git")


snapbox::cmd::Command might be a good option

epage · 2022-04-01T18:08:56Z

tests/post-release-hook/main.rs

+fn run_cargo_release(dir: &Path) -> String {
+    let temp = TempDir::new().unwrap();
+    temp.copy_from(dir, &["**"]).unwrap();
+
+    git(temp.path(), &["init"]);
+    git(temp.path(), &["add", "."]);
+    git(
+        temp.path(),
+        &["commit", "--message", "this is a commit message"],
+    );
+
+    let mut cargo = env::var_os("CARGO").map_or_else(|| Command::new("cargo"), Command::new);
+    let output = cargo
+        .stderr(Stdio::piped())
+        .args(["run", "--manifest-path"])
+        .arg(Path::new(PROJECT_ROOT).join("Cargo.toml"))
+        .args(["--", "release", "-vv"])
+        .current_dir(&temp)
+        .spawn()
+        .unwrap()
+        .wait_with_output()
+        .unwrap();
+
+    temp.close().unwrap();
+
+    if !output.status.success() {
+        panic!("cargo release exited with {}", output.status);
+    }
+    let output = String::from_utf8(output.stderr).unwrap();
+    eprintln!("{output}");
+    output
+}


Please use snapbox::cmd::Command and snapbox::cmd::cargo_bin!

epage · 2022-04-01T18:11:12Z

tests/post-release-hook/main.rs

+fn run_cargo_release(dir: &Path) -> String {
+    let temp = TempDir::new().unwrap();
+    temp.copy_from(dir, &["**"]).unwrap();
+
+    git(temp.path(), &["init"]);
+    git(temp.path(), &["add", "."]);
+    git(
+        temp.path(),
+        &["commit", "--message", "this is a commit message"],
+    );
+
+    let mut cargo = env::var_os("CARGO").map_or_else(|| Command::new("cargo"), Command::new);
+    let output = cargo
+        .stderr(Stdio::piped())
+        .args(["run", "--manifest-path"])
+        .arg(Path::new(PROJECT_ROOT).join("Cargo.toml"))
+        .args(["--", "release", "-vv"])
+        .current_dir(&temp)
+        .spawn()
+        .unwrap()
+        .wait_with_output()
+        .unwrap();
+
+    temp.close().unwrap();


If you have reusable fixture initialization, I'd recommend splitting that out into a dedicated function and have each test call cargo-release in its own way. This will make it easier to add new test cases in the future

epage · 2022-04-01T18:13:40Z

tests/post-release-hook/crate/src/main.rs

+
+fn main() {
+    eprintln!("START_ENV_VARS");
+    for (key, value) in env::vars_os() {


If we ensure these are sorted, we can use snapboxs built-in snapshot testing

We'd just define the following and snapbox's asserter will take the ... and treat it as a multi-line wildcard.

START_ENV_VARS ... DRY_RUN=true ... RELEASE_TAG=post-release-hook-ws-a-v42.42.42 ... END_ENV_VARS

epage reviewed Mar 29, 2022

View reviewed changes

Person-93 added 4 commits March 30, 2022 19:49

refactor(config): Add "post_release_hook" field to Config

c38ce58

feat: Add support for post-commit hook

9c64d22

test: Add integration test for post-release hook

fc4f117

feat: Exit 107 on post-release hook fail

811dd46

Person-93 force-pushed the feature-post-release-hook branch from e50cb1a to 811dd46 Compare March 31, 2022 00:15

Person-93 commented Mar 31, 2022

View reviewed changes

epage reviewed Apr 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feature post release hook #438

[WIP] Feature post release hook #438

Person-93 commented Mar 29, 2022 •

edited

epage commented Mar 29, 2022

Person-93 commented Mar 29, 2022

epage commented Mar 29, 2022

epage commented Mar 29, 2022

Person-93 commented Mar 29, 2022

epage left a comment

epage Mar 29, 2022

Person-93 Mar 29, 2022

epage Mar 30, 2022

Person-93 Mar 30, 2022

epage Mar 30, 2022

Person-93 commented Mar 29, 2022

Person-93 commented Mar 29, 2022

Person-93 commented Mar 31, 2022

Person-93 commented Mar 31, 2022

Person-93 Mar 31, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

epage Apr 1, 2022

		@@ -0,0 +1 @@
		../crate/.gitignore

[WIP] Feature post release hook #438

Are you sure you want to change the base?

[WIP] Feature post release hook #438

Conversation

Person-93 commented Mar 29, 2022 • edited

Open Questions

Todo

epage commented Mar 29, 2022

Person-93 commented Mar 29, 2022

epage commented Mar 29, 2022

epage commented Mar 29, 2022

Person-93 commented Mar 29, 2022

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Person-93 commented Mar 29, 2022

Person-93 commented Mar 29, 2022

Person-93 commented Mar 31, 2022

Person-93 commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Person-93 commented Mar 29, 2022 •

edited