-
Notifications
You must be signed in to change notification settings - Fork 253
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
Coverity CI changes: remove internal workflow and add 'covscan' label workflow for PRs #7698
Coverity CI changes: remove internal workflow and add 'covscan' label workflow for PRs #7698
Conversation
42c1038
to
10b9545
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.
LGTM! Thank you for taking care of it
.github/workflows/coverity.yml
Outdated
workflow_dispatch: | ||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
jobs: | ||
coverity: | ||
coverity_daily: |
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.
Isn't this going to be run on each pull request?
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.
You are right, I missed that. I added if: ${{ github.event_name == 'schedule' }}
to this job
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 think we can keep it as single job. Add a step that will get check the labels, if labels are not there and it is a pull request -> skip/cancel.
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 switched it to a single job with a multi-line if conditional:
+ if: |
+ ${{ github.event_name == 'schedule' }} ||
+ ${{ github.event.label.name == 'coverity' && github.event_name == 'pull_request' }}
.github/workflows/coverity.yml
Outdated
# This job likely cannot be re-run on the same day or we will | ||
# hit the build submission limit (see above link) | ||
pull_request_target: | ||
branches: [master] |
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.
shouldn't you narrow it down to
types:
- labeled
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 could be, do you suggest if someone wants to run covscan against their PR more than once they should just remove and re-add the label? If yes I will add a comment in this workflow about this.
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. I think that current implementation will run covscan on each push if label is set, won't t? We need to trigger it only on desire. So either by setting the label or maybe by /covscan
comment.
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.
Okay makes sense, I added the
types:
- labeled
889b617
to
d152355
Compare
.github/workflows/coverity.yml
Outdated
workflow_dispatch: | ||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
jobs: | ||
coverity: | ||
if: | | ||
${{ github.event_name == 'schedule' }} || | ||
${{ github.event.label.name == 'coverity' && github.event_name == 'pull_request' }} |
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.
pull_request_target instead of pull_request?
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.
Fixed.
d152355
to
53123ef
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.
LGTM, let's see how it works
Pushed PR: #7698
|
No description provided.