Skip to content

feat: configure pre-commit hooks via CLI args, add no-banner-comment-check#39

Open
alexmohr wants to merge 1 commit into
mainfrom
feat/pre-commit-hook-args-config
Open

feat: configure pre-commit hooks via CLI args, add no-banner-comment-check#39
alexmohr wants to merge 1 commit into
mainfrom
feat/pre-commit-hook-args-config

Conversation

@alexmohr
Copy link
Copy Markdown
Contributor

@alexmohr alexmohr commented May 6, 2026

Summary

  • Hook scripts (reuse-annotate-hook.py, no-unicode-check.py) now accept configuration as CLI args (--copyright, --license, --template, --ignore-paths, --allowed-chars) instead of env vars / patched constants
  • Configuration now lives in .pre-commit-config.yml under each hook's args: block — no runtime patching needed
  • run_checks.py no longer patches configs or hook scripts at runtime; patch_config and patch_hook_script are removed along with all the flags they served
  • action.yml drops 6 flags from the run_checks.py invocation and sets SKIP=reuse-annotate directly in the step env
  • pr-commit.py added as a zero-argument local entry point: uv run pr-commit.py
  • no-unicode-check hook is now a static entry in .pre-commit-config.yml (previously injected at runtime), covering .py,.yml,.toml,.jinja2
  • no-banner-comment-check.py added as a new hook to flag banner-style comments

⚠️ Breaking Changes

Lint strictness escalated in shared-lints.toml:

  • pedantic changed from warn to deny (priority -1) — all pedantic lint violations will now hard-fail CI instead of warning
  • clone_on_ref_ptr changed from warn to deny — cloning reference-counted pointers via .clone() instead of Arc::clone() / Rc::clone() is now a CI failure

Consumer repos with existing pedantic lint violations or .clone() on Arc/Rc will see CI failures after upgrading. Remediation: fix the violations or temporarily override in the repo's Cargo.toml with [lints.clippy] pedantic = { level = "warn", priority = -1 }.

Consumer migration

Consumer repos using a custom .pre-commit-config.yml (via pre-commit-config-path) should configure the opensovd hooks as local entries pointing to pinned raw URLs:

repos:
  - repo: local
    hooks:
      - id: reuse-annotate
        name: Add missing license headers (REUSE)
        # Replace <SHA> with a pinned commit from eclipse-opensovd/cicd-workflows
        entry: uv run https://raw.githubusercontent.com/eclipse-opensovd/cicd-workflows/<SHA>/pre-commit-action/reuse-annotate-hook.py
        args:
          - --copyright=Your Copyright Holder       # SPDX-FileCopyrightText value
          - --license=Apache-2.0                    # SPDX license identifier
          - --template=your-template                # name of .reuse/templates/<name>.jinja2 in your repo
          - --ignore-paths=docs/**,*.md             # comma-separated glob patterns to skip
        language: system
        pass_filenames: true
        exclude: ^(LICENSE|NOTICE|CONTRIBUTORS)$
      - id: no-unicode-check
        name: No Unicode characters allowed
        entry: uv run https://raw.githubusercontent.com/eclipse-opensovd/cicd-workflows/<SHA>/pre-commit-action/no-unicode-check.py
        args:
          - --allowed-chars=µ,§                     # comma-separated characters to permit (omit = reject all non-ASCII)
        language: system
        files: '\.(py|yml|toml|jinja2)$'            # extensions to scan
        pass_filenames: true
      - id: no-banner-comment-check
        name: No banner-style comments allowed
        entry: uv run https://raw.githubusercontent.com/eclipse-opensovd/cicd-workflows/<SHA>/pre-commit-action/no-banner-comment-check.py
        args:
          - --banner-chars==\-#\*/~_+               # regex char class of fill characters
          - --min-length=5                           # minimum run of fill chars to flag a line
        language: system
        files: '\.(py|yml|toml|jinja2)$'            # extensions to scan
        pass_filenames: true

Each hook is configured entirely through its args: block. The files: pattern controls which extensions are checked.

Checklist

  • I have tested my changes locally
  • I have added or updated documentation
  • I have linked related issues or discussions
  • I have added or updated tests

Related

Closes #34, #36, #37 (features integrated into this branch and draft PRs closed)
See also: #40 (reusable conventional-commits and release workflows, split from this PR)

Notes for Reviewers

The no-unicode-check hook was previously injected into the config at runtime by patch_config() — it is now a static hook entry in .pre-commit-config.yml. Consumers who relied on no-unicode-extensions being empty (disabled) should remove the no-unicode-check hook from their custom config.

The conventional-commits.yml and release.yml workflows have been moved to a separate PR (#40) to keep this PR focused.


Alexander Mohr alexander.m.mohr@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information

@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch 5 times, most recently from 45520ab to 5eeef2c Compare May 6, 2026 04:55
@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch 2 times, most recently from db7edaf to ede8cd8 Compare May 6, 2026 05:02
@alexmohr
Copy link
Copy Markdown
Contributor Author

alexmohr commented May 6, 2026

Fyi @lh-sag this makes the cicd usage much closer to standard uv and makes usage in core feasible. #38 documents what core has and what is still missing here.
Aligning on ci is done fairly easy, so imho a good first step to get all repos on the same page.
As soon as your PR in core is merged we can integrate the PR checks etc. here as well, then start migrating in core and probably others.
For the in depth code stuff I'll wait for our workshop(s).

@lh-sag
Copy link
Copy Markdown

lh-sag commented May 6, 2026

Fyi @lh-sag this makes the cicd usage much closer to standard uv and makes usage in core feasible. #38 documents what core has and what is still missing here. Aligning on ci is done fairly easy, so imho a good first step to get all repos on the same page. As soon as your PR in core is merged we can integrate the PR checks etc. here as well, then start migrating in core and probably others. For the in depth code stuff I'll wait for our workshop(s).

With the recent supply-chain attacks e.g. trivy, is there a way to run scripts locally and w/o internet access?

@alexmohr
Copy link
Copy Markdown
Contributor Author

alexmohr commented May 6, 2026

Fyi @lh-sag this makes the cicd usage much closer to standard uv and makes usage in core feasible. #38 documents what core has and what is still missing here. Aligning on ci is done fairly easy, so imho a good first step to get all repos on the same page. As soon as your PR in core is merged we can integrate the PR checks etc. here as well, then start migrating in core and probably others. For the in depth code stuff I'll wait for our workshop(s).

With the recent supply-chain attacks e.g. trivy, is there a way to run scripts locally and w/o internet access?

To run it locally you would need to point the uv config, as shown on the PR description, to a local file instead. But this isn't really practical for the CI.
I will add a commit that pins all versions, as the invocation of the Python scripts is made through SHA instead of a branch, we should be fairly well protected against supply chain attacks. But there is no real way for an offline mode while still sharing the config and not duplicating a lot of code.

Are you mainly concerned about supply chain attacks? (Imho we're almost good to go here) Or do you need offline support?

@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch 7 times, most recently from 6b3e296 to 51576e9 Compare May 6, 2026 14:55
@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch 6 times, most recently from 477c064 to 9b1134e Compare May 7, 2026 09:02
@alexmohr alexmohr marked this pull request as ready for review May 7, 2026 09:08
@alexmohr alexmohr changed the title feat: configure pre-commit hooks via args instead of run_checks.py flags feat: configure pre-commit hooks via CLI args, add no-banner-comment-check May 7, 2026
Copy link
Copy Markdown
Contributor Author

@alexmohr alexmohr left a comment

Choose a reason for hiding this comment

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

After this PR merges to main, update line 43 of .pre-commit-config.yaml from rev: feat/pre-commit-hook-args-config to rev: main (or pin to the merge commit SHA for reproducibility). The branch name is used here because the SHA is not reachable from main until the PR merges.

Comment thread .pre-commit-config.yaml
@lh-sag
Copy link
Copy Markdown

lh-sag commented May 11, 2026

Fyi @lh-sag this makes the cicd usage much closer to standard uv and makes usage in core feasible. #38 documents what core has and what is still missing here. Aligning on ci is done fairly easy, so imho a good first step to get all repos on the same page. As soon as your PR in core is merged we can integrate the PR checks etc. here as well, then start migrating in core and probably others. For the in depth code stuff I'll wait for our workshop(s).

With the recent supply-chain attacks e.g. trivy, is there a way to run scripts locally and w/o internet access?

To run it locally you would need to point the uv config, as shown on the PR description, to a local file instead. But this isn't really practical for the CI. I will add a commit that pins all versions, as the invocation of the Python scripts is made through SHA instead of a branch, we should be fairly well protected against supply chain attacks. But there is no real way for an offline mode while still sharing the config and not duplicating a lot of code.

Are you mainly concerned about supply chain attacks? (Imho we're almost good to go here) Or do you need offline support?

I am not comfortable that my local pre-commit runs scripts from a remote repository every time I invoke it. This would also require internet connectivity even if you develop fully locally.

Cant we have a local one?

@alexmohr
Copy link
Copy Markdown
Contributor Author

I am not comfortable that my local pre-commit runs scripts from a remote repository every time I invoke it. This would also require internet connectivity even if you develop fully locally.

I'll look into how these things can be cached when prek is running the first time and is not doing the download on every run. This way it can be run fully locally while still sharing a configuration.

I really want to have the repos in sync because, so we can share common things like the markdown lint you added in core but no one else had enabled.
I do understand that running this fully offline after the first download is a must have. (The first run must be online with the current opensovd-core configuration too, because it has to download things like markdown lint etc.)

If I don't find a way to fully achieve offline support I can agree on scrapping this. But I'm not ready to give this up just yet :)

@lh-sag
Copy link
Copy Markdown

lh-sag commented May 11, 2026

What I like in s-core is that they have another layer between CI and repository - bazel. For us this does not need to be bazel but we could offload e.g. build environment and certain configuration to a devcontainer. This can be re-used by developers and CI. I am happy that we have CI but github actions is quite annoying and if we can streamline CI w/o touching too much of the actions I would be more than happy.

So the general question is how we want out CI/CD architecture to be? github actions + devcontainer + common entries, which allows for customization. There are more alternatives but I feel the s-core way is having benefit especially with our fragmented project setup.

@alexmohr
Copy link
Copy Markdown
Contributor Author

alexmohr commented May 11, 2026

What I like in s-core is that they have another layer between CI and repository - bazel. For us this does not need to be bazel but we could offload e.g. build environment and certain configuration to a devcontainer. This can be re-used by developers and CI. I am happy that we have CI but github actions is quite annoying and if we can streamline CI w/o touching too much of the actions I would be more than happy.

I personally prefer not having to use a devcontainer with a lot of things baked in because compiling it locally without any containers is just a bit easier imho.
I do think we need to maintain support to build with cargo and no bazel. But we will need bazel anyway to integrate with S-Core, so it might be an opportunity to provide additional checks via bazel. If this enables is re-use of S-Core things,.even better.

So the general question is how we want out CI/CD architecture to be? github actions + devcontainer + common entries, which allows for customization. There are more alternatives but I feel the s-core way is having benefit especially with our fragmented project setup.

The configuration should work fully offline now. I do need more testing though. As said above I'm all for seeking out benefits in getting closer to S-Core but we have to maintain an option to build "only" open SOVD.
This approach could be used to align within opensovd.
For example we could further align with score for things like the formatting and especially clippy. But last time I checked the clippy rules where too strict in score. I.e. they do not even allow except or unwrap in tests which makes testing a bit cumbersome.

We probably should discuss next week what we can adopt from score right away, so we don't reinvent the wheel whole making integration also easier.

I'm adding quite a few things here because these are mostly findings I kept commenting repeatedly on PRs

@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch 6 times, most recently from fdb5145 to 489ddbd Compare May 18, 2026 08:54
@alexmohr alexmohr force-pushed the feat/pre-commit-hook-args-config branch from 489ddbd to 671b24a Compare May 18, 2026 11:32
@alexmohr alexmohr marked this pull request as ready for review May 18, 2026 12:39
@alexmohr
Copy link
Copy Markdown
Contributor Author

Marking this as ready to review.
Now supports to be fully offline, as the checks in this repo are just precommit / prek hooks like any other hook.
Downloaded once at init, then works offline.

Once this is merged, we can start using this in CDA eclipse-opensovd/classic-diagnostic-adapter#328 over the next 2-3 weeks. If we don't find any issues during this I would create pull requests to update the remaining repos.
I prepared a PR in opensovd core already, the most changes come from the aligned copyright header and the updated rust format settings.
fyi @theswiftfox @floroks

# This program and the accompanying materials are made available under the
# terms of the Apache License Version 2.0 which is available at
# https://www.apache.org/licenses/LICENSE-2.0
style_edition = "2024"
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.

also add

unstable_features = true

thx @mbfm

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.

2 participants