-
Notifications
You must be signed in to change notification settings - Fork 665
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(cppcheck-all): rename to cppcheck-daily and make it run only daily #7312
Conversation
Signed-off-by: kminoda <koji.minoda@tier4.jp>
1ad2623
to
d4c3055
Compare
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Why did you remove - name: Fail the job if Cppcheck failed
if: steps.cppcheck.outcome == 'failure'
run: exit 1 Part, I think it's better if it fails on failure. So we can track where things went wrong. |
@xmfcx Now that this CI is going to be a monitor rather than a check, I think it would be better to make it succeed regardless of the cppcheck results. Also it is obvious that cppcheck still outputs bunch of errors. But if this may be confusing for other developers, I can leave that part. What do you think? |
I still think it should fail on failure. It is a daily job anyways. Most people won't even see it so it is ok. |
It is not obvious, it will only be known if you click into the individual workflow run logs.
This action can stay to run daily forever. Maybe some files will go past cppcheck somehow, this will warn us in general.
It won't be, it is only accessible if someone purposefully goes to https://github.com/autowarefoundation/autoware.universe/actions/workflows/cppcheck-monitor.yaml page. Because it is a scheduled daily job. And people going to that link wants to know the answer to this question: "since when was this workflow failing?" If you hide it, then we need to click each workflow run one by one. |
To be honest, the name |
Signed-off-by: kminoda <koji.minoda@tier4.jp>
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.
Thank you! Let's wait for pre-commit.ci to fix the file ending and we can merge ✅
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
…autowarefoundation#7312) Signed-off-by: kminoda <koji.minoda@tier4.jp>
…#7312) Signed-off-by: kminoda <koji.minoda@tier4.jp>
Description
Rename cppcheck-all to cppcheck-monitor to make the name self-contained.
I also would like to make it pass always as it may be a noise for developers.
Tests performed
Please check the CI results of this PR (it should pass)
Effects on system behavior
None
Interface changes
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.