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

fix(hack): various fixes & improvements to cherry-pick script #12714

Merged
merged 11 commits into from
Feb 29, 2024

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Feb 29, 2024

Motivation

As Isitha and I now have Approver permissions and were working on the release rotation per #12592, we both used the script and both found some issues, one of which was mentioned in #11997 (comment)

Fix a few bugs in the script and make it more ergonomic

Modifications

bugfixes

  • error out when the current branch that is checked out is not equivalent to the branch we're doing a cherry-pick on, as you can't cherry-pick onto other branches (see also this SO question)
  • correctly order the commits in reverse chronological order -- the commits should be cherry-picked in the same order they were added
    • add a simple tac (reverse cat) to achieve this
  • like the dry-run, commits that aren't going to be cherry-picked should be skipped
    • previously this would error out with no error message, which was very confusing

misc improvements

  • enable dry-run by default, for safety and simplicity (the arg is now optional)

  • add flags to git cherry-pick

    • use -Xpatience to run the slower patience diff strategy for hopefully less merge conflicts
    • use -x to add "(cherry picked from commit ...)"
      • this flag is very useful for posterity for backports as it creates a backlink to the original commit
  • other improvements:
    • remove redundant set -eu -- this only needs to be set once in the script
    • rename variables and functions to be more explicit etc
      • e.g. use conventional get* functions
    • reduce nesting by removing the else

Verification

Run ./hack/cherry-pick.sh release-3.5 fix in a few variations

  • on a different branch
  • with and without dry run

- the commits were being cherry-picked as most recent first, then least recent, when it should be the other way around
  - use `tac` (reverse `cat`) to flip this

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- the line `commit=$(grep -q "$(echo "$m" | prNo)" /tmp/prs && echo "${m:0:9}")` could error out when `grep` had an error, which would fail the script since `set -e` is set
  - it was also duplicated in the `if` branch
  - instead, skip / `continue` when that has an error and simplify the rest of the logic

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- make the function names more explicit and as one might name them in a full programming language, using `get*` prefixes
- correct relative location of the docs

- reduce nesting by removing the `else`

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- the flag is specifically made for backporting

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- unfortunately, cherry-picking onto a different branch is not currently possible, per the in-line comment

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/build Build or GithubAction/CI issues label Feb 29, 2024
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 marked this pull request as draft February 29, 2024 07:27
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 marked this pull request as ready for review February 29, 2024 08:00
@agilgur5 agilgur5 added the area/contributing Contributing docs, ownership, etc issues & PRs label Feb 29, 2024
@terrytangyuan terrytangyuan merged commit 7ba20fe into argoproj:main Feb 29, 2024
15 checks passed
@agilgur5 agilgur5 deleted the fix-cherry-pick-script branch February 29, 2024 08:48
@agilgur5
Copy link
Member Author

For reference, since this script is not on the release-3.5 branch (or release-3.4 or earlier for that matter), I copied the script to /tmp, checked out the release branch, and then ran the /tmp script:

cp hack/cherry-pick.sh /tmp/cherry-pick.sh
git checkout release-3.5
. /tmp/cherry-pick.sh

@agilgur5
Copy link
Member Author

Due to the above, I backported this to release-3.5 as c956d70

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 10, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 6, 2024
…oj#12714)

(cherry picked from commit 7ba20fe)
Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 7, 2024
…oj#12714)

(cherry picked from commit 7ba20fe)
Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/contributing Contributing docs, ownership, etc issues & PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants