-
Notifications
You must be signed in to change notification settings - Fork 252
CLDSRV-814: prevent premature codecov results #6032
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
CLDSRV-814: prevent premature codecov results #6032
Conversation
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development/9.0 #6032 +/- ##
================================================
Coverage 83.39% 83.39%
================================================
Files 189 189
Lines 12208 12208
================================================
Hits 10181 10181
Misses 2027 2027
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| require_ci_to_pass: true | ||
| notify: | ||
| wait_for_ci: true | ||
| after_n_builds: 9 |
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 needed since you also specify "wait for ci" ?
(Generally, I dislike this option as it creates a risk that we forget to update it, and possibly lose the notification if the workflow is reworked to reduce number of "reports")
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.
Yes it is needed, if I only leave wait_for_ci it will get a single report, and wait for the job of that particular report to finish, instead of waiting for the whole workflow to finish.
And yes it does create a bit of maintenance, but honestly in my experience number of reports tend to go up, instead of down. So I don't think it's that much of a big deal.
| --- | ||
| codecov: | ||
| require_ci_to_pass: true | ||
| notify: |
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.
Does this affect GitHub comment?
There is another setting explicitly affecting it if I remember correctly, but it has fewer options.
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.
It does, the github comment will wait for those conditions to be reached before being sent now.
|
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-814. Goodbye tcarmet. The following options are set: approve |
Prevent premature codecov results to be published on PR status before all tests successfully run and pass.
development/9.0contains 9 codecov uploads,development/9.2contains 10, should be fine to set it up as 9 starting from9.0and if needed we can bump it.