Skip to content

chore(lint): drop the last fieldalignment nolints and reorder their structs - #643

Merged
IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-610
Oct 26, 2026
Merged

IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-610

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

#100 turned on fieldalignment in #589, #590, #593 and #596. A few structs sat in files that open PRs were also changing, so instead of being reordered they got a //nolint:govet // fieldalignment: file owned by open PR #N marker. Those PRs have all merged now: #441 at 06:41Z, #471 at 06:33Z and #491 at 06:51Z on 2026-10-26.

Struct File Pointer bytes
AllocationInput pkg/ladder/allocate.go 328 -> 264
BufferReshapeConfig pkg/ladder/capability.go 48 -> 24
DuplicateChecker pkg/recfilter/dedupe.go 24 -> 16
Client providers/azure/services/savingsplans/client.go 112 -> 104

A blanket //nolint:govet turned off every govet analyzer on these structs, not only fieldalignment. Now that it is gone, those analyzers check the structs again, and they report nothing.

Only field order changes. Is anything depending on the old order?

  • Inside this repo, go vet (which checks for unkeyed composite literals) passes in all five modules, both with and without the integration tag.
  • savingsplans.Client has only unexported fields, so no other package can build one with a struct literal.
  • In local checkouts of the platform, CLI and MCP repos, a search found no unkeyed literals of AllocationInput, BufferReshapeConfig or DuplicateChecker.

Closes #610

Verification

Unit, lint and build checks on this branch:

  • Fails before the fix. With only the four markers removed and the fields in their old order, golangci-lint v2.10.1 run with --allow-parallel-runners reports:
    pkg/ladder/allocate.go:68:22: fieldalignment: struct with 328 pointer bytes could be 264 (govet)
    pkg/ladder/capability.go:90:26: fieldalignment: struct with 48 pointer bytes could be 24 (govet)
    pkg/recfilter/dedupe.go:30:23: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
    providers/azure/services/savingsplans/client.go:65:13: fieldalignment: struct with 112 pointer bytes could be 104 (govet)
    
  • Passes after the fix. After the reorder, golangci-lint run --allow-parallel-runners with the repo's own .golangci.yml reports 0 issues. across the whole module, run from both pkg/ and providers/azure/. The config already sets build-tags: integration.
  • No markers left. Both grep -rn "file owned by open PR" --include=*.go . and grep -rln "nolint:govet" --include=*.go . return nothing.
  • Build and vet. go build ./..., go vet ./... and go vet -tags integration ./... pass in each of pkg, providers/aws, providers/azure, providers/gcp and ci_cd_sanity_tests.
  • Unit tests. All pass:
    • (cd pkg && go test -count=1 ./ladder/)
    • (cd pkg && go test -count=1 ./recfilter/)
    • (cd providers/azure && go test -count=1 ./services/savingsplans/)
  • Cyclomatic complexity. gocyclo -over 10 -ignore "_test\.go" on the four changed files reports nothing.

Parent verification at 7580943 (branch is up to date with main): go test -race -short -count=1 ./... in the pkg and providers/azure modules passes. This is the only branch for this repo in this batch, so there was no combined tree.

Out of scope

  • If you run the standalone fieldalignment analyzer, it also flags some structs in _test.go files under pkg/recfilter. The repo's golangci-lint config does not report these, so they are left alone.

Maintainer: #610 currently has no triage labels, so there are none to mirror onto this PR. Please add any you want.

🤖 Generated with Claude Code

…tructs

LeanerCloud#441, LeanerCloud#471 and LeanerCloud#491 have merged, so the structs that kept a
"//nolint:govet // fieldalignment: file owned by open PR #N" marker to
avoid conflicting with them can now be reordered. LeanerCloud#471 already removed
the gcp markers; this removes the remaining four and reorders the
structs into fieldalignment's optimal order, keeping every field comment:

- pkg/ladder: AllocationInput (328 -> 264 pointer bytes) and
  BufferReshapeConfig (48 -> 24)
- pkg/recfilter: DuplicateChecker (24 -> 16)
- providers/azure/services/savingsplans: Client (112 -> 104)

The blanket //nolint:govet also hid every other govet analyzer on those
structs; with it gone they are checked again (and are clean).

Closes LeanerCloud#610

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7580943ef53aba1e80e0a2955557aeae5f8eb31e

Blocking:

  • none

Non-blocking:

  • pkg/recfilter/dedupe.go:39 and providers/azure/services/savingsplans/client.go:74: the reorder moves the plain config/identity fields (LookbackHours, subscriptionID/region) below the test seams. That's a little harder to read, but it's what fieldalignment asks for and all the comments stay with their fields. No change needed.

Attacked:

  • Fail-before: put the four files back to their origin/main field order with only the //nolint:govet markers stripped. golangci-lint v2.10.1 then reports exactly the four fieldalignment findings the body quotes (328->264, 48->24, 24->16, 112->104). With the PR's order it reports 0 issues. Restored afterwards.
  • Field-order breakage: grepped platform, cli, mcp and go for every literal of AllocationInput, BufferReshapeConfig, DuplicateChecker and savingsplans.Client. All of them, multi-line or single-line, are keyed, and the one platform production use (handler_ladder.go:377) is keyed. None of these structs has json tags or goes through a marshal/hash/reflect path in their packages, so field order can't change any serialized form or idempotency key. BufferReshapeConfig is money-adjacent (exchange payment caps): only its order changed, and the cap fields and their "nil is an error" docs are intact.
  • Issue chore(lint): remove fieldalignment nolints once #441, #471 and #491 land #610 "done when": grep -rn "file owned by open PR\|nolint:govet" --include=*.go . finds nothing at head, so the gcp markers are gone too (removed by fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471).
  • Ran: go build ./..., go vet ./..., go vet -tags integration ./... in all five modules (pkg, providers/{aws,azure,gcp}, ci_cd_sanity_tests); go test -race -short -count=1 on pkg/ladder, pkg/recfilter, providers/azure/services/savingsplans and the downstream providers/aws/ladder (all ok); golangci-lint with and without --build-tags integration on the changed packages (0 issues); gocyclo -over 10 on the four files (clean). The changed packages have no integration-tagged tests.

Body-stale:

  • none

Reviewer: rF2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 7580943ef53aba1e80e0a2955557aeae5f8eb31e (main merged in; the PR's diff is unchanged, so earlier reviews carry over; every module builds, vets and passes go test -race). CI Success and pre-commit green. Merged by IAmJSD's agent at IAmJSD's request (acting owner while the maintainers are away), under the bar IAmJSD set: one approving independent review (rF2) on the head. Squash-merged at exactly 7580943ef53aba1e80e0a2955557aeae5f8eb31e.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 20:14
@IAmJSD
IAmJSD merged commit 5832488 into LeanerCloud:main Oct 26, 2026
15 checks passed
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.

chore(lint): remove fieldalignment nolints once #441, #471 and #491 land

1 participant