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

workflows/check-by-name: Trigger on base branch changes #282707

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jan 21, 2024

Context

In a PR there was an odd failure of the pkgs/by-name check: After a base branch change, it seemingly complained about new non-pkgs/by-name packages introduced by the PR (see #275539 that introduced this new check). Even though the PR only had a single unrelated commit on top of staging at that point.

Why did this happen? Here's the series of events leading to that:

  • PR opened with master as the base branch
    This triggered CI, completing successfully

  • Marked as draft, presumably to avoid the many review requests when changing the base branch in the next step

  • Force pushed, presumably to staging + the changes
    At this point, master is still the base branch, and if the PR were merged at that point, master would've gotten all commits from staging.
    Since the pkgs/by-name checker compares the PR against the base branch of it, it correctly failed with the packages that would've been new on master and aren't using pkgs/by-name yet!

    Also this can only cause failures right now because the two new relevant packages in staging were merged before pkgs/by-name started being enforced (Vulkan 1.3.275 #281591 and libplacebo: 5.264.1 -> 6.338.1 #269415). Now that pkgs/by-name is being enforced (notably for all base branches), it's going to get very improbable.

  • Only afterwards, the base branch was changed to staging.
    However, the pull_request_target GitHub Actions event doesn't trigger for base branch changes by default!

One way to "fix" that is to instead push the merge base of the old and new base branch before switching. This way you also won't have to mark it as a draft to avoid requesting reviews by everybody.

But the better fix is to make CI retrigger when the base branch changes!

This work is sponsored by Tweag and Antithesis

Changes

This PR changes the pkgs/by-name check workflow to also trigger when the base branch changes. This type of event doesn't trigger CI by default:

By default, a workflow only runs when a pull_request_target event's activity type is opened, synchronize, or reopened.

Instead it's part of the edited event, which however also triggers in other cases:

The title or body of a pull request was edited, or the base branch of a pull request was changed.

However, this workflow is fairly quick, and PR's don't get edited that often, so it shouldn't be a problem.

Ping @zeuner

Things done


Add a 👍 reaction to pull requests you find important.

Not doing this can cause CI to report a misleading result when it wasn't
retriggered after a base branch change.
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/ci-will-soon-enforce-pkgs-by-name-for-new-packages/38098/4

@infinisil infinisil mentioned this pull request Jan 21, 2024
13 tasks
@infinisil infinisil added this to the RFC 140 milestone Jan 21, 2024
@infinisil infinisil marked this pull request as ready for review January 22, 2024 22:41
@infinisil infinisil merged commit 2ced5ae into NixOS:master Jan 24, 2024
22 checks passed
@infinisil infinisil deleted the by-name-base-trigger branch January 24, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants