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

chore(release): add prep-release script #4473

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Conversation

nathanielc
Copy link
Contributor

The Flux release process will now require a preparation step.
This still will make and commit any changes to the source code, i.e.
updating docs to mark the correct version a function was introduced.

The process will be:

  1. Run ./prep-release.sh in Flux repo
  2. Merge created PR
  3. Run IDPE release script (which calls the Flux release script)

The Flux release script checks that the prep step was completed so it
can't be easily forgotten.

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@nathanielc nathanielc requested a review from a team as a code owner February 10, 2022 21:37
@nathanielc nathanielc requested review from Marwes and removed request for a team February 10, 2022 21:37
@@ -1,7 +1,7 @@
[package]
name = "flux"
version = "0.5.1"
authors = ["jlapacik <[email protected]>"]
version = "0.154.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to add a step to automatically bump the version in the Cargo file too. But the cargo-bump utility we used doesn't yet support workspace based projects. See https://github.com/wraithan/cargo-bump/blob/master/src/config.rs#L102

We can add that behavior of the release later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is one of those things that has always bothered me.

There are a couple of cargo tools that don't with workspaces that I use regularly. It makes me sad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted PR to cargo-bump for workspace support wraithan/cargo-bump#34

@@ -43,6 +43,9 @@ Each key-value pair must be on a single line.
and categorize packages. _See [Metadata tags](#metadata-tags)._
- **contributors**: Contributor GitHub usernames or other contact information.

When adding a new export to a Flux package the `introduced: NEXT` metadata can be added to the value's docs.
The release process will automatically replace the `NEXT` with the version of the Flux release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sanderson FYI we are looking to add this to our release process.

@nathanielc nathanielc requested review from rockstar and removed request for Marwes February 10, 2022 21:43
Copy link
Contributor

@rockstar rockstar left a comment

Choose a reason for hiding this comment

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

Run IDPE release script (which calls the Flux release script)

I wonder if we should also run etc/checkprepared.sh in the idpe script.

I'd also like to see a script that is the opposite of etc/checkprepared.sh so that we can assert that both scripts are only being run in their required states. This might seem like over-engineering, but footguns in the release process are guaranteed to go off more than not.

@@ -0,0 +1,30 @@
#!/bin/bash

# This scripts purpose is to check that any needed changes to the source
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "script's" - that lack of apostrophe is gonna get me.

EXIT=1
echo "$f contains 'introduced: NEXT'"
fi
done < <(find ./stdlib -name '*.flux')
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is quite the bash-ism. I had to read it a couple of times to realize what it did, and then spent a little more time trying to think of how I would implement it, and none of them are as efficient as this is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do it this way so that if we get spaces in file names it doesn't break.

@@ -1,7 +1,7 @@
[package]
name = "flux"
version = "0.5.1"
authors = ["jlapacik <[email protected]>"]
version = "0.154.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is one of those things that has always bothered me.

There are a couple of cargo tools that don't with workspaces that I use regularly. It makes me sad.

@nathanielc
Copy link
Contributor Author

I wonder if we should also run etc/checkprepared.sh in the idpe script.

I'd rather IDPE know as little as possible about the Flux release process. So that is why the Flux release script calls the checkprepared.sh script itself.

I'd also like to see a script that is the opposite of etc/checkprepared.sh so that we can assert that both scripts are only being run in their required states. This might seem like over-engineering, but footguns in the release process are guaranteed to go off more than not.

I am not sure I follow. Like a checkunprepared script? What would it do? Assert that introduced: NEXT is present? It not always going to be present even in an unprepared repo.

The Flux release process will now require a preparation step.
This still will make and commit any changes to the source code, i.e.
updating docs to mark the correct version a function was introduced.

The process will be:

1. Run ./prep-release.sh in Flux repo
2. Merge created PR
3. Run IDPE release script (which calls the Flux release script)

The Flux release script checks that the prep step was completed so it
can't be easily forgotten.
@rockstar
Copy link
Contributor

I'd rather IDPE know as little as possible about the Flux release process. So that is why the Flux release script calls the checkprepared.sh script itself.

I don't think it would be a horrible thing if the IDPE script knew something about our release process (because it is part of the release process, ultimately), but I take your point that if we can avoid it, we should.

I am not sure I follow. Like a checkunprepared script? What would it do? Assert that introduced: NEXT is present? It not always going to be present even in an unprepared repo.

I cannot, for the life of me, remember what I was trying to say here. I think the thing I'm concerned about is that the state of "unprepared" becomes more complex, and maybe the check is about checking versions and tags, etc. I'm not sure that's necessary, at this point. If it gets more complicated, we should certainly think about it, but for now, I retract my statement. :)

@nathanielc nathanielc merged commit 3e6eff6 into master Feb 16, 2022
@nathanielc nathanielc deleted the chore/release-prep branch February 16, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants