Skip to content

ci: Add linter workflow for C++ and Bazel files - #637

Open
thompson-tomo wants to merge 1 commit into
open-telemetry:mainfrom
thompson-tomo:linter
Open

ci: Add linter workflow for C++ and Bazel files#637
thompson-tomo wants to merge 1 commit into
open-telemetry:mainfrom
thompson-tomo:linter

Conversation

@thompson-tomo

Copy link
Copy Markdown
Contributor

This adds clang/Buildifier to perform linting as done in httpd but now it is the whole repo.

@thompson-tomo
thompson-tomo marked this pull request as ready for review July 26, 2026 04:21
@thompson-tomo
thompson-tomo requested a review from a team as a code owner July 26, 2026 04:21
steps:
- name: checkout otel contrib
uses: actions/checkout@v7.0.1
- uses: cpp-linter/cpp-linter-action@v2.20.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Would these scripts work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lalitb. Good catch! I'll submit PRs to update the format tools scripts with these file extensions.

@dbarker dbarker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants