feat: add run-linters script for native linting without super-linter#136
feat: add run-linters script for native linting without super-linter#136zeitlinger wants to merge 3 commits intomainfrom
Conversation
3346abc to
aa5183f
Compare
- New tasks/lint/run-linters.sh: runs native linters with changed-file
detection (merge base + staged + unstaged), --autofix, --full flags,
and a built-in tool registry. Callers pass tool names as args:
run-linters prettier markdownlint shfmt
- RUN_LINTERS_EXTRA_REGISTRY env var for extending the registry
- Generalize golangci-lint {MERGE_BASE} handling in super-linter.sh
- Add bats tests and mise test task
aa5183f to
298876e
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new lightweight native-lint runner script (as an alternative to Super-Linter native mode) with changed-file detection, plus a Bats test suite and mise wiring to run those tests. Also updates super-linter.sh to generalize merge-base placeholder handling and filter out deleted files from cached file lists.
Changes:
- Add
tasks/lint/run-linters.shwith a built-in linter registry, changed-file detection,--autofix,--full, and{MERGE_BASE}placeholder support. - Update
tasks/lint/super-linter.shnative mode to use{MERGE_BASE}placeholder substitution and filter cached files by existence. - Add Bats tests and mise task/tool entries to run them.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tasks/lint/run-linters.sh |
New script to run selected native linters using changed-file detection and a registry-based configuration model. |
tasks/lint/super-linter.sh |
Refactors merge-base handling into a generalized {MERGE_BASE} placeholder and filters deleted files out of cached lists. |
tests/run-linters.bats |
Adds Bats coverage for changed-file detection, --full, --autofix, failures, missing tools, and no-match behavior. |
mise.toml |
Adds bats tool + tasks.test to run the new suite. |
.github/renovate-tracked-deps.json |
Tracks the new mise-managed tools for Renovate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "VALIDATE_GITHUB_ACTIONS|actionlint|actionlint {FILE}||.github/workflows/*.yml .github/workflows/*.yaml" | ||
| "VALIDATE_DOCKERFILE_HADOLINT|hadolint|hadolint {FILE}||Dockerfile Dockerfile.* *.dockerfile" | ||
| "VALIDATE_GO_GOLANGCI_LINT|golangci-lint|golangci-lint run||SELF" | ||
| "VALIDATE_GO_GOLANGCI_LINT|golangci-lint|golangci-lint run --new-from-rev={MERGE_BASE}||SELF" |
There was a problem hiding this comment.
--full is intended to lint the entire codebase, but golangci-lint is now always invoked with --new-from-rev={MERGE_BASE} when a merge base exists. That keeps golangci-lint in diff-only mode even when LINT_ALL=true.
Suggestion: only include/expand the {MERGE_BASE} flag when LINT_ALL != true (or strip --*={MERGE_BASE} when LINT_ALL=true).
| "VALIDATE_GO_GOLANGCI_LINT|golangci-lint|golangci-lint run --new-from-rev={MERGE_BASE}||SELF" | |
| "VALIDATE_GO_GOLANGCI_LINT|golangci-lint|golangci-lint run \$( [ \"\${LINT_ALL}\" != \"true\" ] && printf '%s' '--new-from-rev={MERGE_BASE}' )||SELF" |
| # Substitute {MERGE_BASE}; strip --flag={MERGE_BASE} entirely when no merge base available | ||
| if [ -n "$_MERGE_BASE" ]; then | ||
| cmd_template="${cmd_template//\{MERGE_BASE\}/$_MERGE_BASE}" | ||
| else | ||
| cmd_template=$(printf '%s' "$cmd_template" | sed 's/ \?--[a-zA-Z_-]*={MERGE_BASE}//g') | ||
| fi |
There was a problem hiding this comment.
The {MERGE_BASE} substitution logic doesn’t consider LINT_ALL. With --full, _MERGE_BASE will typically be non-empty, so commands like golangci-lint run --new-from-rev={MERGE_BASE} will still run in diff-only mode, contradicting --full semantics.
Suggestion: when LINT_ALL=true, skip {MERGE_BASE} substitution and remove any --*={MERGE_BASE} flags (even if _MERGE_BASE is set).
| trap _on_exit EXIT | ||
|
|
||
| _LINTER_RAN=true | ||
| _failed=() | ||
| _skipped=() | ||
|
|
There was a problem hiding this comment.
_LINTER_RAN is set to true before any linter command is actually executed. This causes the EXIT trap to print the “Try mise run fix…” hint even for failures like an unknown linter name or other early exits.
Suggestion: only set _LINTER_RAN=true immediately before running the first linter command (or gate the hint on a separate flag indicating a linter command was invoked).
| _register actionlint "actionlint {FILE}" "" ".github/workflows/*.yml .github/workflows/*.yaml" | ||
| _register hadolint "hadolint {FILE}" "" "Dockerfile Dockerfile.* *.dockerfile" | ||
| _register codespell "codespell {FILES}" "codespell --write-changes {FILES}" "*" | ||
| _register ec "ec {FILES}" "" "*" |
There was a problem hiding this comment.
The editorconfig checker is registered as ec and invoked as ec {FILES}, but the rest of the repo (e.g. tasks/lint/super-linter.sh) uses the editorconfig-checker binary. If ec isn’t available on PATH, this entry will always be reported as a missing tool.
Suggestion: switch the command to editorconfig-checker {FILES} (and optionally keep ec as an alias tool name if you want the shorter invocation).
| _register ec "ec {FILES}" "" "*" | |
| _register ec "editorconfig-checker {FILES}" "" "*" |
| git -C "$TMPDIR/repo" add baseline.txt changing.txt | ||
| git -C "$TMPDIR/repo" commit -m "initial" -q | ||
| git -C "$TMPDIR/repo" branch -M main | ||
| git -C "$TMPDIR/repo" push origin main -q |
There was a problem hiding this comment.
After git push origin main, the local repo typically won’t have a refs/remotes/origin/main remote-tracking ref until a git fetch is run. Since run-linters.sh computes the merge base against origin/main, these tests may fall back to git ls-files (linting everything) and become flaky.
Suggestion: add a git -C "$TMPDIR/repo" fetch -q origin main (or equivalent) after the push so origin/main exists locally.
| git -C "$TMPDIR/repo" push origin main -q | |
| git -C "$TMPDIR/repo" push origin main -q | |
| git -C "$TMPDIR/repo" fetch -q origin main |
| grep -q "check:changing.txt" "$MOCK_LOG" | ||
| grep -q "check:new.txt" "$MOCK_LOG" | ||
| run ! grep -q "check:baseline.txt" "$MOCK_LOG" | ||
| } |
There was a problem hiding this comment.
run ! grep ... doesn’t do shell negation in Bats; run executes the first argument as a command, so this will try to execute a program named ! (status 127) rather than asserting that grep fails.
Suggestion: use run grep -q ... and then assert [ "$status" -ne 0 ] (or just use ! grep -q ... without run).
| [ "$status" -eq 0 ] | ||
| grep -q "fix:changing.txt" "$MOCK_LOG" | ||
| run ! grep -q "check:" "$MOCK_LOG" | ||
| } |
There was a problem hiding this comment.
Same issue as above: run ! grep ... will try to execute ! as a command rather than negating grep’s exit status.
Suggestion: run grep -q "check:" "$MOCK_LOG" followed by [ "$status" -ne 0 ] (or ! grep -q ... without run).
|
I have to think about if this is the right way to go forward (I do like to get rid of the big super linter docker image though):
|
Bats test files use bash syntax and benefit from shellcheck.
## Summary - Add `*.bats` to the shellcheck file pattern in `super-linter.sh` native mode ## Why Bats test files use bash syntax and shellcheck catches real issues in them (SC2034 unused variables, SC2314 bats-specific `!` usage, etc.). Docker super-linter already lints `.bats` files; native mode was missing the glob pattern, creating a gap where CI catches issues that local `native-lint` misses. Discovered while fixing shellcheck errors in [grafana/docker-otel-lgtm#1156](grafana/docker-otel-lgtm#1156). The same fix is applied to `run-linters.sh` in #136. ## Test plan - [ ] `mise run test` passes
|
close in favor of #139 |
Will likely close in favor of #139
Summary
tasks/lint/run-linters.sh: a lightweight alternative to the super-linter native mode that runs native linters with changed-file detection, without requiring the super-linter env file or VALIDATE_* flag machineryrun-linters prettier markdownlint shfmtRUN_LINTERS_TOOLSenv var as an alternative to positional args (needed for misefile =tasks called from adependslist)RUN_LINTERS_EXTRA_REGISTRYenv var for injecting additional registry entries (used by tests)--autofixalways uses the fix command when one is defined (no per-linter FIX_* opt-in)--fullto lint all files instead of only changed files{MERGE_BASE}placeholder support, replacing the hardcoded golangci-lint special case in both scriptsrun-linters.shandsuper-linter.sh--full,--autofix, linter failure propagation, unknown linter, missing tool binary, no matching filesChanges to
super-linter.sh--new-from-revhandling via{MERGE_BASE}placeholder (works for any future tool too)_CACHED_FILES(same fix asrun-linters.sh)Test plan
mise run testpasses