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

Fix strict branch protection #954

Merged
merged 3 commits into from
Nov 8, 2021

Conversation

kfcampbell
Copy link
Member

@kfcampbell kfcampbell commented Oct 21, 2021

Strict protection on v4 branch protection doesn't work at the moment. This PR makes minor code changes and removes the diff suppression func in order to correct strict branch protection.

The research to get to this point is chronicled in #880.

The TL;DR is that by removing the diff suppression and making a minor code change to the branch protection processing, we can restore strict branch protection with the graphQL client.

@kfcampbell kfcampbell marked this pull request as ready for review October 22, 2021 06:01
@kfcampbell kfcampbell changed the title Experiment for fix on strict branch protection Fix strict branch protection Oct 22, 2021
@guillaumelecerf
Copy link

Confirmed to work as expected, thanks @kfcampbell !

@jcudit jcudit added this to the v4.18.0 milestone Oct 26, 2021
@kfcampbell
Copy link
Member Author

Alright I've spent more time here and done an expanded testing matrix on both the current release and this feature branch. The results are as follows:

main branch

  • branch protection, without required status checks
    • same weird state with the hidden checked box
    • churn: initial push_restrictions = [] then nothing
  • branch protection, with required status checks, strict = false
    • initial apply is broken, no required status checks, same weird state with the hidden checked box
    • churn: initial push_restrictions = [] then nothing, still broken
  • branch protection, with required status checks, strict = false, with contexts
    • initial apply works
    • churn: initial push_restrictions = [] then nothing
    • if contexts are present but empty, same broken behavior as above is exhibited
  • branch protection, with required status checks, strict = true
    • initial apply is broken, no required status checks, same weird state with the hidden checked box
    • churn: initial push_restrictions = [] then nothing, still broken
  • branch protection, with required status checks, strict = true, with contexts
    • initial apply works
    • churn: initial push_restrictions = [] then nothing
    • if contexts are present but empty, same broken behavior as above is exhibited

bugfix branch

  • branch protection, without required status checks
    • same weird state with the hidden checked box
    • churn: initial push_restrictions = [] then nothing
  • branch protection, with required status checks, strict = false
    • initial apply works
    • churn: initial push_restrictions = [] and contexts = [] then nothing
  • branch protection, with required status checks, strict = false, with contexts
    • initial apply works
    • churn: initial push_restrictions = [] then nothing
    • if contexts are present but empty, there's churn: initial push_restrictions = [] and contexts = [] then nothing
  • branch protection, with required status checks, strict = true
    • initial apply works
    • churn: initial push_restrictions = [] and contexts = [] then nothing
  • branch protection, with required status checks, strict = true, with contexts
    • initial apply works
    • churn: initial push_restrictions = [] then nothing

In addition, @jcudit and I have done some internal investigation and it looks like the weird behavior with the hidden checkbox already checked is by design, or at least it's been that way from the beginning.

TL;DR this PR is ready for review! The bug is fixed on this branch and there's no regression on #572.

@jcudit jcudit merged commit b79eb64 into integrations:main Nov 8, 2021
kfcampbell added a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Initial commit of strict protection fix

* Add back commented diff suppresion func to keep linter happy

* Remove diff suppression function entirely
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 this pull request may close these issues.

3 participants