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

feat(git-node): add release promotion step #835

Merged
merged 50 commits into from
Nov 13, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jul 22, 2024

You can try it with git node release -S --promote https://github.com/nodejs/node/pull/53945.

Supersedes #402

/cc @nodejs/releasers

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 18, 2024

Oh! My bad, I didn't test that case, tbh I thought main would be the default already.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 18, 2024

So I tried reproducing by removing the setting for the branch name, and I immediately got an error:

$ git node release -S --promote https://github.com/nodejs/node/pull/54966
   ⚠  You have not told git-node what branch you are trying to land commits on.
--------------------------------------------------------------------------------
   ℹ  For example, if your want to land commits on the `main` branch, you can run:

  $ ncu-config set branch main
--------------------------------------------------------------------------------
Error: Failed to create new session
    at new Session (file:///…/node-core-utils/lib/session.js:27:15)
    at new ReleasePromotion (file:///…/node-core-utils/lib/promote_release.js:22:5)
    at main (file:///…/node-core-utils/components/git/release.js:127:21)

Did you modify that check locally maybe? You should not be able to proceed with an empty value.

@RafaelGSS
Copy link
Member

I did not. I have just gh pr checkout 835 and run the script.

Actually, I found why. I normally use a local ncu configuration and it was set to v22.x-staging indeed. Can we automatically change it to main or at least exit the script if it wasn't set properly?

@targos
Copy link
Member

targos commented Oct 3, 2024

So, I tried the command for v20.18.0. Unfortunately, I didn't go far. At some point it tried to update my local staging branch from upstream but it was already pushed in an earlier step (the manual one, since I ran in dry-run mode by mistake, but the dry-run had already created the tag so it wasn't completely dry).
I guess I should have answered "no" to the questions about v20.x-staging but I was afraid that it would cancel the entire run.

$ node ../node-core-utils/bin/git-node.js release --promote https://github.com/nodejs/node/pull/55170
✔  Received member information of nodejs/releasers
✔  targos is a Releaser
✔  Done loading data for nodejs/node/pull/55170
   ℹ  Last Full PR CI on 2024-10-02T14:11:11Z: https://ci.nodejs.org/job/node-test-pull-request/62893/
   ℹ  Last CITGM CI on 2024-10-02T14:11:11Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3484/
⠋ Querying data for job/node-test-pull-request/62893/   ✘  Last GitHub CI failed
   ℹ  This PR was created on Mon, 30 Sep 2024 08:59:29 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55170#pullrequestreview-2342018931
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/55170#pullrequestreview-2343657161
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/55170#pullrequestreview-2345406712
   ✔  - Ruy Adorno (@ruyadorno) (TSC): https://github.com/nodejs/node/pull/55170#pullrequestreview-2345570558
   ✔  Last Jenkins CI successful
✔  Jenkins CI is passing
✘  GitHub CI is failing for #55170
--------------------------------------------------------------------------------
? Do you want to proceed? Yes
✔  #55170 has necessary approvals
--------------------------------------------------------------------------------
? Tag and sign the release? Yes
⠼ Setting up for next release[v20.18.0-proposal 985262a4a82] Working on v20.18.1
 1 file changed, 2 insertions(+), 2 deletions(-)
✔  Successfully set up for next release
   ℹ  You are running in dry-run mode, meaning NCU will not run the `git push` commands, you would need to copy-paste the following command in another terminal window. Alternatively, pass `--run` flag to ask NCU to run the command for you (might not work if you need to type a passphrase to push to the remote).
   ℹ  Run the following commands to merge the staging branch:
   ℹ  git push upstream HEAD:refs/heads/v20.x HEAD:refs/heads/v20.x-staging
--------------------------------------------------------------------------------
? Ready to continue? Yes
--------------------------------------------------------------------------------
? Cherry-pick release commit to the default branch? Yes
Switched to branch 'v20.x-staging'
Your branch is behind 'upstream/v20.x-staging' by 2 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
? Do you want to try reset the local v20.x-staging branch to upstream/v20.x-staging? Yes
⠙ Bringing upstream/v20.x-staging up to date...From github.com:nodejs/node
 * branch                    v20.x-staging -> FETCH_HEAD
✔  upstream/v20.x-staging is now up-to-date
v20.x-staging is out of sync with upstream/v20.x-staging. Mismatched commits:
 - 7eebd17fa2c 2024-10-03, Version 20.18.0 'Iron' (LTS)
 - 985262a4a82 Working on v20.18.1
--------------------------------------------------------------------------------
? Reset to upstream/v20.x-staging? Yes
HEAD is now at 985262a4a82 Working on v20.18.1
   ✔  Reset to upstream/v20.x-staging
Auto-merging src/node_version.h
CONFLICT (content): Merge conflict in src/node_version.h
error: could not apply 7eebd17fa2c... 2024-10-03, Version 20.18.0 'Iron' (LTS)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config advice.mergeConflict false"
--------------------------------------------------------------------------------
   ℹ  Resolve the conflicts and commit the result
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
? Finished resolving cherry-pick conflicts? Yes
   ✘  Invalid Release commit message: Working on v20.18.1
Error: Aborted
    at ReleasePromotion.validateReleaseCommit (file:///Users/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:234:13)
    at ReleasePromotion.promote (file:///Users/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:168:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 3, 2024

FWIW you can delete the local tag as long as you haven't pushed it.

? Cherry-pick release commit to the default branch? Yes
Switched to branch 'v20.x-staging'

It looks like it thinks the default branch is v20.x-staging and not main, which is unlikely to produce good results. What does ncu-config get branch return?

@targos
Copy link
Member

targos commented Oct 3, 2024

It is v20.x-staging, which is normal from my pov. I work on a v20.x release in my v20.x workspace

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 4, 2024

It is v20.x-staging, which is normal from my pov

Does it need to be v20.x-staging for something else? Landing backport PR maybe? Asking because I'm wondering if we should rename it to defaultBranch to avoid the confusion – or if I choose just make an API call to get the name of the default branch (and assume there's a local branch with that name)

@targos
Copy link
Member

targos commented Oct 4, 2024

I believe it's for backport PRs, but I'm not sure.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 12, 2024

I've confirmed that to land a (backport or not) PR to a staging branch, NCU request you to set a different branch in your config. I've updated the promotion script to make an API call to get the name of the default branch instead of relying on the config.
I've also added a fallback when the version tag already exists to not crash if it points to the right commit and is correctly signed.

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 13, 2024

I've tried it with the v18.20.5 release, and did not get any blocker. I'm tempted to land it, we might still want to iterate on it but I think it's at a point where we can rely on it. @nodejs/releasers, wdyt?

@targos
Copy link
Member

targos commented Nov 13, 2024

LGTM

RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Nov 13, 2024
@aduh95 aduh95 merged commit dfa9c92 into nodejs:main Nov 13, 2024
10 of 11 checks passed
@aduh95 aduh95 deleted the git-node-release-promote branch November 13, 2024 14:07
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Nov 19, 2024
Refs: nodejs/node-core-utils#835
PR-URL: #55835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Refs: nodejs/node-core-utils#835
PR-URL: nodejs#55835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
Refs: nodejs/node-core-utils#835
PR-URL: nodejs#55835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit to nodejs/node that referenced this pull request Nov 26, 2024
Refs: nodejs/node-core-utils#835
PR-URL: #55835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit to nodejs/node that referenced this pull request Dec 10, 2024
Refs: nodejs/node-core-utils#835
PR-URL: #55835
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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.

6 participants