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

Pull requests with failing checks are sometimes able to bypass merge rules #283

Open
rikkolovescats opened this issue May 21, 2024 · 5 comments
Labels
bug Something isn't working ci Everything related to CI

Comments

@rikkolovescats
Copy link
Collaborator

ci checks in PRs can sometimes be overlooked and this issue could allow for PRs with failing tests to be merged in (#282). This isn't ideal, since at the moment PRs are supposed to pass all ci checks before they can be merged in.

Gotta debug why this can happen at times, and if there's a way to prevent it from happening.

@rikkolovescats rikkolovescats added bug Something isn't working ci Everything related to CI labels May 21, 2024
@Loup-Garou911XD
Copy link
Collaborator

ci checks in PRs can sometimes be overlooked and this issue could allow for PRs with failing tests to be merged in (#282). This isn't ideal, since at the moment PRs are supposed to pass all ci checks before they can be merged in.

Gotta debug why this can happen at times, and if there's a way to prevent it from happening.

That pr was passing ci

@vishal332008
Copy link
Collaborator

ci checks in PRs can sometimes be overlooked and this issue could allow for PRs with failing tests to be merged in (#282). This isn't ideal, since at the moment PRs are supposed to pass all ci checks before they can be merged in.
Gotta debug why this can happen at times, and if there's a way to prevent it from happening.

That pr was passing ci

I think from next time, I should edit the plugman and index.json, create PR, and then edit changelog, so that the CI will show passing clearly for the last commit. Then Rikko will not get confused.

@rikkolovescats
Copy link
Collaborator Author

ci checks in PRs can sometimes be overlooked and this issue could allow for PRs with failing tests to be merged in (#282). This isn't ideal, since at the moment PRs are supposed to pass all ci checks before they can be merged in.
Gotta debug why this can happen at times, and if there's a way to prevent it from happening.

That pr was passing ci

That can't be right.. I remember having to push this commit 88d158d directly to main branch to fix an issue related to updating plugin manager through in-game (as well as to fix the coressponding failing unit test) after the merge of #282.

@vishal332008
Copy link
Collaborator

vishal332008 commented Jun 1, 2024

Umm... the PR was able to merge because PR's can be accepted with just this condition Merging can be performed automatically with 1 approving review.. So i think you can change some stuff in settings to add passing CI as a condition too.

@rikkolovescats
Copy link
Collaborator Author

It already requires both an approval as well as the ci to pass to make PR merge button go green. I'm guessing if the ci doesn't run on the last commit (which for some reason seems to have happened in #282), then it only requires the approval check for the PR merge button to go green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci Everything related to CI
Projects
None yet
Development

No branches or pull requests

3 participants