Skip to content

workflows/{merge-group,pr}: improve "no PR failures" handling#435929

Merged
wolfgangwalther merged 3 commits intoNixOS:masterfrom
wolfgangwalther:ci-pr-failures
Aug 24, 2025
Merged

workflows/{merge-group,pr}: improve "no PR failures" handling#435929
wolfgangwalther merged 3 commits intoNixOS:masterfrom
wolfgangwalther:ci-pr-failures

Conversation

@wolfgangwalther
Copy link
Contributor

The first two commits follow up on #435547 (comment) and #435547 (comment).

The last commit improves the "requiredness" of the owners check. The commit message:

The owners check is not reproducible, because it depends on the state of the NixOS org on GitHub. Owners can rename their accounts or they can leave the organisation and access to Nixpkgs can be removed from teams. All of this breaks the owners check for reasons unrelated to the PR at hand.

This PR makes the check for the owners file conditionally required: Only when the ci/OWNERS file is actually modified a failed check will block merging the PR. When that's not the case, the check will still fail visibily in the checklist, but the failure can be ignored.

This is especially relevant for the Merge Queue, which should not be entirely blocked whenever any of these events happen.

Also, it allows passing the checks in a fork when testing, where the owners check will always fail, because the respective teams and members are never part of the "user org" that a fork is.

Things done


Add a 👍 reaction to pull requests you find important.

@wolfgangwalther wolfgangwalther mentioned this pull request Aug 22, 2025
2 tasks
@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 6.topic: policy discussion Discuss policies to work in and around Nixpkgs backport release-25.05 labels Aug 22, 2025
@nixpkgs-ci nixpkgs-ci bot added 9.needs: reviewer This PR currently has no reviewers requested and needs attention. and removed 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Aug 22, 2025
@wolfgangwalther wolfgangwalther force-pushed the ci-pr-failures branch 2 times, most recently from 723f644 to 5ebc6f7 Compare August 24, 2025 10:56
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Diff LGTM

Comment on lines 79 to 81
Copy link
Contributor

Choose a reason for hiding this comment

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

To satisfy my curiosity, does it matter whether permissions are set in the calling or called workflow?

I.e. if a called workflow asked for a permission not present in the calling workflow/job, that will fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so GitHub will complain when requesting permissions in the called workflow, when they were not given in the calling workflow. That will just lead to a start up error, where the workflow won't even start running.

It looks like this: https://github.com/NixOS/nixpkgs/actions/runs/17187479040


Invalid workflow file: .github/workflows/test.yml#L74
The workflow is not valid. .github/workflows/test.yml (Line: 74, Col: 3): Error calling workflow 'NixOS/nixpkgs/.github/workflows/merge-group.yml@5a137567929f60e719b0764d353e13f60580f3d6'. The nested job 'unlock' is requesting 'statuses: write', but is only allowed 'statuses: none'.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 24, 2025
Posting the status manually allows us to avoid the strange "skipped ==
success" logic and properly skip the `unlock` job for pull_request
events in the next commit.

This should be much easier to understand than the previous logic.
…equest trigger

The required status checks should depend on exactly one workflow,
triggered via pull_request_target or merge_group. Anything that is
triggered by pull_request is for testing purposes of the workflows
themselves only.
The owners check is not reproducible, because it depends on the state of
the NixOS org on GitHub. Owners can rename their accounts or they can
leave the organisation and access to Nixpkgs can be removed from teams.
All of this breaks the owners check for reasons unrelated to the PR at
hand.

This PR makes the check for the owners file conditionally required: Only
when the ci/OWNERS file is actually modified a failed check will block
merging the PR. When that's not the case, the check will still fail
visibily in the checklist, but the failure can be ignored.

This is especially relevant for the Merge Queue, which should not be
entirely blocked whenever any of these events happen.

Also, it allows passing the checks in a fork when testing, where the
owners check will *always* fail, because the respective teams and
members are never part of the "user org" that a fork is.
@wolfgangwalther wolfgangwalther merged commit 8a36e80 into NixOS:master Aug 24, 2025
59 of 62 checks passed
@wolfgangwalther wolfgangwalther deleted the ci-pr-failures branch August 24, 2025 19:10
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Aug 24, 2025

Successfully created backport PR for release-25.05:

@github-actions github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Aug 24, 2025
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 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 8.has: port to stable This PR already has a backport to the stable release. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants