Skip to content
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

🔨 Only retry flaky tests when PR is ready for merging #7126

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Jan 28, 2025

What do these changes do?

When the PR is ready for merging (all conversations closed, all required approvals have been given and all tests look OK), Mergify will take over.
It will deal with flaky tests and make sure the code lands on master. It will restart the CI till it becomes green.

Why not let Mergify restart the CI on all fails? What if you have a breaking test? This will cause lots of useless CI runs.

Fix: using correct workflow file path

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK added the t:maintenance Some planned maintenance work label Jan 28, 2025
@GitHK GitHK changed the title 🎨 Only retry flaky tests when PR is approved 🎨 Only retry flaky tests when PR is ready for merging Jan 28, 2025
@GitHK GitHK marked this pull request as ready for review January 28, 2025 16:46
.github/mergify.yml Outdated Show resolved Hide resolved
@@ -24,19 +24,29 @@ queue_rules:

# NOTE: in case of flaky tests you above checks will fail
# the PR will be removed from the queue.
# This retries the checks, as soon as they become green,
# the PR will be pusehd to the queu again
# Once the PR is ready to be merged the flaky tests will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put a limit in retries ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll investigate If there is a sane way to do this. Currently I did not find any nice solutions

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my questions. thanks!

.github/mergify.yml Outdated Show resolved Hide resolved
Comment on lines +39 to +41
- "#approved-reviews-by>=2" # Requires 2 approving reviews
- "#changes-requested-reviews-by=0" # No changes requested
- "#review-threads-unresolved=0" # All review threads resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to define all these things? can't it just check the github PR status?
I find it annoying as these settings are typically defined at the github repository level. now this needs to be duplicated in this file? Actually this also includes draft and conflict. In that case Github would anyway prevent you from merging. can this thing obliterate that fact??? if yes that looks dangerous. if not then I don't see the point in duplicating these settings.

Also other question:

  • what about hotfix branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This configuration file is not that clear. Unfortunately I also got very confused on how to use it at first.

This section pull_request_rules has nothing to do with the "Merge Queue" (section queue_rules). This part instructs the "Workflow Automation" part of Mergify to restart PRs whose tests have failed.
The "Merge Queue" part already respects and enforces all checks setup in GitHub.
The "Workflow Automation" does not know anything about these rules, so as a good measure when restarting the CI jobs (due to flaky tests) we check that the PR is ready to be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I want to try this only on the PRs but it can be added to all sorts of branches even hot fix ones. We can have a regex on the name of the branch and if it's a hotfix we let the "Workflow Automation" to restart it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. but still I don't get why you need to put all the criteria like "approved-reviews-by>=2" for example. this is setup in the settings of github. I don't get why this needs to be there.

@pcrespov pcrespov merged commit 483d4e4 into ITISFoundation:master Jan 29, 2025
57 checks passed
@GitHK GitHK deleted the pr-osparc-mergify-only-retries-when-pr-is-approved branch January 29, 2025 10:04
@GitHK GitHK changed the title 🎨 Only retry flaky tests when PR is ready for merging 🔨 Only retry flaky tests when PR is ready for merging Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants