fix: relax run tool - #151
naveena456 wants to merge 6 commits into
Conversation
|
@naveena456 Without passing CI checks, I cannot merge this PR |
There was a problem hiding this comment.
🟡 Changes recommended
Consumer installations cannot access the version catalog, and the new test fails to exercise host behavior inside containerized CI.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds local pinned-tool reuse to avoid unnecessary Bazel execution.
Changes:
- Detects matching local tool versions with Bazel fallback.
- Adds
--strictmode. - Adds tests, documentation, and CI coverage.
File summaries
| File | Description |
|---|---|
tools/run-tool |
Implements version-aware selection and strict mode. |
tools/tests/run_tool_test.sh |
Tests selection and fallback paths. |
tools/README.md |
Documents the new behavior. |
tools/BUILD.bazel |
Registers the test target. |
.github/workflows/ci.yaml |
Runs the new test in CI. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| actionlint) printf '%s\n' "1.7.7" ;; | ||
| apm) printf '%s\n' "0.27.0" ;; | ||
| bazelisk) printf '%s\n' "1.27.0" ;; | ||
| buildifier) printf '%s\n' "8.2.1" ;; | ||
| opencode) printf '%s\n' "1.18.15" ;; | ||
| pre-commit) printf '%s\n' "4.5.1" ;; | ||
| ruff) printf '%s\n' "0.11.13" ;; | ||
| shellcheck) printf '%s\n' "0.10.0" ;; | ||
| starpls) printf '%s\n' "0.1.22" ;; | ||
| uv) printf '%s\n' "0.10.4" ;; | ||
| uvx) printf '%s\n' "0.10.4" ;; | ||
| yamlfmt) printf '%s\n' "0.17.0" ;; |
There was a problem hiding this comment.
IMHO this is a road block. This means the script cannot be copied and used across many devcontainer updates, but instead it will need constant maintenance at user repos.
bazel knows what version to use, but we explicitly want to avoid calling it. Would it be possible to generate mini lockfile from bazel which only contains the versions of these tools? Preferably in a format we can already parse.
Then bazel needs to be called once to create the mini lockfile. The tool versions are sourced from this lockfile which should be way faster. run-tool could then track the the modification time of both the mini lockfile and MODULE.bazel.lock. If MODULE.bazel.lock has a newer time, the mini lockfile needs to be regenerated.
If you think this is too complicated, I am also open to ignore the tool version and just call the tool, when it is present on the host. I am not very thrilled by how much complexity was added already with this PR and previous ones to abstract tool execution via bazel and host.
@AlexanderLanin What do you think?
There was a problem hiding this comment.
solution makes sense I guess, for the problem as stated. However I did not anticipate that we would end up with tool version lists in every user repo. No good ideas at the moment. Maybe ignoring versions is the next best thing. Lets sleep on it.
|
This PR is no longer applicable due to a scope change during the review process. Some implementation complexity was identified during the review and has already been addressed in #154 , which supersedes this PR. |
|
stale |
Enhanced the script to avoid unnecessary Bazel execution when a matching local tool is available.
Closes #148