-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add mergify configuration #42
base: master
Are you sure you want to change the base?
Conversation
I'd rather hold on to mergify until we have better coverage, or somehow os-autoinst is integrated into the testing (or testing gets better in general) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment
@foursixnine We'd still require 2 approvals and no pending reviews. |
1ceaa74
to
e496d7e
Compare
Having low coverage didn't stop us or you from merging regression introducing changes in the past ;) I still consider it an important task by human reviewers to approve only after thorough review which should include manual testing as long as further automatic testing does not cover more. This merely codifies rules into automatic executions and also helps in case of trivial things, e.g. everyone tested and approved but there are just simple typos to fix or tidy rules to apply, etc. |
tidy rules seem to have updated. I need to do #44 first |
- "check-success~=🐪 Perl" | ||
# we don't need to spell out all tests but we can check a minimum | ||
# amount | ||
- "#check-success=>6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it, so that it's the entire workflow?. Checking, only a few doesn't sound too reassuring to me. Still with two reviewers approving, should't be the end of the world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't need to include all as we also check that none failed. So we basically only need to cover any potential short "racy" time where some checks would have succeeded already but others did not yet even show up as pending. Likely this can not even happen as long as we only have github actions. This would e.g. only apply for something external like travisCI or circleCI which would report back with a pending status only after some seconds or even minutes
- name: automatic merge on special label | ||
conditions: | ||
- and: *base_checks | ||
- and: | ||
# mergify config checks needs at least two rules in "and" so we repeat | ||
# one from the base checks | ||
- base=master | ||
- "label=merge-fast" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed here? I mean for now fast merge makes sense, but in the future shuld be removed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the difference to the first rule is merely that we don't want to await approval from multiple persons. So for example for a urgent security fix or bugfix that you create I would set that label and as soon as all other checks have passed then mergify would merge regardless of the number of approvals. But I can give that label while tests are still running. mergify will still await successful tests before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the record, it's not that I'm completely against mergify... it's that id prefer that merges happen when we know all is good.
I agree. I just want to give a proper written definition of what "all is good" means to us :) |
No description provided.