feat: Release tools #93
Conversation
fmarek-kindred
commented
Oct 30, 2023
fmarek-kindred
commented
- The shell script to bump versions and tag the project
- The workflow to catch a new tag and publish crates
- Work also contains the rename of dependency metrics to talos_metrics
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #93 +/- ##
==========================================
- Coverage 57.36% 57.35% -0.01%
==========================================
Files 115 113 -2
Lines 5747 5666 -81
==========================================
- Hits 3297 3250 -47
+ Misses 2450 2416 -34
☔ View full report in Codecov by Sentry. |
| @@ -22,3 +22,7 @@ serde_json = "1.0" | |||
| # Async | |||
| tokio = { version = "1", features = ["full", "test-util"] } | |||
| async-trait = "0.1" | |||
|
|
|||
| [workspace.metadata.release] | |||
There was a problem hiding this comment.
This section is being read by cargo-release utility.
| exit 1 | ||
| fi | ||
|
|
||
| NEXT_VERSION=$(echo $ANSWER | sed 's/-dev//')-dev |
There was a problem hiding this comment.
Why are we creating and tagging a dev version?
There was a problem hiding this comment.
We are not tagging dev version.
Why we are creating was explained on Thu in our meeting. In short, the previous version was already tagged, we everything what happens with sources (like development and testing of additional commits) after that does not belong to tagged version any more. Its a standard process.
There was a problem hiding this comment.
This is not really a standard practice I have seen, that is why I would like to understand more on this.
Looking at the script, between the tagging of NEW_VERSION and NEXT_VERSION the only commit inserted is this one here:-
git commit -a -m "chore(npm): Release npm $NEW_VERSION" --no-verifyThere is really no need to have NEXT_VERSION, We can always look at the last tag and interpret everything from then is not published.
You can look at tokio, serde or other popular packages, not just in rust but js/other languages as well, there are subsequent commits in master since the last release, but the version is not changed to some alpha, beta, dev..etc.
If something version needs to be tested as alpha, beta..etc, that is only when we create those tags.
There was a problem hiding this comment.
Looking at the script, between the tagging of NEW_VERSION and NEXT_VERSION the only commit inserted is this one here
There is also cargo release line which invokes cargo-release tool. This tool is changing versions in all Cargo.toml files and committing that change. Obviously it cannot do it for NPM modules, so we do it ourselves using the line you copied in your comment.
There was a problem hiding this comment.
There is really no need to have NEXT_VERSION
Properly written apps include version number into bundled build. For example, a website may have an invisible watermark in the footer section saying current version is vXYZ, or sent into Google Analytics and etc. The API server may include http header with x-app-version: vXYZ. This info is also used when tagging log lines which are sent to observability aggregator. This information is taken from metadata included in the executable. That metadata typically comes from package.json or from Cargo.toml or from build.gradle (in Java) and etc. When we are running the app which is built after TAG we want to show the true version. If package.json was not updated then app would display NEW_VERSION, but it may be incorrect if between TAG and now there are commits.
I don't understand why in the meeting you had no problems with it, but after I spent time implementing now it seems you are agains a simple version bump with -dev suffix? What harm does it do?
There was a problem hiding this comment.
I probably didn't understand the usage of the NEXT_VERSION well, till I actually looked into the implementation.
I agree on having the dev or beta etc tag is helpful, but I just feel we shouldn't make it strict/mandatory to have the second tag, on the release commit, but only have it on need basis. I think this is the part I must have not understood in the last meeting.
| echo "Otherwise type version number, for example, type '2.0.0' the tag name will be v2.0.0" | ||
| unset ANSWER | ||
| read ANSWER | ||
| if [ "${ANSWER}" == "a" ] || [ "${ANSWER}" == "" ]; |
There was a problem hiding this comment.
Why not pass the tag via arguments?
There was a problem hiding this comment.
This way we could later plug it as an action in github if required
There was a problem hiding this comment.
We create a tag first and then execute publish action. What action do you refer to?
There was a problem hiding this comment.
-
I tried earlier today to run the script, and although I ran into some other error related to napi, this led me to think, everytime I run the script and there are errors, when I re-run it, it's pretty cumbersome to repeat the inputs vs just using arguments
-
I think this script is doing too much. This should be run as a pre-publish step with just the below lines
echo """
Bumping versions of all crates to $NEW_VERSION
Bumping versions of the following NPMs:
- cohort_sdk_client $NEW_VERSION
- packages/cohort_sdk_js $NEW_VERSION
"""
currentDir=$(pwd)
cd cohort_sdk_client
npm version $NEW_VERSION
cd $currentDir
cd packages/cohort_sdk_js
npm version $NEW_VERSION
cd $currentDir
git add --all
git commit -a -m "chore(npm): Release npm $NEW_VERSION" --no-verify
# This will update version in Cargo.toml files and in dependencies, then commit
cargo release --workspace --no-confirm --no-tag --no-publish --no-push -x $NEW_VERSION
returnStatus=$(($?+0))We shouldn't control through the script how the versions are created, we don't need to. Any one should be able to use standard git tag command and create a tag and push.
The publish github action should get the versions ready for publishing to npm and cargo via the above lines of code.
And finally publish.
There was a problem hiding this comment.
Sorry you have to explain it in details. The way you described will not work, because git tag is not the only step here.
Lets me say that using of script is optional and all can be done manually. With script it is easier as it makes sure that there are no mistakes in "releasing" from dirty woking tree, or from non-master branch, unless person specifies it explicitly with argument. With script tag annotation and commit annotations become standard, without typos. Version bumps are automated and committed. In the end of the day the release script is bumping versions, committing and tagging. Tag push is manual step of course.
Do you have another release process in mind?
There was a problem hiding this comment.
No, the script is very useful. But what I am suggesting is we shouldn't enforce creating the tags only using this script.
So right now, this script is responsible for
NEW VERSION
- Getting the version from user
- Bumping versions in
package.json- Bumping versions in
Cargo.toml- Create a release commit
- Tagging the repo
-NEXT VERSION- Getting the version from user
- Bumping versions in
package.json- Bumping versions in
Cargo.toml- Create a release commit
What I was suggesting was:
NEW VERSION
- Getting the version from user
- Bumping versions in
package.json- Bumping versions in
Cargo.toml- Create a release commit
- Tagging the repo
NEXT VERSION
- Bumping versions in
package.json- Bumping versions in
Cargo.toml- Create a release commit
- Push the commit and tag to remote.
This way, we can use the script in as part of github actions in publish.yaml as the first step before the publishing step.
| if [ "${ANSWER}" != "y" ]; | ||
| then | ||
| echo "Your answer was '${ANSWER}'" | ||
| exit 1 |
There was a problem hiding this comment.
Shouldn't this be exit 0 ?
There was a problem hiding this comment.
It should be 1 to indicate error. Don't we use usually use != 0 as indicator of abort in shell?
There was a problem hiding this comment.
This is related to my understanding of not really needing NEW_VERSION tag, that is why it is safe to exit with 0
