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

branch protection rules impede PRs that only modify conformance from being merged #2262

Closed
japaric opened this issue Jun 27, 2024 · 2 comments · Fixed by #2269
Closed

branch protection rules impede PRs that only modify conformance from being merged #2262

japaric opened this issue Jun 27, 2024 · 2 comments · Fixed by #2269

Comments

@japaric
Copy link
Collaborator

japaric commented Jun 27, 2024

as it has been observed in PRs like #2258 the current branch protection rules require that tasks like 'cleanliness' and 'compatibility' run and that prevents PRs like that from being merged.

when we introduced the conformance test suite, we tweaked CI so that only the 'conformance' job runs when code outside \conformance is not modified. in this scenario, 'cleanliness' and others do not run so the branch protection rules prevent the PR from being merged.

I investigated an alternative to this setup that preserves the behavior of running only the conformance test suite when hickory source code is not modified in this repository: https://github.com/japaric/github-branch-protection-rules-experiment/

Starting from something that resembles the hickory-dns setup, I replaced ignore-paths with an equivalent job that performs a git diff check. this job produces an output that other jobs check to decide if tests are to be executed or not.

under this new setup, all jobs run but the actual tests steps are skip based on the outcome of the git diff job. this preserves the CI time saving properties of the path-ignores solution without clashing with the branch protection rules.

these two PRs show that is the case:

I have set up branch protection rules to require both jobs, 'unit tests' and 'conformance tests', to run and in both of the above cases, they don't block the PRs from being merged. both required jobs have green checks next to them

I didn't find a way to have a step that early returns from a job so if this alternative is implemented we'll have to add an if condition to most steps of all jobs minus the 'conformance' job

japaric/github-branch-protection-rules-experiment#6 should give you an idea of the magnitude of the required changes. namely,

  • the 'conformance.yml' workflow gets merged into the "main" workflow
  • a git diff job needs to be added to the workflow
  • several steps (in the PR only step is modified) gets an if conditional
  • (none of the existing branch protection rules needs to be modified)

note that I didn't get everything working correctly in that PR so you should check https://github.com/japaric/github-branch-protection-rules-experiment/blob/98386e017a5e19873f3244734351a01fd82c113e/.github/workflows/main.yml for the actual final version

cc @djc

@djc
Copy link
Collaborator

djc commented Jun 27, 2024

Hmm... would this be better if we implemented the conformance tests in a different job? Or maybe can invert the "burden of change proof" so that only the conformance tests job tests for changes instead of all the jobs?

@japaric
Copy link
Collaborator Author

japaric commented Jul 1, 2024

another simple fix is to run all the tests all the time. this is less CI time efficient but given that CI is not the bottleneck in getting PRs merged (+), we can probably live with that.

(+) unlike say rust-lang/rust where PRs are approved faster than it takes to run the full CI pipeline (1-2 hours?). there it makes sense to "roll up" PRs or the queue of approved PRs would never shrink

japaric added a commit that referenced this issue Jul 1, 2024
an issue in GitHub branch protecion rules prevents us from merging PRs
that only change the /conformance directory with the current CI-time
saving setup.

this commit works around the issue by *always* running all the tests

closes #2262
japaric added a commit that referenced this issue Jul 1, 2024
an issue in GitHub branch protecion rules prevents us from merging PRs
that only change the /conformance directory with the current CI-time
saving setup.

this commit works around the issue by *always* running all the tests

closes #2262
@djc djc closed this as completed in 1fd2cd9 Jul 2, 2024
@djc djc closed this as completed in #2269 Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants