-
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: fix mergify matrix integration tests #8267
Conversation
abbaacd
to
f327e26
Compare
d896682
to
13925e9
Compare
13925e9
to
ddd6443
Compare
.mergify.yml
Outdated
- or: | ||
- label=bypass:integration |
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.
Why not go all the way, and have a single integration-result
? Is there something I'm missing?
Then most of this logic can collapse into just:
- or:
- label=bypass:integration
- check-success=integration-result
- check-neutral=integration-result
- check-skipped=integration-result
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.
Yeah I guess. I had tried that before, but with the wrong if
condition. This should work just as well
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.
Done. I also removed the label=bypass:integration
because that label being applied will caused the result job to be skipped, and makes the logic for mergify simpler to realize the condition has no other way to succeed.
ddd6443
to
98a408c
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.
Very pretty and squeaky clean! Just one question, but I'll approve assuming you have a good answer.
@@ -311,3 +317,22 @@ jobs: | |||
timeout-minutes: 4 | |||
with: | |||
datadog-token: ${{ secrets.DATADOG_API_KEY }} | |||
|
|||
integration-test-result: |
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 love it! This is totally clear and maintainable.
.github/workflows/integration.yml
Outdated
# Mark a failure of `test` bootstrap job as a successful result | ||
continue-on-error: ${{ matrix.bootstrap-version == 'test' }} |
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.
Since I don't really know the motivation behind having a separate 'test'
bootstrap version (I think just that it was different than the 'main'
mode for vaults), is there a reason its result should be ignored? Is this just codifying behaviour we already had, avoiding a flake, or something else?
98a408c
to
3e0bc6c
Compare
Description
Mergify can get stuck when matrix tests fail, not realizing that the PR cannot make forward progress anymore.
This change reports a matrix job result as normal job, and switches mergify to using that job's result instead
Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
Will manually test variations on this PR
Upgrade Considerations
None