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

Introduce bump.sh script to automate version bumping #27

Merged
merged 31 commits into from
Nov 15, 2022
Merged

Conversation

ctmbl
Copy link
Contributor

@ctmbl ctmbl commented Nov 5, 2022

Context:
#19 brought the first version bump for the iScsc website. However, this one was achieved manually as described in the wiki page but this procedure wasn't expected to last long.

Problem:
The version bump, even if its procedure should be described, shouldn't be achieved manually.

Solution:
I then looked for automatic tools to bump version of software and especially JS, node.js and website software.
I found:

So I first went with npm-version because it seems like the more reliable and simple, and I built a sh script around it.

Script details:
The script takes one argument which is the new version and apply it to package.json, frontend/package.json and backend/package.json.
The script will in order:

  • check that:
    • needed packages are installed: semver (from npm), npm, git
    • supplied version is semantically correct and > than current
    • working directory is clean
  • checkout on main and switch to a new branch I'm not quite convinced of that part, I would like your opinion
  • bump versions
  • run npm install to update the *-lock.jsons
  • commit frontend and backend together then commit and tag root separately
  • try to push to remote

It partially relies on package.json scripts to do this job as well as npm version!

Caveats, Limitations and Testing:
Currently the script is safe, I think, I tried to write it without assuming anything about the user current project state but you may find some mistakes 😕
I must say that the output isn't the clearer you ever saw but it does the job.
You can't fully test it right now, because it will checkout on main and then don't have the right package.jsons scripts state (it won't commit and output an error because of this when bumping root).
It has a "dry run" mode to test it without touching the user's repo: try DRY_RUN=foo ./bump 0.1.1

What's next?
I don't think that this script will stays as it is for long, right after finishing to write this PR I'll go try release-it, but this is a first step!

@ctmbl ctmbl added enhancement New feature or request Priority: Medium The Issue must be addressed as soon as possible Severity: Minor The bug or Issue can be annoying for the user labels Nov 5, 2022
@ctmbl ctmbl added this to the iScsc blog v0.2.0 milestone Nov 5, 2022
@ctmbl ctmbl requested review from atxr, amtoine and Turtyo November 5, 2022 19:06
@ctmbl ctmbl self-assigned this Nov 5, 2022
Copy link
Contributor

@atxr atxr 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 your work!

checkout on main and switch to a new branch I'm not quite convinced of that part, I would like your opinion

What's your problem about this? It looks good to me 🤔

I have several points I want to discuss:

  • Where do you actually bump the version? Like where are you changing the version in the package.json
  • Maybe switching back to the original branch at the end could be nice
  • Do we need to merge manually the pushed branch at the end
  • A little doc on README would be nice! At least saying this feature is available and add the corresponding files to the tree

I will try the script after your answer

package.json Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

well the script looks great!
hard to tell if there is a bug without testing, but great work 💪

i've got generatlly the same questions as @atxr, and did add some more of them and as well as some remarks and suggestions

i think the script works fine as it is, so i do not require changes nor approve explicitely 👌

backend/package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
@ctmbl
Copy link
Contributor Author

ctmbl commented Nov 9, 2022

Thanks for the review to both of you @atxr and @amtoine !!

Where do you actually bump the version? Like where are you changing the version in the package.json

The npm version actually does the job! to see this behavior try:

git checkout origin/main
git switch -c tmp
npm version major
OR 
npm version 1.5.3
git show

Maybe switching back to the original branch at the end could be nice

Actually I could, but even if I log the script actions it would be a little too "hiding" to me 😕 I prefer to let the user on the new branch to force it to see the new changes, can also be useful in case the push fails!
it makes me think that I could add a git show at the end to display the new commits and tag!

Do we need to merge manually the pushed branch at the end

Yes and that's intended, no one should have the right to directly push to main, but you'll see that I push on the iscsc repository, not the forked one, so the remote branch will be on the main repo and anyone will be able to open a PR!

A little doc on README would be nice! At least saying this feature is available and add the corresponding files to the tree

You're right I'll add this (you can even open a thread review to force me to do so before the merge! (because the merge is blocked while there are still opened review threads))

I hope that answer your questions!

@amtoine I'm answering your remarks soon!
@Turtyo will you have the time to look at that PR?

Copy link
Contributor

@atxr atxr left a comment

Choose a reason for hiding this comment

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

Some little changes requested here
Also, can you just add an example of the command to run in the wiki to bump the version?

README.md Show resolved Hide resolved
bump.sh Outdated Show resolved Hide resolved
@ctmbl
Copy link
Contributor Author

ctmbl commented Nov 12, 2022

Here I come with the last version of the scrip I hope!

Changelog:

  • Move the postversion scripting of root package.json in bump.sh--> makes more sense
  • Add exit in case of failure of commit or root bumping
  • Add a warning at the end for reviewing the PR because running npm install updates the *lock.jsons and so may add unwanted updates to the PR!!
  • Fix the use of subshell

The last changes to be done now is I think coloring the logs and maybe refactoring the code! (even if I think it is pretty linear so functions may not help much)

@ctmbl ctmbl requested review from atxr and amtoine November 12, 2022 07:18
@amtoine
Copy link
Member

amtoine commented Nov 12, 2022

Here I come with the last version of the scrip I hope!

Changelog:

  • Move the postversion scripting of root package.json in bump.sh--> makes more sense
  • Add exit in case of failure of commit or root bumping
  • Add a warning at the end for reviewing the PR because running npm install updates the *lock.jsons and so may add unwanted updates to the PR!!
  • Fix the use of subshell

The last changes to be done now is I think coloring the logs and maybe refactoring the code! (even if I think it is pretty linear so functions may not help much)

maybe the PR should have been a draft 'till that point, right? 😕
i don't see any connection between these changes and the review threads 🤔

don't get me wrong, the PR is fine and the changelog looks cool 😉
and i'll review that as soon as possible 👍

EDIT

these changes precisely address my thread right? 👀
i might have missed the point, sorry for the confusion if so 😕

if the thread and the 8a95601..d681cf5 are related, i'll approve 😌

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

the last changes look fine 👌

still waiting for my thread to be closed and i'll approve the changes 👌

the changelog

git diff 8a95601..d681cf5

@ctmbl
Copy link
Contributor Author

ctmbl commented Nov 12, 2022

@amtoine

these changes precisely address my thread right?

Ahaah absolutely! But you figured it out so that's fine!
And you're right I absolutely should have provide a git diff sorry for that...

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

all of this looks good to me 😋

@amtoine
Copy link
Member

amtoine commented Nov 13, 2022

as a friendly reminder and in order to complete this successful PR:

  • @atxr, you'll need to approve the changes explicitely
  • @ctmbl, you'll need to fix the conflicts with the main branches
    i think, git merge is less confusing in the PR review process than git rebase 🤔
    but that's my opinion 😋

😉

Resolve README.md conflicts and package.json
@ctmbl
Copy link
Contributor Author

ctmbl commented Nov 14, 2022

@amtoine
It's done! Nothing too severe just some conflicts due to #35 and #26 merges!
And tks for the git merge tips: actually you're right I find it far cleaner!

@amtoine
Copy link
Member

amtoine commented Nov 14, 2022

It's done! Nothing too severe just some conflicts due to #35 and #26 merges!

i see that, makes sense 👌

And tks for the git merge tips: actually you're right I find it far cleaner!

yeaaah 😋

i think rebasing is the best when working on personal branches, i.e. branches only one person works on
however, when multiple persons are on the same branch / a PR review is in progress, a merge commit, especially when "rebase merging" is the PR strategy, as the best 😉 😌

let's wait for the remaining reviews 😋

Copy link
Contributor

@atxr atxr left a comment

Choose a reason for hiding this comment

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

All my threads all resolved!
And the script is quite clean now!
Ready to merge 🚀

@amtoine
Copy link
Member

amtoine commented Nov 15, 2022

last thing, do we wait for @Turtyo? 🤔

@ctmbl
Copy link
Contributor Author

ctmbl commented Nov 15, 2022

@amtoine

last thing, do we wait for @Turtyo? thinking

Don't think so he had plenty of time to review this one 😕

@ctmbl ctmbl merged commit 1087a26 into iScsc:main Nov 15, 2022
@ctmbl ctmbl deleted the bumper branch November 15, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority: Medium The Issue must be addressed as soon as possible Severity: Minor The bug or Issue can be annoying for the user
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants