ci: Add linter workflow for C++ and Bazel files - #637
Conversation
| steps: | ||
| - name: checkout otel contrib | ||
| uses: actions/checkout@v7.0.1 | ||
| - uses: cpp-linter/cpp-linter-action@v2.20.0 |
There was a problem hiding this comment.
since this is just for clang-format and buildifier can we use the existing cpp-build-tools docker image?
https://github.com/open-telemetry/cpp-build-tools/tree/main/cpp_format_tools
There was a problem hiding this comment.
Hmm, not sure of the benefit a docker image brings other than becoming another repo to maintain especially given how minimal it does. Using dedicated checks we have better control over filtering and when using a ready to go action we can annotations https://github.com/open-telemetry/opentelemetry-cpp-contrib/actions/runs/30187032106/job/89753466790 which make it easier to see issues as they appear directly in the diff view.
There was a problem hiding this comment.
The action looks nice and getting feedback in the PR by line for clang-tidy warnings would be helpful (perhaps less helpful for formatting).
The main concern with a new Github Action from the marketplace is security. Since the docker image is managed by opentelemetry-cpp maintainers and the repo is controlled by the OpenTelemetry org it is low effort to use here. An alternative is to use the ci scripts from opentelemetry-cpp since it is already a dependency.
Please see:
- https://github.com/open-telemetry/opentelemetry-cpp/blob/main/ci/install_format_tools.sh
- https://github.com/open-telemetry/opentelemetry-cpp/blob/98445d3a2f163808a98b242b06a4d3da97a31819/ci/do_ci.sh#L650
Would these scripts work?
There was a problem hiding this comment.
Hmm, yes they could work but it is something needing to be maintained and we also need to maintain the link etc.
Note https://github.com/zizmorcore/zizmor is being rolled out across the otel org, does that help to address your concerns about using the action?
There was a problem hiding this comment.
Thanks for the context.
The scripts above and otel/cpp_format_tools are already maintained and used in both CI and the dev image for opentelemetry-cpp.
We recently invested in a consistent dev/CI toolchain for formatting, clang-tidy, and IWYU. All rely on LLVM, and for clang-tidy/IWYU we manage LLVM versions explicitly to stay current. Introducing a separate marketplace action for formatting is unnecessary here and makes long-term alignment of tooling harder across core + contrib.
On zizmor: the rollout helps with general hardening/visibility, but it does not replace initial action vetting and ongoing review of action upgrades.
Please update this PR to use otel/cpp_format_tools instead of cpp-linter/cpp-linter-action.
Reference: https://github.com/open-telemetry/cpp-build-tools/tree/main/cpp_format_tools
Once updated, I’m happy to re-review.
There was a problem hiding this comment.
One thing to consider when switching to otel/cpp_format_tools: its current script formats only .cc and .h files. This repo also has many .cpp and some .hpp files, including nginx and webserver sources. We should make sure those extensions are covered so the new check is repo-wide.
There was a problem hiding this comment.
Thanks @lalitb. Good catch! I'll submit PRs to update the format tools scripts with these file extensions.
There was a problem hiding this comment.
Thanks for the PR. I'm requesting changes following the discussion here: #637 (comment)
Please use the otel/cpp_format_tools image to perform the format check and the following job template to include the harden-runner step and pin all actions:
format:
name: Format
runs-on: ubuntu-24.04
steps:
- name: Harden the runner (Audit all outbound calls)
uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0
with:
egress-policy: audit
This adds clang/Buildifier to perform linting as done in httpd but now it is the whole repo.