Skip to content

NO-ISSUE: Use uv run dev.py lint from the pre-commit hook#679

Open
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:use_dev_py_lint_from_pre_commit_hook
Open

NO-ISSUE: Use uv run dev.py lint from the pre-commit hook#679
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:use_dev_py_lint_from_pre_commit_hook

Conversation

@jhernand

@jhernand jhernand commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

The pre-commit hook was calling bin/golangci-lint directly, which required the tool to be
installed separately before linting could work. This changes the hook to call uv run dev.py lint
instead, so that the project's development tooling manages the linter lifecycle end to end.

  • The lint command now calls setup.install_golangci_lint before running, making it
    self-contained.
  • The is_installed helper in setup.py is updated to check the project's bin/ directory first
    and to use the resolved path when verifying the installed version.
  • The separate uv run dev.py setup step in the CI workflow is removed since lint now handles
    its own setup.

Test plan

  • Run uv run dev.py lint locally without golangci-lint pre-installed and verify it installs
    the tool and runs the linter successfully.
  • Run uv run dev.py lint locally with golangci-lint already in bin/ and verify it skips
    installation and runs the linter.
  • Trigger the pre-commit hook by committing a .go file and verify it invokes
    uv run dev.py lint.
  • Verify CI passes without the uv run dev.py setup step.

Summary by CodeRabbit

  • Chores
    • Streamlined CI/CD workflow by consolidating tool setup with linting operations.
    • Updated pre-commit hook configuration to use unified linting command.
    • Improved tool installation detection to prioritize project-level binaries.

The pre-commit hook was calling `bin/golangci-lint` directly, which meant
that the tool had to be installed separately before linting could work.
This changes the hook to call `uv run dev.py lint` instead, so that the
project's development tooling manages the linter lifecycle end to end.

To support this the `lint` command now calls `setup.install_golangci_lint`
before running the linter, making it self-contained. The `is_installed`
helper in `setup.py` is also updated to look in the project's `bin/`
directory first and to use the resolved path when checking the installed
version, so it correctly detects tools already downloaded by previous
runs.

Because `lint` now installs its own dependencies, the separate
`uv run dev.py setup` step in the CI workflow is no longer needed and
has been removed.

Assisted-by: Cursor
Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR refactors the linting workflow to prefer project-local golangci-lint binaries, embed tool installation into the lint command, and unify CI and pre-commit hooks to use a single dev.py lint entry point. Binary path resolution, lint integration, and workflow unification are coordinated to ensure consistent tooling.

Changes

Linting workflow unification

Layer / File(s) Summary
Binary path resolution in setup
dev/setup.py
is_installed() prefers project bin/<tool.name> over system PATH and validates versions using the resolved binary path, ensuring consistency between detected and actual tool versions.
Lint command with integrated tool installation
dev/lint.py
lint command imports setup and calls setup.install_golangci_lint() before running linting with the literal golangci-lint binary name.
CI and pre-commit workflow integration
.github/workflows/check-pull-request.yaml, .pre-commit-config.yaml
CI check-go-code job removes separate setup step and relies on lint's embedded installation; pre-commit golangci-lint hook entry is unified to uv run dev.py lint.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Possibly related PRs

  • osac-project/fulfillment-service#675: This PR refactors the lint/setup integration originally introduced in PR #675, consolidating binary resolution and tool installation into a unified workflow.

Suggested labels

approved, lgtm


Suggested reviewers

  • rgolangh
  • eliorerz

Poem

🔨 Tools once scattered, now aligned,
Prefer the bin, leave chaos behind,
Setup embedded, workflows in sync,
One path to lint—no missing link! ✨

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: refactoring the pre-commit hook to use uv run dev.py lint instead of invoking bin/golangci-lint directly. It is concise, specific, and clearly communicates the primary objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed No hardcoded secrets (API keys/tokens/passwords/private keys), embedded-cred URLs, or valid >32-char base64 strings found in PR-changed files vs origin/main; risk: low/no impact.
No-Weak-Crypto ✅ Passed Scanned PR’s changed files (.github/workflows/check-pull-request.yaml, .pre-commit-config.yaml, dev/lint.py, dev/setup.py): no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom crypto found; dev/setup u...
No-Injection-Vectors ✅ Passed No SQL concat, shell=True/user input, python eval/exec builtins, pickle.loads, unsafe yaml.load, os.system, or dangerouslySetInnerHTML found in the PR changes; commands.eval only runs a subprocess...
Container-Privileges ✅ Passed Repo manifests contain no privileged:true/hostPID/hostNetwork/hostIPC/SYS_ADMIN and no allowPrivilegeEscalation:true; affected deployments set allowPrivilegeEscalation:false and runAsNonRoot:true.
No-Sensitive-Data-In-Logs ✅ Passed PR changes only remove CI setup step and switch lint invocation; added/affected logs are generic (“Running linter”, version/checksum), with no tokens/passwords/PII.
Ai-Attribution ✅ Passed Commit f7bace3 includes “Assisted-by: Cursor” and no “Co-authored-by” for AI; required attribution trailers are present. Low risk.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci-robot

Copy link
Copy Markdown

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

The pre-commit hook was calling bin/golangci-lint directly, which required the tool to be
installed separately before linting could work. This changes the hook to call uv run dev.py lint
instead, so that the project's development tooling manages the linter lifecycle end to end.

  • The lint command now calls setup.install_golangci_lint before running, making it
    self-contained.
  • The is_installed helper in setup.py is updated to check the project's bin/ directory first
    and to use the resolved path when verifying the installed version.
  • The separate uv run dev.py setup step in the CI workflow is removed since lint now handles
    its own setup.

Test plan

  • Run uv run dev.py lint locally without golangci-lint pre-installed and verify it installs
    the tool and runs the linter successfully.
  • Run uv run dev.py lint locally with golangci-lint already in bin/ and verify it skips
    installation and runs the linter.
  • Trigger the pre-commit hook by committing a .go file and verify it invokes
    uv run dev.py lint.
  • Verify CI passes without the uv run dev.py setup step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci Bot requested review from eliorerz and rgolangh June 12, 2026 08:23
@openshift-ci

openshift-ci Bot commented Jun 12, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jhernand

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jun 12, 2026

Copy link
Copy Markdown

@jhernand: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vmaas f7bace3 link true /test e2e-vmaas
ci/prow/images f7bace3 link true /test images
ci/prow/unit f7bace3 link true /test unit

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@sk-ilya

sk-ilya commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@sk-ilya

sk-ilya commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

/retest-required

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants