Skip to content

workflows/test: init#435547

Merged
wolfgangwalther merged 4 commits intoNixOS:masterfrom
wolfgangwalther:ci-pr-test
Aug 24, 2025
Merged

workflows/test: init#435547
wolfgangwalther merged 4 commits intoNixOS:masterfrom
wolfgangwalther:ci-pr-test

Conversation

@wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Aug 21, 2025 •
edited
Loading

(1st commit from #435526, 2nd and 3rd from #435535 - the latter is needed to avoid name conflicts of artifacts between the push and pr parts of this new workflow)

This workflow runs the PR and Push workflow files on a pull_request trigger. The intent is to test changes to the workflow files immediately. Previously, these were run directly from the respective workflow files.

The new approach allows us to move the logic to run this only when workflow files changed from the pull_request trigger into a job. This has the advantage that older jobs are cleaned up, when the PR changes from a state of "workflow files changed" to "no workflow files changed". This can happen when changing a PR's base from staging to master, in which case changes from master would temporarily appear in the PR as changes. When these include changes to workflow files, this would trigger the PR workflow via pull_request. Once the base is changed, the PR is closed and re-opened, so CI runs again - but since it's on the same commit and the new run doesn't trigger pull_request, the results of the previous run are still kept and displayed. These results may include cancelled or failed jobs, which are impossible to recover from without another force-push.

Checking this condition at run-time is only possible, because we move it into a separate workflow, turning the pr.yml workflow into a re-usable workflow. This will make sure to skip the whole workflow at once, when no change was detected, which will prevent the "no PR failures" job from appearing as skipped - which would imply "success" and make the PR mergeable immediately. Instead the "no PR failures" job is not shown at all for this trigger, which is generally what we want.

Do the same for push.yml for consistency. Also adds the merge_group.yml file to the test workflow to confirm we don't break that.

Note: This does not fully disable the "no PR failures" job for pull_request triggers, yet. It only affects the special case when switching branches in a PR. There will still be a second change required for the more general case, i.e. auto-merging a CI-related PR, which might trigger either of the two "no PR failures" jobs first and thus ignore the other.

Things done

Add a 👍 reaction to pull requests you find important.

@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 21, 2025
wolfgangwalther
wolfgangwalther commented Aug 21, 2025
View reviewed changes
@MattSturgeon
Copy link
Contributor

MattSturgeon commented Aug 21, 2025

Note: This does not fully disable the "no PR failures" job for pull_request triggers, yet. It only affects the special case when switching branches in a PR. There will still be a second change required for the more general case, i.e. auto-merging a CI-related PR, which might trigger either of the two "no PR failures" jobs first and thus ignore the other.

Just thought. Another way we could handle that would be to make the job name dynamic.

E.g. it is named no PR failures for pull_request_target and merge_group events, but we name it something else like no PR failures (test) for pull_request events.

MattSturgeon
MattSturgeon reviewed Aug 21, 2025
View reviewed changes
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.

A fundamental issue here may be the security limitations applied to the pull_request trigger when the PR is from a fork.

This would imply that to run the test.yml workflow, you must push your PR branch to the non-fork nixpkgs repo rather than to your fork.

This would be especially inconvenient for non-committers.

Is it possible to configure these workflows so that they run as much as possible when the secrets are not available?

One complication is that you also cannot use secrets in if conditions, although you can have a step check whether a secret is non-empty and write to GITHUB_OUTPUT. I guess you could do something similar with permissions, maybe using the permissions REST API endpoint?

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Aug 21, 2025

Just thought. Another way we could handle that would be to make the job name dynamic.

E.g. it is named no PR failures for pull_request_target and merge_group events, but we name it something else like no PR failures (test) for pull_request events.

That's exactly my thought on how to handle this additionally. I find no PR failures (test) too dangerous because of "partial matches" - do we know how GitHub behaves there exactly? Thus my idea was to go with no Test failures and no PR failures - aka interpolating the wrapper workflow's name in there.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Aug 21, 2025

A fundamental issue here may be the security limitations applied to the pull_request trigger when the PR is from a fork.

This would imply that to run the test.yml workflow, you must push your PR branch to the non-fork nixpkgs repo rather than to your fork.

This would be especially inconvenient for non-committers.

Is it possible to configure these workflows so that they run as much as possible when the secrets are not available?

Note that this is already the case - this PR doesn't change anything in that regard. We already have the case that:

  • PRs in pull_request events try to do as much as they can.
  • PRs in pull_request have different privileges between "not from fork" and "from fork" scenarios.

There's not much we can do different here without tripping up actionlint. That's why the secrets and permissions are all passed down from the test.yml workflow, even if they are most often dysfunctional.

To clarify: The test.yml workflow should already work both from forks and from within the same repo.

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 21, 2025
@wolfgangwalther

This comment was marked as outdated.

Sign in to view
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 22, 2025
@wolfgangwalther

This comment was marked as outdated.

Sign in to view
@wolfgangwalther

This comment was marked as outdated.

Sign in to view
wolfgangwalther
wolfgangwalther commented Aug 22, 2025
View reviewed changes
.github/workflows/test.yml Outdated
Comment on lines 35 to 36
Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Aug 22, 2025

Choose a reason for hiding this comment

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

I added this here even though technically the prepare script does not post review comments, yet. But since this wouldn't result in a merge conflict with #435596, I am very likely to forget about it. Thus, I'll rather set it to false right now. It will become effective once #435596 is merged.

@MattSturgeon

This comment was marked as outdated.

Sign in to view
@wolfgangwalther
workflows/push: remove unused permissions and secrets
9d4fb0c 
These were left-over from when the eval workflow still had the labeling
and reviewer components.
@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Aug 22, 2025

(resolved another one of my own merge conflicts...)

@wolfgangwalther wolfgangwalther mentioned this pull request Aug 22, 2025
Merged
2 tasks
@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Aug 22, 2025

Can't see my mistake. Do job names not support dynamic expressions? We use them for matrix jobs all the time... maybe this only works for these, so I need to turn this into a matrix job? Ugh...

Bizarre. I wonder if this is an oversight in some GHA internal optimisation? It's strange that it removes the ${{ }} syntax but then just treats the expression as a literal string!

If it is fixed by the job being a matrix, I wonder if strategy: matrix is enough, with a singleton list matrix whose values are not used?

Came up with a different, imho much better, because less hacky, approach in #435929.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Aug 23, 2025

Just added the test.yml workflow file to the conditions in test.yml it self - if the file changes, it needs to be tested, too.

Other than the stuff mentioned above, I'm hoping this will also help with unnecessarily scheduled jobs in haskell-updates. Currently, the pull_request trigger fires in #429810 all the time, even though the "Files" tab doesn't show any changes to the workflows. Seems like the paths: trigger for pull_request works on a different idea of "diff" between base and head. Probably similar to the "is there a merge conflict or not?" thing. With the new approach herein, I'm hoping these jobs won't be scheduled anymore.

MattSturgeon
MattSturgeon approved these changes Aug 23, 2025
View reviewed changes
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.

LGTM

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 23, 2025
@wolfgangwalther
workflows/test: init
443f30f 
This workflow runs the PR and Push workflow files on a `pull_request`
trigger. The intent is to test changes to the workflow files
immediately. Previously, these were run directly from the respective
workflow files.

The new approach allows us to move the logic to run this only when
workflow files changed from the pull_request trigger into a job. This
has the advantage that older jobs are cleaned up, when the PR changes
from a state of "workflow files changed" to "no workflow files changed".
This can happen when changing a PR's base from staging to master, in
which case changes from master would temporarily appear in the PR as
changes. When these include changes to workflow files, this would
trigger the PR workflow via `pull_request`. Once the base is changed,
the PR is closed and re-opened, so CI runs again - but since it's on the
same commit and the new run doesn't trigger `pull_request`, the results
of the previous run are still kept and displayed. These results may
include cancelled or failed jobs, which are impossible to recover from
without another force-push.

Checking this condition at run-time is only possible, because we move it
into a separate workflow, turning the `pr.yml` workflow into a re-usable
workflow. This will make sure to skip the whole workflow at once, when
no change was detected, which will prevent the "no PR failures" job from
appearing as skipped - which would imply "success" and make the PR
mergeable immediately. Instead the "no PR failures" job is not shown at
all for this trigger, which is generally what we want.

Do the same for `push.yml` for consistency.
@wolfgangwalther
workflows/test: run push job on correct commit
06a0eba 
Previously, the test for the push workflow was running on the HEAD
commit of the PR only. It would be better to run it on the merged result
instead, just like any other tests we run in a PR.
@wolfgangwalther
workflows/test: test merge-group workflow
534d41e 
Changes to the merge-group workflow should also validate that the file
is hooked up correctly and works - otherwise we risk merging CI changes
that cause the merge queue to fail consistently.
@wolfgangwalther wolfgangwalther merged commit 0b91796 into NixOS:master Aug 24, 2025
53 of 54 checks passed
@wolfgangwalther wolfgangwalther deleted the ci-pr-test branch August 24, 2025 10:14
@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
@nixpkgs-ci nixpkgs-ci bot mentioned this pull request Aug 24, 2025
Merged
1 task
@wolfgangwalther

This comment was marked as outdated.

Sign in to view
@wolfgangwalther

This comment was marked as outdated.

Sign in to view
@wolfgangwalther wolfgangwalther mentioned this pull request Aug 24, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MattSturgeon MattSturgeon MattSturgeon approved these changes

@philiptaron philiptaron Awaiting requested review from philiptaron

@Mic92 Mic92 Awaiting requested review from Mic92

@zowoq zowoq Awaiting requested review from zowoq

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wolfgangwalther @MattSturgeon