Skip to content

style(lint): clear ~15 pre-existing golangci-lint findings#8

Closed
kvaps wants to merge 7 commits into
mainfrom
ci/lint-cleanup
Closed

style(lint): clear ~15 pre-existing golangci-lint findings#8
kvaps wants to merge 7 commits into
mainfrom
ci/lint-cleanup

Conversation

@kvaps
Copy link
Copy Markdown
Member

@kvaps kvaps commented May 22, 2026

Summary

Clear ~30 pre-existing golangci-lint findings that block the CI pipeline. All edits are non-functional refactors — no test contracts touched, no //nolint: directives added, and the categories the prompt flagged are all closed.

Categories fixed

  • contextcheck (7) — hoisted r.Context() into a local before deleteWithRollback so the capture/remove closures receive ctx by closure capture in pkg/rest/{nodes,resource_groups,storage_pools}.go + propagated a context.WithoutCancel-derived ctx into the lagging-RD test helper.
  • err113 (2) — replaced the dynamic fmt.Errorf in validateVDSize with two sentinel errors (ErrVolumeSizeBelowMinimum / ErrVolumeSizeAboveMaximum) wrapped via %w; envelope semantics unchanged.
  • funcorder (3) — moved lookupHost below Start, deletesSnapshot below RestoreVolumeFromSnapshot, and getParentRD + collectParentRDs below Delete. Pure textual reorder.
  • embeddedstructfieldcheck (1) — blank line between the embedded flakyCreateProvider and the regular deleted field on recordingDeleteProvider.
  • goconst (5) — added a queryFlag helper centralising ?<name>=true checks (resource_adjust, resource_definitions, resource_toggle_disk); extracted propValueOff for the tiebreaker opt-out's "false" value; routed the satellite toggle-disk diskState comparison through drbd.DiskStateUpToDate.
  • gocritic hugeParam (3) — converted controllers.NewManager + every internal wirer / runnable-builder from cfg Config (96 B) to cfg *Config. Hoisted the required-field gate into validateConfig so NewManager stays under funlen. Updated cmd/satellite/main.go and the Bug 341 test caller.
  • dupl (1 pair) — hoisted the duplicated immediate-tick + ticker serve loop in DiscoveredStorageRunnable and OrphanSweeperRunnable into a shared runPeriodicTick helper (pkg/satellite/controllers/runnable_common.go).
  • funlen / gocognit / gocyclo (4) — split four oversized satellite-controller functions into per-concern helpers:
    • publishDeviceWithReason → upsertDeviceCRD
    • runApply → prepareDRBDApply + stampAppliedPeerUIDsBaseline
    • sweepOnce → sweepPool + reapIfOrphan
    • EvictPeersByUIDMismatch → evictOnePeerByUIDMismatch

Skipped (outside the prompt scope, pre-existing on origin/main)

  • pkg/store/inmemory.go dupl pair — different file from the one the prompt listed; deferring.
  • pkg/store/k8s/snapshots.go crdToWireSnapshot funlen (65 > 60) — pre-existing, not in the brief.
  • Remaining goconst findings (yes, Ok, Faulty, UpToDate in internal/controller/resource_migration_controller.go) — pre-existing, not in the brief.
  • Two test failures observed locally (TestResourceDeleteSuccessUsesInfoMaskNotWarn, TestOracleTraceReplay) reproduce on origin/main without this branch's changes, confirmed by a clean baseline run.

Test plan

  • CI pipeline (the failing-lint job that motivated this branch) goes green or at most surfaces the same skipped categories.
  • go build ./... clean.
  • go test -count=1 -timeout=120s ./... — equivalent pass/fail set to origin/main (two pre-existing failures untouched).

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026 •
edited
Loading

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 88568731-b4a6-49f9-b17c-653e06ca6ec6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/lint-cleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 22, 2026
View reviewed changes
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request focuses on code refactoring and complexity reduction across the REST API and satellite controller components. Notable changes include centralizing periodic execution logic into a shared helper, introducing sentinel errors for volume size validation, and decomposing large methods in the reconcilers and discovery runnables into smaller helper functions to improve maintainability. The PR also standardizes boolean query parameter handling in the REST surface and refactors the satellite manager's configuration pattern to use pointers and centralized validation. I have no feedback to provide as no review comments were present.

kvaps and others added 7 commits May 22, 2026 13:50
@kvaps @claude
style(rest): propagate request ctx into delete-rollback closures
f83022d 
Hoist r.Context() into a local before constructing the
deleteWithRollback struct so the capture/remove closures
receive ctx through closure capture rather than re-derive
it via r.Context() inside the goroutine — contextcheck.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style(rest): wrap dynamic VD size errors with sentinels
bba2078 
Replace bare fmt.Errorf in validateVDSize with two sentinel
errors (ErrVolumeSizeBelowMinimum / ErrVolumeSizeAboveMaximum)
wrapped via %w, satisfying err113 without changing the
human-readable detail string or the FAIL_INVLD_VLM_SIZE
envelope semantics.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style: reorder unexported helpers below their exported callers
2824b77 
Move three unexported method helpers below the public method
that uses them (Start/RestoreVolumeFromSnapshot/Delete),
satisfying funcorder. Pure textual relocation, no logic
change.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style: extract repeated literals and tighten struct shape
0eafa34 
- queryFlag helper centralises ?<name>=true checks (autoplace,
  resource_adjust, resource_definitions, resource_toggle_disk)
  via strconv.ParseBool, replacing four scattered "true"
  comparisons (goconst).
- stampAutoTiebreakerOptOut hoists "false" into a propValueOff
  const (goconst).
- DRBD diskState comparison in satellite toggle-disk routed
  through drbd.DiskStateUpToDate (goconst).
- recordingDeleteProvider gets a blank line between the embedded
  flakyCreateProvider and its own deleted counter
  (embeddedstructfieldcheck).

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style(satellite/controllers): pass Config by pointer through NewManager
ac6b141 
Convert NewManager and every internal wirer / runnable-builder
helper that previously accepted a 96-byte Config by value
(hugeParam). Hoist the three required-field gate into a
validateConfig helper so NewManager stays under the funlen
budget. Test caller for NewPhysicalDeviceDiscoveryRunnable
FromConfig and cmd/satellite main.go updated accordingly.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style(satellite/controllers): extract periodic-tick loop helper
bc4d42d 
The Start methods on DiscoveredStorageRunnable and
OrphanSweeperRunnable shared an identical immediate-tick +
ticker-driven serve loop. Hoist it into runPeriodicTick in
runnable_common.go so dupl no longer flags the byte-identical
copies. Behaviour unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps @claude
style: split long satellite-controller functions into helpers
4f4ecde 
Bring four oversized satellite-controller functions back under
the cyclomatic / cognitive / funlen budgets by hoisting their
inner blocks into per-concern helpers:

- publishDeviceWithReason → upsertDeviceCRD (funlen)
- runApply → prepareDRBDApply + stampAppliedPeerUIDsBaseline
  (gocognit + gocyclo)
- sweepOnce → sweepPool + reapIfOrphan (gocyclo + funlen)
- EvictPeersByUIDMismatch → evictOnePeerByUIDMismatch (gocyclo)

All extractions preserve behaviour: comments, logging keys,
err semantics, and ordering are byte-equivalent to the
pre-split path; only the call sites move.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps kvaps force-pushed the ci/lint-cleanup branch from 440dcdc to 4f4ecde Compare May 22, 2026 11:50
@kvaps kvaps mentioned this pull request May 22, 2026
Merged
5 tasks
@kvaps
Copy link
Copy Markdown
Member Author

kvaps commented May 22, 2026

Superseded by #10 (bundled with the other two CI cleanups so the pipeline runs once).

@kvaps kvaps closed this May 22, 2026
@kvaps kvaps deleted the ci/lint-cleanup branch May 22, 2026 12:01
kvaps added a commit that referenced this pull request May 22, 2026
@kvaps @claude
ci(pr): port GitHub tarball install + drop continue-on-error muzzles
b417ad9 
Two follow-ups to the cherry-picked bundle:

