Skip to content

PMM-7 add a Groovy lint gate for the pmm/ pipelines - #4366

Open
travagliad wants to merge 2 commits into
masterfrom
claude/groovy-lint-gate-gjfudu
Open

PMM-7 add a Groovy lint gate for the pmm/ pipelines#4366
travagliad wants to merge 2 commits into
masterfrom
claude/groovy-lint-gate-gjfudu

Conversation

@travagliad

Copy link
Copy Markdown
Contributor

Adds a Groovy lint check for PMM's Jenkins pipelines, and clears the backlog it would otherwise start red on.

Scoped to pmm/ throughout. No other product directory is linted, gated or reported on, and neither is the repo-root vars/ shared library, since that one is loaded by every product's builds. The workflow file has to live in .github/workflows/ — GitHub's rule — so it is named for PMM and filtered to pmm/** twice: once by its paths: trigger, so it never appears on another product's PR, and once in the file list it builds, so a PR touching both pmm/ and another directory is still only judged on its pmm/ files.

Measured before deciding anything

npm-groovy-lint 18.0.0 (CodeNarc 3.7.0 / Groovy 3.0.9) over the 62 .groovy files under pmm/:

stock recommended tuned
violations 5,557 (7 error, 1,902 warning, 3,648 info) 384 (0 error, 93 warning, 291 info)
rules firing 54 29
files with ≥1 hit 62 of 62 49
files that fail the check 7 0

Every file offends under the stock ruleset, so a gate over all of pmm/ would be red on arrival and ignored within a week. Two things fix that:

  1. The gate only sees the diff. It resolves the changed pmm/ .groovy files with git diff --diff-filter=ACMR base.sha HEAD and lints just those. The rest of the backlog is paid down as files get touched, by whoever already has the file open.
  2. The ruleset is tuned to Jenkins DSL. pmm/.groovylintrc.json extends CodeNarc's own recommended-jenkinsfile profile and switches off the rules pipeline DSL structurally violates — repeated string literals (1,002 hits: pipelines repeat branch names, agent labels, credential ids), UnnecessaryGString (486), SpaceAroundOperator (667, from KEY=credentials('id') in environment blocks), Indentation (210, both 2- and 4-space styles are established here), ParameterName (268, Jenkins params are UPPER_CASE), and block-size rules. LineLength goes to 200 rather than off (661 hits → 82). Every rule that is off or promoted carries its reason and its measured count in a comment, so this is arguable rather than mysterious.

Only error severity fails a check; warning and info are advisory. Errors are annotated on the diff, everything else goes to the step log and the job summary.

Second commit: the 7 dead assignments

So the gate starts clean instead of blocking the next person to touch one of these files. Each was checked against its file first, and nothing that has an effect was removed:

  • openshift_cluster_destroyresourceCount is parsed out of DESTROY_RESULT and never used; the build description beside it reads ocpVersion only.
  • pmm3-package-testing-{amd64,arm64}curl -s ifconfig.me into a PUBLIC_IP that is never read. The block below uses VM_IP throughout, including the echo on the very next line.
  • pmm3-release-candidate — keeps the build job: 'pmm3-rewind-submodules-fb' call, drops only the unused binding it was assigned to.
  • pmm3-upgrade-ami-test-runner, pmm3-upgrade-ami-tests, pmm3-upgrade-test-runner — pure list expressions assigned and never read.

Net: 6 discarded values and one wasted network call. No behaviour changes.

One rule that measurement talked me out of

UnnecessarySelfAssignment is off, not promoted to error. Its two hits are env.CLUSTER_NAME = env.CLUSTER_NAME in pmm3-ha-eks and pmm3-ha-rosa, which is the documented way to persist an environment{} var onto the build so build job: callers can read it back out of buildVariables — deliberate, and commented as such in both files. CodeNarc has no way to know that.

Notes for review

  • No file under pmm/ fails to parse.
  • --noserver is deliberate: the default CodeNarc daemon mode times out after 120s on a long file list and returns nothing at all.
  • The linter version is pinned exactly (npm-groovy-lint@18.0.0) and actions are pinned to commit SHAs, matching ppg-ami-factory.yml.
  • A manual workflow_dispatch with scope=all re-measures the whole pmm/ backlog without gating anything.
  • Also adds pmm/AGENTS.md (+ a short CLAUDE.md pointer), which opens with the scope rule — stay inside pmm/, use root vars/ but do not change it, pmm/v3/vars/ is PMM's own library — and covers how pmm-qa and pmm-submodules connect to pmm/v3/.

Verified locally end to end (lint → annotations → job summary → exit code) on the full 62-file set and on single-file runs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XcEM8jZrmdWWDJH65wxA2D


Generated by Claude Code

claude added 2 commits August 21, 2026 22:31
Scoped to pmm/ throughout. No other product directory is linted, gated or
reported on, root vars/ included.

Measured first: npm-groovy-lint 18.0.0 over the 62 .groovy files under pmm/
with the stock "recommended" ruleset reports 5,557 violations (7 error, 1,902
warning, 3,648 info) across 54 rules, with every file offending. A gate over
all of pmm/ is red on arrival, so the workflow diffs the PR against its base
and lints only the pmm/ .groovy files the PR adds or changes.

pmm/.groovylintrc.json extends CodeNarc's recommended-jenkinsfile profile and
turns off the rules Jenkins pipeline DSL structurally violates (repeated string
literals, GString quoting, indentation, UPPER_CASE parameter names, block size),
raises LineLength to 200, and promotes a short list of real defects to error.
That brings the pmm/ backlog to 384 violations, all advisory. No file under
pmm/ fails to parse.

UnnecessarySelfAssignment is off rather than promoted: `env.X = env.X` is the
documented way to persist an environment{} var onto the build so that
`build job:` callers can read it back out of buildVariables, and pmm3-ha-eks
and pmm3-ha-rosa both rely on it.

Only error severity fails a PR; warning and info are advisory. Errors are
annotated on the diff, the rest goes to the step log and job summary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcEM8jZrmdWWDJH65wxA2D
Every UnusedVariable the new gate reports under pmm/, so it starts clean
instead of blocking the next person to touch one of these files. Each was
checked against its file first; nothing that has an effect was removed.

- openshift_cluster_destroy: resourceCount is parsed out of DESTROY_RESULT and
  never used - the build description next to it reads ocpVersion only.
- pmm3-package-testing-{amd64,arm64}: `curl -s ifconfig.me` into a PUBLIC_IP
  that is never read; the block below uses VM_IP throughout, including the echo
  on the following line.
- pmm3-release-candidate: keeps the `build job: 'pmm3-rewind-submodules-fb'`
  call, drops only the unused binding it was assigned to.
- pmm3-upgrade-ami-test-runner, pmm3-upgrade-ami-tests, pmm3-upgrade-test-runner:
  pure list expressions assigned and never read.

No behaviour changes: 6 discarded values and one wasted network call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcEM8jZrmdWWDJH65wxA2D
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