Skip to content

ci/github-script/prepare: qualify recommendation to retarget from nixos-*#528319

Open
GraysonTinker wants to merge 1 commit into
NixOS:masterfrom
GraysonTinker:ci-improve-retarget-rec
Open

ci/github-script/prepare: qualify recommendation to retarget from nixos-*#528319
GraysonTinker wants to merge 1 commit into
NixOS:masterfrom
GraysonTinker:ci-improve-retarget-rec

Conversation

@GraysonTinker

@GraysonTinker GraysonTinker commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Currently the script unconditionally recommends that the PR be retargeted against stable if it was originally made against a stable nixos branch, which can be confusing for first-time contributors.

These PRs are always in error, and are usually made by first-time contributors who have not read CONTRIBUTING.md. Accordingly, we shouldn't expect them to know the differences between the stable and unstable branches. A recommendation of retargeting against stable is likely to cause confusion because it is presented as the absolutely correct choice.

Change the message to instead specify they should read the relevant documentation section.

Untested and done from my phone, so double-check everything's right.

Things done

@nixpkgs-ci nixpkgs-ci Bot requested a review from a team June 5, 2026 09:45
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions backport release-25.11 backport release-26.05 Backport PR automatically labels Jun 5, 2026
@MattSturgeon

MattSturgeon commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Currently the script unconditionally recommends that the PR be retargeted against stable

That's not the intended behaviour. The condition correctBranch = stable ? `release-${version}` : 'master' is supposed to handle that.

Are you saying that that is not working? If so, we should fix it, not remove it.

@GraysonTinker

GraysonTinker commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Currently the script unconditionally recommends that the PR be retargeted against stable

That's not the intended behaviour. The condition correctBranch = stable ? `release-${version}` : 'master' is supposed to handle that.

Are you saying that that is not working? If so, we should fix it, not remove it.

I phrased this rather confusingly, sorry for that. I've updated the original message.

@MattSturgeon MattSturgeon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Concept SGTM now I've re-read your updated explanation. Sorry for the confusion.

I've also not tested, but the diff LGTM.
LMK if you do test this on a fork, or using the local test harness. The changes seem fairly simple though.

Will wait a while for further input. If nothing happens after a week or so, ping me and I'll merge at a time I can monitor and react.

Thanks for the improvement!

Comment thread ci/github-script/prepare.js Outdated
@nixpkgs-ci nixpkgs-ci Bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 5, 2026
Comment thread ci/github-script/prepare.js Outdated
const { stable, version } = baseClassification
const correctBranch = stable ? `release-${version}` : 'master'
const { version } = baseClassification
const stable = `release-${version}`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If a PR targets nixos-unstable, the value of this variable is release-unstable, which of course doesn't exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It now says release-* or master when targeting nixos-unstable.

@GraysonTinker GraysonTinker force-pushed the ci-improve-retarget-rec branch from 9603378 to 3ab1563 Compare June 10, 2026 19:17
@GraysonTinker GraysonTinker force-pushed the ci-improve-retarget-rec branch from 3ab1563 to 4743865 Compare June 10, 2026 19:19
@GraysonTinker

Copy link
Copy Markdown
Contributor Author

'The `nixos-*` and `nixpkgs-*` branches are pushed to by the channel release script and should not be merged into directly.',
'',
`Please target \`${correctBranch}\` instead.`,
`Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then target \`${releaseStable}\` or \`master\` instead.`,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess these PRs could be supposed to go to a staging branch, probably would be best to allow for that?

Suggested change
`Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then target \`${releaseStable}\` or \`master\` instead.`,
`Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then target the correct branch (probably \`${releaseStable}\` or \`master\`) instead.`,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. backport release-26.05 Backport PR automatically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants