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

Configure skip-duplicates jobs to not skip duplicates (yes, really) #79764

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Feb 20, 2025

Summary

None

Purpose of change

Partially addresses #79241
Patches the exploit that allowed skipping CI checks but still getting a passing ✔️ checkmark

Describe the solution

We are using skip-duplicates job as a glorified path filter since regular path filter does not play nicely with required checks (see #49923).
Out of the box we also get the "skip duplicate" functionality i.e. "don't run the same check again if there is already a successful CI run of the same code". It's very nice, but as described in #79241 it runs into issues when we also conditionally disable checks on draft prs.

The CI exploit looks something like this:

  • create a draft PR
  • let the CI run. skip-duplicates would report that "there're no exiting duplicate runs, go ahead", then later we would decide "you know what, it's draft, let's not waste CPU cycles on this" and skip the checks.
  • CI run succeeds ✔️
  • Undraft the pr
  • Let the CI run again. skip-duplicate would look and find an already successful previous run on the exact same code(!), and say "you know, there's no need to run this thing again - we've done it already. Let's not waste CPU cycles here" and skip the checks.
  • CI run succeeds ✔️
  • checks were never run

These two redundancy checks are seemingly in conflict, and I couldn't find a better way to resolve it than removing one of the checks.
Even though both are somewhat rare, my hunch is that "duplicates" check triggers less often under normal conditions than "draft" check, so I disabled the "duplicates" one to save more CPU-time, but I don't have a strong opinion here. (Disabling the "draft" check makes the code cleaner. Tradeoffs...).

Describe alternatives you've considered

  • Leave duplicates checking be, but remove the draft pr special casing
  • remove the duplicates check from other jobs (astyle, json style) for consistency. They can't trigger the exploit since they have no "is in draft?" checks, and they run turbo-fast already, so i decided not to bother.

Testing

None other than ensuring the workflow is valid.

Additional context

@github-actions github-actions bot added Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Feb 20, 2025
@GuardianDll GuardianDll merged commit 04d0270 into CleverRaven:master Feb 21, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants