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

ci(cppcheck-all): rename to cppcheck-daily and make it run only daily #7312

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Jun 6, 2024

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda kminoda marked this pull request as ready for review June 6, 2024 08:09
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@github-actions github-actions bot added the type:ci Continuous Integration (CI) processes and testing. (auto-assigned) label Jun 6, 2024
kminoda and others added 2 commits June 6, 2024 20:40
@kminoda kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 6, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

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.

@kminoda
Copy link
Contributor Author

kminoda commented Jun 6, 2024

@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.
We can remove this CI eventually when all the cppcheck errors have been mitigated. (This way we may also be able to know when the CI is failing for some other reason, e.g. errors while installing cppcheck)

But if this may be confusing for other developers, I can leave that part. What do you think?

@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

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.

@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

@kminoda

Also it is obvious that cppcheck still outputs bunch of errors.

It is not obvious, it will only be known if you click into the individual workflow run logs.

We can remove this CI eventually when all the cppcheck errors have been mitigated. (This way we may also be able to know when the CI is failing for some other reason, e.g. errors while installing cppcheck)

This action can stay to run daily forever. Maybe some files will go past cppcheck somehow, this will warn us in general.

But if this may be confusing for other developers, I can leave that part. What do you think?

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.

@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

To be honest, the name cppcheck-all is better, it conveys what it does better. Maybe even cppcheck-daily.

.github/workflows/cppcheck-monitor.yaml Outdated Show resolved Hide resolved
.github/workflows/cppcheck-monitor.yaml Outdated Show resolved Hide resolved
.github/workflows/cppcheck-monitor.yaml Outdated Show resolved Hide resolved
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@xmfcx xmfcx changed the title ci(cppcheck-all): rename to cppcheck-monitor ci(cppcheck-all): rename to cppcheck-daily and make it run only daily Jun 6, 2024
Copy link
Contributor

@xmfcx xmfcx left a 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>
@xmfcx xmfcx merged commit f788069 into autowarefoundation:main Jun 6, 2024
16 of 17 checks passed
@kminoda kminoda deleted the ci/fix_cppcheck_all branch June 6, 2024 14:13
kyoichi-sugahara pushed a commit to kyoichi-sugahara/autoware.universe that referenced this pull request Jun 6, 2024
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:ci Continuous Integration (CI) processes and testing. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants