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: handle the case when required checks is disabled #3648

Conversation

peterzcc
Copy link

@peterzcc peterzcc commented Aug 5, 2023

what

Check if the optional field RequiredStatusChecks in the response is nil, and return a success early if "Require status checks to pass before merging" is disabled

why

tests

references

closes #2663

@peterzcc peterzcc requested a review from a team as a code owner August 5, 2023 07:29
@peterzcc peterzcc changed the title Fix the panic when required checks is disabled fix the panic when required checks is disabled Aug 5, 2023
@github-actions github-actions bot added go Pull requests that update Go code provider/github labels Aug 5, 2023
@peterzcc peterzcc changed the title fix the panic when required checks is disabled fix: handle the case when required checks is disabled Aug 5, 2023
@peterzcc
Copy link
Author

peterzcc commented Aug 7, 2023

@nitrocode could I have a review?

@jamengual
Copy link
Contributor

We will try to review soon

@peterzcc peterzcc force-pushed the peterzeng-check-required-statuschecks-nil branch from c88fcb3 to 22a4dd3 Compare August 8, 2023 05:37
@peterzcc
Copy link
Author

peterzcc commented Aug 8, 2023

pending the testing fix from #3653

@chenrui333
Copy link
Member

refresh the latest changes

@chenrui333
Copy link
Member

@peterzcc can you add tests for the change? Thanks!

@peterzcc peterzcc force-pushed the peterzeng-check-required-statuschecks-nil branch from c970e9e to a4f0a28 Compare August 14, 2023 05:57
@peterzcc
Copy link
Author

@chenrui333 The tests turns out to be non-trivial with the absence of ListCheckRunsCheckSuite mocking in existing tests. While this change is a trivial fix to handle the nil pointer case which would happen when Require status checks to pass before merging is disabled.

@jamengual jamengual added the waiting-on-review Waiting for a review from a maintainer label Sep 27, 2023
@GenPage
Copy link
Member

GenPage commented Oct 6, 2023

Looks like this was already merged in #3672 Got confused, but it was the merge commit update that caused it.

@GenPage GenPage closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code provider/github waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--gh-allow-mergeable-bypass-apply flag will panic if no required checks on GH branch protection settings
4 participants