-
Notifications
You must be signed in to change notification settings - Fork 208
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
ci: combine integration tests #8253
Conversation
56c9034
to
ccbd066
Compare
43190e4
to
a139345
Compare
a188441
to
b3f0271
Compare
Ok I think this is actually ready for review now. @warner I don't automatically expect a review, just more an FYI |
b3f0271
to
7de02d9
Compare
7de02d9
to
9dc7bb5
Compare
9dc7bb5
to
0d6ccef
Compare
Sorry for the churn. I had to remove the run to completion test (it's not compatible with the skip duplicate test, at least not without tweaks). I did add a couple checks for merge strategy and linear history (which we should be able to mark as required once this lands) |
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.
LGTM! Just one minor comment for consideration.
Thanks for doing this work... looking forward to the next review.
.github/workflows/mergify-ready.yml
Outdated
github.event.pull_request.draft == true || | ||
github.event.pull_request.base.ref != 'master' || ( | ||
contains(github.event.pull_request.labels.*.name, 'automerge:squash') || | ||
contains(github.event.pull_request.labels.*.name, 'automerge:merge') || |
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.
FWIW, I'd like to rename automerge:merge
to automerge:expert
, in accordance with earlier discussions how it might otherwise be overused by people who may not know its nuances.
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.
Or automerge:no-update
.
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.
Or automerge:manual-updates
.
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.
I already updated the label description to indicate it's an expert option, and I'm hoping the new no-merge-commits check will help reduce it's usage (it should bump it off the queue if a merge commit is added to make the PR up to date)
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.
Out of these I prefer the automerge:no-updates
0d6ccef
to
e0eb08b
Compare
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.
I can speak on behalf of the DataDog test recording integration: it won't break since it's dynamically registered, but may possibly start logging to a different metric than before. As long as @raphdev is aware, I think this is safe to merge.
As discussed, the |
refs: #7629
Description
This combines all longer integration type tests into a single workflow.
It also restores the logic to avoid running these tests on PRs before ready to merge.
It also restores duplicate checks to avoid re-running these tests on the exact same content.
It does not remove the github based concurrency limit for pull request, which makes these tests potentially cancel and re-start on the same content if the test was still in progress. I remember there were issues in the custom concurrent checks logic and I'll look into restoring that later.
This is the first step towards switching back to mergify.
Security Considerations
None
Scaling Considerations
Not production scaling, but this should help reining in how often these expensive CI jobs run.
Documentation Considerations
None for now
Testing Considerations
Will test the behavior on this PR
Upgrade Considerations
None