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

Add helper script to create patch releases from cherry-picked commits #6602

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

agoscinski
Copy link
Contributor

@agoscinski agoscinski commented Nov 4, 2024

The script helps with creating a patch release that requires to cherry-pick a list of commits, appends the original commit hash to the cherry-picked commit and creating a short summary of it.

I used it for the v2.6.3 release in PR #6604, so any bugs should be revealed there.

@agoscinski agoscinski changed the title Add helper to create minor releases from cherry-picked commits Add helper script to create minor releases from cherry-picked commits Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.90%. Comparing base (ef60b66) to head (9b0d0c8).
Report is 142 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6602      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42147     +703     
==========================================
+ Hits        32120    32831     +711     
+ Misses       9324     9316       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@khsrali
Copy link
Contributor

khsrali commented Nov 5, 2024

Thanks a lot, @agoscinski.
Tests? 😈

@agoscinski
Copy link
Contributor Author

I don't think you want a PR being blocked because a test on a developers util fails that is only require for minor releases. That does not make much sense. The moment you need it, it is also tested by usage.

@khsrali
Copy link
Contributor

khsrali commented Nov 5, 2024

Tests doesn't necessarily need to go in ci, but just have it sort of documented for the expected behavior.
However, I understand that might be an overkill, so I suggest at least add a phrase on top of the file, e.g.:
"The behavior is not guaranteed, please check before push"
just to save time for future developers.
Thanks

Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

This is cool. :-)

exit 1
fi

# The first argument is the GitHub repo identifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment seems outdated since the repo name is hardcoded? (also the one above that mentions that least two arguments are needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True!

Suggested change
# The first argument is the GitHub repo identifier

else
echo "Failed to cherry-pick commit $commit"
# Abort the cherry-pick in case of conflict
git cherry-pick --abort
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you don't check why the cherry pick failed exactly, maybe the safer thing here would be to just break out of the for loop and let the user deal with the failed cherry pick manually?

More importantly, sometimes the order of commits is important so if one cherry pick fails, it's probably better to stop and don't try to cherry pick any other commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True!

@agoscinski
Copy link
Contributor Author

Tests doesn't necessarily need to go in ci, but just have it sort of documented for the expected behavior.

There is a description of the file at the top. You can suggest improvements for it.

@danielhollas
Copy link
Collaborator

@agoscinski btw: there is a wiki page 'How to make a release' that would be good to update once you merge this: https://github.com/aiidateam/aiida-core/wiki/How-to-make-a-new-release#3-cherry-picking-commits

I guess in general these wiki pages are quite outdated, for example the dependency management quide was still referencing a setup.py|setup.json files. 😅 I've updated it a little bit.

@agoscinski agoscinski changed the title Add helper script to create minor releases from cherry-picked commits Add helper script to create patch releases from cherry-picked commits Nov 25, 2024
@agoscinski
Copy link
Contributor Author

Remember update in commit message minor -> patch release

@agoscinski
Copy link
Contributor Author

@agoscinski btw: there is a wiki page 'How to make a release' that would be good to update once you merge this: https://github.com/aiidateam/aiida-core/wiki/How-to-make-a-new-release#3-cherry-picking-commits

Yes, thanks! because the command there was not working as I would like to it to, I made this utility. I will update the wiki once we merge this.

@danielhollas
Copy link
Collaborator

Yes, thanks! because the command there was not working as I would like to it to, I made this utility. I will update the wiki once we merge this.

btw: One idea that I had but didn't have time to mention at the coding week: I wonder if at least some of the documentation in Wiki should be moved into the repo (e.g. as simple markdown files). Otherwise, as we can see, it's very easy for wiki to become outdated, and also it is kinda hidden from sight (perhaps this should be discussed on Discourse not here).

The idea comes among other things from the CPython repo, where they recently started to move the devguide into the repo itself, for similar reasons. See: https://github.com/python/cpython/tree/main/InternalDocs

@unkcpz
Copy link
Member

unkcpz commented Nov 25, 2024

The idea comes among other things from the CPython repo, where they recently started to move the devguide into the repo itself, for similar reasons. See: https://github.com/python/cpython/tree/main/InternalDocs

This is interesting.

I wonder if at least some of the documentation in Wiki should be moved into the repo

The problem is what need to move and what is better to keep in the wiki. @danielhollas you remember that for AiiDAlab we somehow do the opposite to move some guidance to the wiki.
In my opinion, things as guidance/standard that we rarely change can goes to documentation and require review process for changes. Things such as tips and just created but not agreed in the team can leave in the wiki. Once those become standard, can then moved to the doc. But as @danielhollas said, would be good to have simple markdown for those.

Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Looks good!

cherry_picked_hash=$(git log -1 --pretty=format:"%h" HEAD)
commit_summaries+=("- $short_commit_message [[${commit}]](https://github.com/$GITHUB_REPO/commit/${original_long_hash})")
else
echo "Failed to cherry-pick commit $commit"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this message could be extended to say something about possible next steps? But I am not sure.

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 am also not sure what should be the next step^^ Read the error message and try to fix it is a bit too generic to be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. Also I guess the person using this script should know what their doing. 😅

@agoscinski
Copy link
Contributor Author

btw: One idea that I had but didn't have time to mention at the coding week: I wonder if at least some of the documentation in Wiki should be moved into the repo (e.g. as simple markdown files). Otherwise, as we can see, it's very easy for wiki to become outdated, and also it is kinda hidden from sight (perhaps this should be discussed on Discourse not here).

Yes, I self assigned myself at the beginning to do this (see issue #6400), but it was too much work and I did not know what is relevant so I postponed this task to the future.

agoscinski and others added 4 commits November 26, 2024 10:46
The script helps with creating a minor release that requires to
cherry-pick a list of commits, appends the original commit hash to the
cherry-picked commit and creating a short summary of it.
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.

4 participants