search

bombsquad-community / plugin-manager

A Plugin Manager for Bombsquad 1.7+
https://bombsquad-community.web.app/pluginmanager
Other
38 stars 28 forks source link

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

Open rikkolovescats opened 1 month ago

rikkolovescats commented 1 month ago

ci checks in PRs can sometimes be overlooked and this issue could allow for PRs with failing tests to be merged in (https://github.com/bombsquad-community/plugin-manager/pull/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.

Loup-Garou911XD commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/bombsquad-community/plugin-manager/commit/88d158dcb893d128436fd088de5d319920c19d62 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 commented 1 month ago

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 commented 1 month ago

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.