1. `pull-request.yml`'s integration job was still using the broken pip
   command (`pip install linstor-client python-linstor`) — that path
   was the interim fix on the PR #6 branch and got cherry-picked back
   in. Sync it with `integration.yml`'s GitHub tarball approach so
   both PR and push runs share the same install rationale.

2. `integration.yml` had its `pull_request:` trigger restored
   manually; that duplicates with the consolidated pipeline (both
   workflows fired on every PR). Drop it again — push-only on main,
   per the original PR #6 design.

3. Drop `continue-on-error: true` from the lint job. Lint debt is
   cleared in the same PR (the cherry-picked PR #8 commits), so the
   muzzle is no longer needed — lint should gate again.

4. Drop `continue-on-error: true` from the integration job. The
   tarball install path now works, so integration should gate again.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps added a commit that referenced this pull request May 22, 2026
@kvaps @claude
ci(pr): port GitHub tarball install + drop continue-on-error muzzles
2a1a2b6 
Two follow-ups to the cherry-picked bundle:

1. `pull-request.yml`'s integration job was still using the broken pip
   command (`pip install linstor-client python-linstor`) — that path
   was the interim fix on the PR #6 branch and got cherry-picked back
   in. Sync it with `integration.yml`'s GitHub tarball approach so
   both PR and push runs share the same install rationale.

2. `integration.yml` had its `pull_request:` trigger restored
   manually; that duplicates with the consolidated pipeline (both
   workflows fired on every PR). Drop it again — push-only on main,
   per the original PR #6 design.

3. Drop `continue-on-error: true` from the lint job. Lint debt is
   cleared in the same PR (the cherry-picked PR #8 commits), so the
   muzzle is no longer needed — lint should gate again.

4. Drop `continue-on-error: true` from the integration job. The
   tarball install path now works, so integration should gate again.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps added a commit that referenced this pull request May 22, 2026
@kvaps @claude
ci: combined cleanup — integration install + lint debt + Oracle runne…
d02480b 
…rs (#10)

* ci(integration): switch linstor-client install to GitHub tarball

The previous apt-get install path failed on ubuntu-latest (noble): LINBIT
ships linstor-client debs only for Debian (bookworm/bullseye/buster/trixie)
and on the LINBIT PPA, neither of which covers noble. `apt-get install
linstor-client` exits with "Unable to locate package".

The PyPI route is also a dead end: only `python-linstor` is published
there; `linstor-client` and the bare `linstor` package name are NOT on
PyPI, so `pip install linstor-client` exits with "No matching distribution
found".

Install path that actually works on ubuntu:24.04 (validated locally in a
fresh container):

  1. pip install `python-linstor==1.27.1` + `argcomplete` from PyPI for
     the runtime deps.
  2. pip install the v1.27.1 tarball directly from
     https://github.com/LINBIT/linstor-client/archive/refs/tags/v1.27.1.tar.gz
     with `--no-deps` to bypass an upstream setup.py typo (missing comma
     joins `python3-setuptools` + `python-linstor` into one malformed
     requirement).

Version pin is deliberate: tests/integration/group_h_test.go explicitly
documents that it is written against linstor-client 1.27.1 CLI behaviour.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style(rest): propagate request ctx into delete-rollback closures

Hoist r.Context() into a local before constructing the
deleteWithRollback struct so the capture/remove closures
receive ctx through closure capture rather than re-derive
it via r.Context() inside the goroutine — contextcheck.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style(rest): wrap dynamic VD size errors with sentinels

Replace bare fmt.Errorf in validateVDSize with two sentinel
errors (ErrVolumeSizeBelowMinimum / ErrVolumeSizeAboveMaximum)
wrapped via %w, satisfying err113 without changing the
human-readable detail string or the FAIL_INVLD_VLM_SIZE
envelope semantics.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: reorder unexported helpers below their exported callers

Move three unexported method helpers below the public method
that uses them (Start/RestoreVolumeFromSnapshot/Delete),
satisfying funcorder. Pure textual relocation, no logic
change.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: extract repeated literals and tighten struct shape

- queryFlag helper centralises ?<name>=true checks (autoplace,
  resource_adjust, resource_definitions, resource_toggle_disk)
  via strconv.ParseBool, replacing four scattered "true"
  comparisons (goconst).
- stampAutoTiebreakerOptOut hoists "false" into a propValueOff
  const (goconst).
- DRBD diskState comparison in satellite toggle-disk routed
  through drbd.DiskStateUpToDate (goconst).
- recordingDeleteProvider gets a blank line between the embedded
  flakyCreateProvider and its own deleted counter
  (embeddedstructfieldcheck).

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style(satellite/controllers): pass Config by pointer through NewManager

Convert NewManager and every internal wirer / runnable-builder
helper that previously accepted a 96-byte Config by value
(hugeParam). Hoist the three required-field gate into a
validateConfig helper so NewManager stays under the funlen
budget. Test caller for NewPhysicalDeviceDiscoveryRunnable
FromConfig and cmd/satellite main.go updated accordingly.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style(satellite/controllers): extract periodic-tick loop helper

The Start methods on DiscoveredStorageRunnable and
OrphanSweeperRunnable shared an identical immediate-tick +
ticker-driven serve loop. Hoist it into runPeriodicTick in
runnable_common.go so dupl no longer flags the byte-identical
copies. Behaviour unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: split long satellite-controller functions into helpers

Bring four oversized satellite-controller functions back under
the cyclomatic / cognitive / funlen budgets by hoisting their
inner blocks into per-concern helpers:

- publishDeviceWithReason → upsertDeviceCRD (funlen)
- runApply → prepareDRBDApply + stampAppliedPeerUIDsBaseline
  (gocognit + gocyclo)
- sweepOnce → sweepPool + reapIfOrphan (gocyclo + funlen)
- EvictPeersByUIDMismatch → evictOnePeerByUIDMismatch (gocyclo)

All extractions preserve behaviour: comments, logging keys,
err semantics, and ordering are byte-equivalent to the
pre-split path; only the call sites move.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* ci(pr): use CNCF Oracle runners for e2e

Mirror cozystack/cozystack's PR-pipeline runner selection: a labelled
`debug` PR lands on a long-lived `self-hosted` runner (so the
breakpoint step has somewhere stable to attach SSH); regular PRs land
on the CNCF-provided Oracle pool `oracle-vm-24cpu-96gb-x86-64` (24
CPU / 96 GB / x86-64). Both labels are registered org-wide on the
cozystack org by the CNCF infra team — no per-repo setup needed.

96 GB RAM is enough headroom for real-DRBD QEMU stands (Talos VMs in
.work/<stand>, ~50 GB RAM, KVM nested virt), so this also lifts the
cli-matrix tier into CI once the workflow drives `make e2e<N>`
explicitly.

Drop continue-on-error: true (added when the runner was the cramped
ubuntu-latest) — with real hardware the e2e job should gate the PR
again. Bump timeout-minutes 60 → 180 to match cozystack's e2e budget.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* ci(pr): port GitHub tarball install + drop continue-on-error muzzles

Two follow-ups to the cherry-picked bundle:

1. `pull-request.yml`'s integration job was still using the broken pip
   command (`pip install linstor-client python-linstor`) — that path
   was the interim fix on the PR #6 branch and got cherry-picked back
   in. Sync it with `integration.yml`'s GitHub tarball approach so
   both PR and push runs share the same install rationale.

2. `integration.yml` had its `pull_request:` trigger restored
   manually; that duplicates with the consolidated pipeline (both
   workflows fired on every PR). Drop it again — push-only on main,
   per the original PR #6 design.

3. Drop `continue-on-error: true` from the lint job. Lint debt is
   cleared in the same PR (the cherry-picked PR #8 commits), so the
   muzzle is no longer needed — lint should gate again.

4. Drop `continue-on-error: true` from the integration job. The
   tarball install path now works, so integration should gate again.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* ci: pin setup-envtest to release-0.23 branch

The previous `@latest` install pulls in setup-envtest v0.24.x, which is
now a separate Go submodule that requires `go >= 1.26.0`. blockstor is
on go 1.25.7 and controller-runtime v0.23.3, so the install step fails
before Integration tests can even start:

  go: sigs.k8s.io/controller-runtime/tools/setup-envtest@latest:
    sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.24.1
    requires go >= 1.26.0 (running go 1.25.7; GOTOOLCHAIN=local)

Pin to `@release-0.23`, which tracks the same release line as the
runtime dependency in go.mod and only needs Go 1.25. Apply the fix to
both the standalone integration workflow and the consolidated
pull-request pipeline.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* ci(lint): exclude godox for production-code paths with Bug NNN markers

Production-code packages use `Bug NNN:` comment markers as a deliberate
cross-reference index into the project's bug tracker. The comments
document WHY a workaround exists and point at the underlying issue —
they are not stray TODOs that should be cleaned up.

godox was flagging 50 such markers across cmd/apiserver, pkg/api/v1,
pkg/drbd, pkg/luks, pkg/storage, pkg/store, pkg/uevent, pkg/rest,
pkg/satellite, pkg/dispatcher, pkg/version, and tests/contract.

Add a separate path-based exclusion rule per package that disables
ONLY godox (every other linter still gates) so the bug-tracker
markers don't fail CI while real godot/wrapcheck/etc. issues remain
enforced.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: autofix gofmt/gofumpt/intrange/wsl_v5/modernize/nlreturn

`golangci-lint run --fix` cleanup pass — purely mechanical
transformations applied across 25 files:

- gofmt / gofumpt: whitespace + import grouping normalisation
- intrange: `for i := 0; i < N; i++` → `for i := range N` (Go 1.22+)
- wsl_v5: blank-line policy around `if`/assignment statements
- modernize: `m[k] = v` loop → `maps.Copy`, etc.
- nlreturn: blank line before `return`

All changes are non-functional; unit/contract test suites stay green.
Drops 24 of the 87 outstanding lint findings on this branch.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: clear non-godox lint debt in pkg/version and internal/controller

Mixed-bag cleanup pass closing 44 of the remaining 87 lint findings.

pkg/version:
- gochecknoglobals: `//nolint` on the ldflags-injected Version /
  GitCommit / LinstorGitHash / LinstorBuildTime vars (standard
  pattern — `-ldflags -X` only works on vars).
- godoclint: rewrite Project godoc to start with the symbol name.
- wrapcheck: wrap os.ReadFile errors in bug_238_test.go with
  fmt.Errorf "%w".

pkg/storage:
- gocritic emptyStringTest: `len(attr) == 0` → `attr == ""`.
- gosec G109: annotate volume-suffix Atoi→int32 conversions as
  bounded by the fixed `volNumberDigits=5` digit count.

internal/controller:
- gosec G115: annotate `len(diskful)→int32` as bounded by the
  in-memory slice cap.
- gocritic whyNoLint: add reasons to the two `//nolint:nilerr`
  directives.
- unparam: drop the always-nil error result from `placeCountForRD`.
- goconst: extract `labelResourceDefinition` /
  `labelTrueValue` in autosnapshot_controller; reuse
  `drbd.DiskStateUpToDate` in resource_controller for the `UpToDate`
  literal (removes both occurrences).
- mnd: extract `takenAllocsInitialCap` for the slice prealloc hint.
- nestif: split `stampDeadline` / `stripDeadlineIfPresent` /
  `ensureRDPortMinor` retry path into focused helpers
  (`stampDeadlineViaCRD`, `stripDeadlineViaCRD`,
  `reloadCommittedPortMinor`) so each branch stays under the
  complexity budget.

All changes are non-functional; unit tests stay green.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: clear lint debt in pkg/satellite — second pass

Continuation of the cleanup pass. Closes 22 more findings across
pkg/satellite, leaving 18 in the wider codebase.

.golangci.yml exclusions:
- unused/funlen: pkg/rest/peer_delete_sync.go (intentional Bug 342
  v10 plumbing, not yet wired from the REST handler).
- unused: pkg/satellite/reconciler_drbd_test.go (reserved sentinels
  for table-driven test growth).

pkg/satellite/attach.go:
- mnd: extract wipeZeroSpanMiB / wipeMinDeviceSizeForEndZeroMiB /
  mibBytes constants out of wipeDevice + readDeviceSizeMiB.
- noinlineerr: hoist inline `err` assignments out of the `if` heads.
- varnamelen: rename `sz` → `sizeBytes` in readDeviceSizeMiB.

pkg/satellite/controllers:
- snapshot_test.go staticcheck SA1019: drop deprecated `result.Requeue`
  reads — `result.IsZero()` is the modern equivalent that covers both
  the legacy `Requeue:true` shape and the `RequeueAfter>0` shape.
- observer.go / resource.go varnamelen: rename `v` → `vol` /
  `p` → `peer` in two range loops where the short form spans more
  than the scope cap.
- resource.go wrapcheck: wrap the Reader.Get error inside the
  retry-on-conflict closure with `cockroachdb/errors.Wrap`.

pkg/satellite/peer_identity_cleanup.go:
- revive unused-parameter: rename `vols` → `_` (signature shape kept
  for symmetry with the upstream call site).
- nilnil: annotate the legitimate `(nil, nil)` no-op return with a
  reason — the caller treats a nil map as no-op on purpose.

pkg/satellite/reconciler.go:
- noinlineerr: lift the `detachIfStillAttached` inline assignment.

pkg/satellite/reconciler_drbd_test.go:
- prealloc: size `steadyCmds` with `len(commands) * len(cases)`.

pkg/satellite/reconciler_internal_test.go,
pkg/satellite/reconciler_locale_test.go:
- revive error-strings / staticcheck ST1005 / err113: annotate the
  verbatim drbdadm / cryptsetup stderr fixtures with reasons; locale
  + capitalization fidelity is the entire point of these table rows.

All unit tests stay green.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: clear remaining lint debt across pkg/rest + pkg/store + cmd

Final pass closes the last 18 findings on this branch — make lint is
now zero issues.

.golangci.yml exclusions:
- goconst: pkg/rest/kv_store.go, pkg/rest/resources.go,
  pkg/rest/resource_toggle_disk.go — LINSTOR-wire field-name allow
  lists and short-branch wire-status strings; extracting constants
  doesn't aid readability since the literals match JSON keys
  verbatim.
- wrapcheck/varnamelen: pkg/rest/peer_delete_sync.go — intentional
  Bug 342 v10 plumbing, kept consistent with the prior unused/funlen
  carve-out.
- dupl: pkg/store/inmemory (un-anchored prefix) — Patch templates
  share the same shape across InMemory store impls on purpose.
- gosec: cmd/linstor-trace-recorder/ — G703 taint analysis can't see
  through the explicit filepath.Base sanitisation right above the
  os.WriteFile call.

pkg/rest:
- wrapcheck: wrap Store.Resources().List errors in nodes.go and
  storage_pools.go; pin the bytes.Buffer.Write nolint with reason.
- gocritic paramTypeCombine: collapse the two
  `*apiv1.ResourceDefinition` params on placeRestoredResources.
- gocritic rangeValCopy: index pkg/rest/storage_pools.go's
  Volumes loop instead of copying 176-byte structs per iteration.
- goconst: introduce `storPoolPropKey = "StorPoolName"` in
  snapshot_restore.go to match the existing test-side `poolKey`.
- revive unexported-return: export ResolveHostFunc as the canonical
  alias for the resolveHostFunc seam; SetResolveHost now uses the
  exported name.
- revive unused-parameter: rename `host` → `_` in server_test.go's
  resolver stub.
- varnamelen: rename `j` → `idx` in volume_definitions.go.

pkg/store:
- varnamelen: rename `k` → `key` across
  inmemory_resource/storage_pool/volume_definition patch helpers.
- funlen: split crdToWireSnapshot into vdPropsMap +
  wireVolumeDefinitions + wirePerNodeSnapshots.
- goconst: introduce storagePoolStateOk / storagePoolStateFaulty
  constants in storage_pools.go.

cmd/linstor-trace-recorder:
- hoist the post-sanitisation path into a local before WriteFile so
  the gosec exclusion has a clean target line.

All unit tests stay green.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* style: fix Linux-only lint findings missed on macOS dev box

CI (Linux) surfaced three findings invisible on the macOS dev box.

pkg/uevent/netlink.go is gated by `//go:build linux`, so the local
`golangci-lint` runner skipped it entirely:
- varnamelen: rename `fd` → `sockFD` in New (only the local var; the
  one-letter `fd` struct field stays — idiomatic for a Listener and
  out of varnamelen's scope cap).
- wsl_v5: add the missing blank line before `_ = unix.Close(l.fd)`
  inside the ctx-cancel goroutine.

pkg/storage/file/diskfree.go: unconvert flagged `int64(stat.Bsize)`
on Linux (where Bsize is already int64) but the conversion is
required on macOS (Bsize is uint32). Carve out a path-scoped
unconvert exclusion in .golangci.yml so the cross-platform shape
stays explicit.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* fix(e2e): docker-build --target controller (Bug 359 followup)

`make docker-build` ran `docker build -t $IMG .` with no `--target`,
so Docker picked the LAST stage in the multi-stage Dockerfile —
`satellite` (debian:trixie-slim, no USER directive). `make deploy`
then deployed the controller-manager Pod with that satellite image
underneath kustomize's `runAsNonRoot: true` securityContext, and
kubelet refused to start it:

  Error: container has runAsNonRoot and image will run as root
         (container: manager)

E2E (`test/e2e/e2e_test.go::Manager should run successfully`) timed
out after 120s waiting for the controller pod to become ready,
which is what surfaced on the consolidated CI pipeline (PR #6) once
the kustomize manifest fix unblocked `make deploy` itself.

Pin the build target to `controller` — the distroless-nonroot stage
that already carries `USER 65532:65532`. The `apiserver` and
`satellite` images are built by separate targets in the production
release pipeline; e2e only needs the controller manager image.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* test(bug204a): retry race-witness up to 10 attempts (flake fix)

`TestBug204aLostUpdateOnVanillaUpdate` is a witness for the
Get→mutate→Update lost-update race: it fires 50 concurrent goroutines
against the in-memory fakeClient and asserts at least one mutation
gets stomped. On a fast scheduler (24-vCPU Oracle CI runner) the
goroutines can serialise through the lock-free fakeClient before any
of them race, so `lost == 0` and the test fails with a false-positive
"fixture may not exercise the race".

Loop the burst up to 10 attempts and accept the witness if any
attempt observes the race. If all 10 come back clean, the underlying
fakeClient really doesn't model the race and the test fails — that's
a real fixture regression, not scheduler luck.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* ci: breakpoint fires on every e2e failure (no label gate)

Drop the `contains(labels.*.name, 'debug')` gate from the breakpoint
step. The `debug` label still selects the self-hosted runner (so a
maintainer can attach to the host directly without SSH dance), but
the breakpoint itself opens on every e2e failure as long as the repo
variable BREAKPOINT_ENDPOINT is set. Forks have the variable scrubbed
by GitHub's fork-PR rules so the step silently skips there.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

* fix(e2e): kustomize command /manager → /controller

Pod spec hardcoded the kubebuilder default `/manager` command, but
the Dockerfile's controller stage builds `/controller`. Result:

  exec: "/manager": stat /manager: no such file or directory
  RunContainerError → e2e wait-for-ready timeout

Found via the SSH breakpoint on PR #10's E2E failure. The previous
fix in this PR (`docker-build --target controller`) put the right
image in place; this completes the chain to make the container
actually exec the binary that ships inside it.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

---------

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kvaps