os-autoinst-distri-opensuse
os-autoinst-distri-opensuse copied to clipboard
os-autoinst
Add mergify for additional checks and automatic merging based on special label
After this PR is merged pull requests can be automatically merged with a special label and as soon as all rules specified are met. For example: Only if all checks pass, a verification run was provided, no review pending or asking for changes, at least two approvals given, no label to wait, etc. Additional checks are conducted and show up as check in github status details same as other checks. Pull requests can still be merged manually as is but this approach helps for a couple of reasons:
- Acceptance criteria for merge are codified into rules that everybody can read
- Nobody needs to wait anymore if everything was already provided and approved, verification runs have been checked but you just wait for the CI to finish
- No trivial things are overlooked in review which can be automatically checked
- The rules can be extended in the future to cover further checks. For example I plan to automatically check provided verification runs or use the feature to semi-automatically trigger verification runs and wait for their successful completion
You can try out how mergify behaves within the test pull request https://github.com/okurz/os-autoinst-distri-opensuse/pull/4
Related progress issues:
- https://progress.opensuse.org/issues/72841
- https://progress.opensuse.org/issues/49502
- https://progress.opensuse.org/issues/48641
@os-autoinst/tests-maintainer as this should simplify all your work I am asking you for review.
Sorry, but I don't think this is a good approach of merging things automatically, even with 2 approvals. And I'm afraid it may bring more troubles then benefits to our codebase. I've seen a lot of times when a PR received a couple of "Approvals" but then some new reviewer came and raised a valid point to change the PR and a lot of people change their mind.
Also, nothing prevents from altering a PR after receiving "Approve". How to deal with such cases?
One more thing is that it makes "Approve" button to be taken more serious then before. So, enabling auto-merge also introduces artificial limitations in review process, and I guess a lot of people will just avoid them by typing LGTM without pressing "Approve" button:)
-1 I think that it could potentially break the work flow of some teams, in the sense that even users with no privileges to merge (which cannot even set labels) can approve it, right? ok I just verified that this only can happen when that person is asked to review by somone with privileges. But at the end it is easy that some PR, where WIP or label is missed can be approved by someone less experience and even approved by someone external to the team interested in the change but not really knowledgeable of the code, not giving the opportunity to more senior colleagues to take a look. It could be also a waste of time to have to sync with other colleagues about github features, for example requesting things like 'set the label here for me' or 'do this other github feature here' or clarify why is a 'request for change' and not a suggestion, instead of syncing about the works itself.
I'm really not a fan of enabling mergify for the tests, while it would be amazing and a lot of time saving, unless one can have a list of submitters whose's PR's can be automerged.
Reasoning for this: We run this on openSUSE and SLE as is, so if somebody misses checking a zipped file (of which we have many), or some random binary... we can end up in a bad place, as @Vogtinator mentions:
People can change things between approvals and add malicious code...
IMO this is a consideration of the team's process. Do not approve early on without making sure that review and CI are finished. i doubt that many are checking the changes done after the approve. So even now a malicious code or whatever can pass through in theory.
FUD aside, the other side of the problem is while we only would need one or two persons to approve, someone else's code might break. You break it, you fix it, is cool though.
So, from my pov:
- Until we have an automated check that the pr is not adding compressed files
- Until we have an automated check that there are no external things being loaded
It's a no from me.
-1 I think that it could potentially break the work flow of some teams, in the sense that even users with no privileges to merge (which cannot even set labels) can approve it, right?
No, this does not count as real "approval".
ok I just verified that this only can happen when that person is asked to review by somone with privileges.
correct
But at the end it is easy that some PR, where WIP or label is missed can be approved by someone less experience and even approved by someone external to the team interested in the change but not really knowledgeable of the code, not giving the opportunity to more senior colleagues to take a look.
Well, mistakes can happen if this is even a realistic risk. However I am sure people would learn quickly that approval is something to take serious. Everyone can write "+1" if they are unsure.
It could be also a waste of time to have to sync with other colleagues about github features, for example requesting things like 'set the label here for me' or 'do this other github feature here' or clarify why is a 'request for change' and not a suggestion, instead of syncing about the works itself.
I am not sure what you mean here. There is no need to discuss about github features if you don't want to use them. All the old workflows can still work. Only if really all rules are met then mergify would merge automatically. The author can prevent that with a mere "WIP" in the subject, in the git commit message, asking for review, etc., any of these will work
Sorry, but I don't think this is a good approach of merging things automatically, even with 2 approvals. And I'm afraid it may bring more troubles then benefits to our codebase. I've seen a lot of times when a PR received a couple of "Approvals" but then some new reviewer came and raised a valid point to change the PR and a lot of people change their mind.
What considered a valid point? that it is wrong or that someone changed his mind of how it should look like? it is always possible to miss/break something even with the best reviews and VR and everything. But the review shouldnt block the PR in such a dictate way.
Maybe you have a few approvals because it is free now, without consequence. I expect that the review can become more effective with this approach.
Also, nothing prevents from altering a PR after receiving "Approve". How to deal with such cases?
One more thing is that it makes "Approve" button to be taken more serious then before. So, enabling auto-merge also introduces artificial limitations in review process, and I guess a lot of people will just avoid them by typing LGTM without pressing "Approve" button:)
Sorry, but I don't think this is a good approach of merging things automatically, even with 2 approvals. And I'm afraid it may bring more troubles then benefits to our codebase. I've seen a lot of times when a PR received a couple of "Approvals" but then some new reviewer came and raised a valid point to change the PR and a lot of people change their mind.
True, that can happen. If someone feels like "I don't see anything wrong but I can not take the responsibility to approve" then just +1. We can also extend the configuration to ensure that a PR stays open for a minimum time or we only merge automatically if there are even more approvals than just 2 or we only keep the part where mergify would act on a specific label
Also, nothing prevents from altering a PR after receiving "Approve". How to deal with such cases?
same as before. Reviewers either need to trust authors or only approve if everything is really fine. And if anyone wants to abuse the system I am sure we will teach them fast. I trust my colleagues … or at least I trust that I can find them when they mess up ;)
One more thing is that it makes "Approve" button to be taken more serious then before.
And that's a good point, right? :)
I'm really not a fan of enabling mergify for the tests, while it would be amazing and a lot of time saving, unless one can have a list of submitters whose's PR's can be automerged.
Reasoning for this: We run this on openSUSE and SLE as is, so if somebody misses checking a zipped file (of which we have many), or some random binary... we can end up in a bad place, as @Vogtinator mentions:
People can change things between approvals and add malicious code...
FUD aside, the other side of the problem is while we only would need one or two persons to approve, someone else's code might break. You break it, you fix it, is cool though.
So, from my pov:
* Until we have an automated check that the pr is not adding compressed files * Until we have an automated check that there are no external things being loaded
That's a good point in general but you are applying too strict rules for a little helper for what reviewers so far also not always looked into.
How about only including the rule "automatic merge on special label" for a start?
I'm really not a fan of enabling mergify for the tests, while it would be amazing and a lot of time saving, unless one can have a list of submitters whose's PR's can be automerged. Reasoning for this: We run this on openSUSE and SLE as is, so if somebody misses checking a zipped file (of which we have many), or some random binary... we can end up in a bad place, as @Vogtinator mentions: People can change things between approvals and add malicious code... FUD aside, the other side of the problem is while we only would need one or two persons to approve, someone else's code might break. You break it, you fix it, is cool though. So, from my pov:
* Until we have an automated check that the pr is not adding compressed files * Until we have an automated check that there are no external things being loadedThat's a good point in general but you are applying too strict rules for a little helper for what reviewers so far also not always looked into.
Let's have those two extra checks first, and then talk about this again, I'm not opening up to a free and big fat security hole, just because people believe in CI/CD a little bit too much.
A quote from one of my favorite movies: 
How about only including the rule "automatic merge on special label" for a start?
Same standing.
I'm really not a fan of enabling mergify for the tests, while it would be amazing and a lot of time saving, unless one can have a list of submitters whose's PR's can be automerged.
By the way, we can have that list of submitters, e.g. with
- author=@tests-maintainer
should I include that?
Reasoning for this: We run this on openSUSE and SLE as is, so if somebody misses checking a zipped file (of which we have many), or some random binary... we can end up in a bad place
Within o3 we run with apparmor at least.
* Until we have an automated check that the pr is not adding compressed files
ok, that should be possible. So how about a check in github action that conducts this and if it fails then mergify would not merge automatically. Reviewers would be alerted because they see the red X and take extra care about the added file.
The check could be
git diff --name-only origin/master.. | xargs file --mime | grep 'application/.*\(zip\|compressed\)'
this detects not only plain "zip" files but gzip, bzip2, 7z, rar, etc.
* Until we have an automated check that there are no external things being loaded
Do you mean no external things loaded during CI test execution or within openQA test code? The first can be covered with executing tests with unshare -r -n which takes away network access and any reliance on external network would quickly be discovered. Or do you mean within the openQA test code? If that, how do you qualify "no external things being loaded" exactly?
Updated the PR:
- Split the changes into individual commits so if you like you can review the commits one by one and comment on each individually to better separate the discussions
- Now approvals will be automatically removed as soon as changes have been introduced so that approvals will not stick and hence no one can sneak in malicious code that way
- Changed the "automatic merge" based on all successful checks to instead just provide another status check instead of an automatic merge
What I still don't get is what benefit mergify actually brings.
FWICT, instead of hitting the merge/auto-merge button it's enough to submit an approving review, and once the criteria are fulfilled, mergify will do the "work" of actually performing the merge. Why not just hit the merge button directly? Is it to make it harder to accidentially merge a PR which wasn't meant to be merged by accident? If so, this could be done by extending the already existing PR checks mechanism and not fully by mergify.
The downsides of mergify are that there's more complexity involved, both in configuration and usage, and a third-party is trusted with full access to the repo.
@okurz mind if I close this PR?.
Depends if you think if we should have this change included in general or not. I can change stuff if you have good suggestions
What if we disable the automerge feature and only let mergify to check and moderate?
Depends if you think if we should have this change included in general or not. I can change stuff if you have good suggestions
In general I think we don't need this.
What if we disable the automerge feature and only let mergify to check and moderate?
Thatś what the rest of the checks also do already :)
Depends if you think if we should have this change included in general or not. I can change stuff if you have good suggestions
In general I think we don't need this.
Why not? It already helps us in multiple projects.
What if we disable the automerge feature and only let mergify to check and moderate?
Thatś what the rest of the checks also do already :)
Not quite. I could repeat the original description but I'd rather suggest you read it again and please reference that in your answer.
Depends if you think if we should have this change included in general or not. I can change stuff if you have good suggestions
In general I think we don't need this.
Why not? It already helps us in multiple projects.
I'm answering to your question above. On the why not, obviously nobody has cared about it or seen the need to have it for amost a year (even with the use of a tag)
If you have more questions, please refer to my previous answer
What if we disable the automerge feature and only let mergify to check and moderate?
Thatś what the rest of the checks also do already :)
Not quite. I could repeat the original description but I'd rather suggest you read it again and please reference that in your answer.
You can read here: https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#issuecomment-992518111.
If you have more questions, please refer to my previous answer
(Yes, I linked to the same place 3 times on purpose).
In general I prefer to close this PR, instead of keeping the same Münchhausen trilemma or whataboutism
I'm answering to your question above. On the why not, obviously nobody has cared about it or seen the need to have it for amost a year (even with the use of a tag)
I don't understand how you can apply that as reasoning. That would imply that basically any idea noted in a ticket or also any future pull request would be useless because "nobody did it so far hence it might is not useful enough"?!?
If you have more questions, please refer to my previous answer
you might have missed my suggestions and further questions in https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#issuecomment-992537335 trying to answer your ponits and my update in https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#issuecomment-992664089
I'm answering to your question above. On the why not, obviously nobody has cared about it or seen the need to have it for amost a year (even with the use of a tag)
I don't understand how you can apply that as reasoning. That would imply that basically any idea noted in a ticket or also any future pull request would be useless because "nobody did it so far hence it might is not useful enough"?!?
I suggest you to open a ticket then.
If you have more questions, please refer to my previous answer
you might have missed my suggestions and further questions in #13455 (comment) trying to answer your ponits and my update in #13455 (comment)
Didn't miss them, read them back then and also before replying today... My standing is still the same.
@foursixnine I don't understand what you mean or what else you would want me to do. I guess unless other people chime in to help resolve our deadlock, maybe by voting, after sufficient waiting time I will close this PR unmerged.
@os-autoinst/tools-team what's your take on this?
@foursixnine I don't understand what you mean or what else you would want me to do. I guess unless other people chime in to help resolve our deadlock, maybe by voting, after sufficient waiting time I will close this PR unmerged.
@os-autoinst/tools-team what's your take on this?
- None of the 3 related tickets seems really related. The tickets are about adding automated verification runs and staging tests. Although I could see how you consider it best practice.
- The points from https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#issuecomment-992537335 seem misplaced here since this PR is not implementing execution of openQA tests and afair we only validate the schedule so far. I would second those concerns otherwise.
- The title seems wrong (now). I don't see automatic merging in the config YAML
@foursixnine I don't understand what you mean or what else you would want me to do.
See https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#pullrequestreview-829957246
I guess unless other people chime in to help resolve our deadlock
Sure
PS: I removed the notready label, so stale will mark it as stale and close it automatically in 90 days if it isn't merged/closed
@kalikiana
* The points from [Add mergify for additional checks and automatic merging based on special label #13455 (comment)](https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13455#issuecomment-992537335) seem misplaced here since this PR is not implementing execution of openQA tests and afair we only validate the schedule so far. I would second those concerns otherwise.
This repo has unit tests and multiple static code checks so it's not just validation of the schedule file validity.
* The title seems wrong (now). I don't see automatic merging in the config YAML
That's correct. I removed that part for now.
@foursixnine I don't understand what you mean or what else you would want me to do.
See #13455 (review)
As stated in before:
- In the current config there is no automatic merge anymore
- I already answered your two suggestions about further automatic checks and wanted to have feedback on those before continuing with any implementation
This repo has unit tests and multiple static code checks so it's not just validation of the schedule file validity.
Yes. And I'm saying I don't see the concerns with what we have right now.