Repository navigation
update go module dependencies - #334
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
{
"verdict": "approve",
"summary": "Dependency-only PR (CHANGELOG.md, go.mod, go.sum) with no source changes; the version bumps are internally consistent (no replace/exclude directives, no stale hashes for dropped modules, structured-merge-diff is v7-only, and nothing in the repo imports the dropped go-openapi/swag root, go-restful or json-iterator packages). Selector traceability: 11 candidate judgment rules, 5 applicable, 6 skipped; adjudication produced 3 non-blocking findings. The only substantive gaps are documentation/risk: the Unreleased bullet names 12 bumps while the go.mod diff touches 50 require lines and silently omits the k8s stack moving from stable v0.37.0 to prerelease v0.38.0-alpha.2, and go.sum carries one unpaired zip-only hash. Build/test correctness is delegated to CI (`make precommit`, .github/workflows/ci.yml:33), which this sandbox has no Go toolchain to run.",
"comments": [
{
"file": "CHANGELOG.md",
"line": 13,
"severity": "major",
"blocking": false,
"blocking_reason": "",
"message": "The Unreleased bullet names 12 dependency bumps, but the go.mod diff changes 50 require lines (62 removed, 50 added). It omits the most notable change in the PR: k8s.io/api, apiextensions-apiserver, apimachinery and client-go move from stable v0.37.0 to prerelease v0.38.0-alpha.2, plus sigs.k8s.io/structured-merge-diff v6.4.2 -> v7.0.0 (major-version module path change). The changelog guide asks dependency entries to include versions; a reader of the release notes would not learn the CLI's Kubernetes stack is now on an alpha. Please extend the bullet (or add a second `- chore:` bullet) covering the k8s alpha and structured-merge-diff v7 so the release notes match the diff."
},
{
"file": "go.mod",
"line": 87,
"severity": "major",
"blocking": false,
"blocking_reason": "",
"message": "k8s.io/api (and apiextensions-apiserver/apimachinery/client-go) jump from stable v0.37.0 to prerelease v0.38.0-alpha.2. Nothing in this repo imports these directly (all four are `// indirect`), so the alpha was almost certainly pulled in by an upstream module (bborbe/k8s v1.14.24 or strimzi) rather than chosen deliberately. Please confirm that provenance and record it in the changelog: an alpha k8s stack in a prod CLI receives no upstream security backports until v0.38.0 is stable. If the alpha is not MVS-forced, holding the four k8s modules at v0.37.x would be the safer pin. Non-blocking because CI builds and tests the resulting graph."
},
{
"file": "go.sum",
"line": 70,
"severity": "nit",
"blocking": false,
"blocking_reason": "",
"message": "`github.com/go-openapi/swag v0.27.1 h1:...` is an unpaired hash: it has a zip hash but no `/go.mod` hash, and no `require` line in go.mod references `github.com/go-openapi/swag` at all (the split submodules conv/jsonutils/pools/typeutils are what go.mod keeps, and those are all correctly paired). Every other module I spot-checked in the new graph (all four k8s alpha modules, bborbe/k8s v1.14.24, klauspost/compress v1.20.3, golang.org/x/crypto v0.58.0) has both lines. This shape is not what a clean `go mod tidy` emits and looks like residue from the `go mod tidy -e` in the Makefile `ensure` target, which tolerates errors. It is harmless to builds (extra go.sum entries are ignored, and the Makefile builds with `-mod=mod`), but run a clean `go mod tidy` without `-e` and commit the result so go.sum is a tidy-produced artifact."
}
],
"concerns_addressed": [
{
"concern": "correctness: k8s.io/* moves from stable v0.37.0 to prerelease v0.38.0-alpha.2; verify no alpha-only API changes leak into behavior and that this is intentional rather than an unconstrained go get -u",
"disposition": "addressed",
"detail": "Comment on go.mod:87. The four k8s modules are `// indirect` and nothing in the repo's Go sources imports them (grep over **/*.go for k8s packages found no direct use), so the alpha is dependency-driven rather than chosen. Flagged the provenance question and the missing changelog note; non-blocking."
},
{
"concern": "correctness: structured-merge-diff jumps v6.4.2 to v7.0.0 (indirect); check transitive compatibility and that no source still imports the v6 path",
"disposition": "not-an-issue",
"detail": "go.sum contains only sigs.k8s.io/structured-merge-diff/v7 v7.0.0 (h1 + /go.mod, lines 283-284); no v6 entries remain. Grep over **/*.go found no import of structured-merge-diff at any version, and go.mod marks it indirect, so no source still references the v6 path."
},
{
"concern": "correctness: go-openapi/swag split/trim; confirm the module graph resolves and removed indirect entries are genuinely unused",
"disposition": "not-an-issue",
"detail": "go.mod now requires swag/conv, jsonutils, pools and typeutils at v0.29.2 and go.sum pairs every one of them; cmdutils/fileutils/loading/mangling/netutils/stringutils/yamlutils and the root swag module are absent from go.mod and no Go source imports github.com/go-openapi/swag. The only residue is the orphan zip hash covered by the go.sum comment."
},
{
"concern": "correctness: emicklei/go-restful/v3 and json-iterator/modern-go entries dropped; verify nothing still needs them and go mod tidy actually ran",
"disposition": "not-an-issue",
"detail": "Grep of go.sum finds no remaining emicklei/go-restful, json-iterator/go, or modern-go/concurrent|reflect2 entries, and no Go source imports them. The diff removes 62 require lines and adds 50 (net -12), which is the characteristic tidy signature (removal of no-longer-needed indirects), so tidy did run; only the single orphan line suggests it ran with -e."
},
{
"concern": "correctness: go.sum 113 additions / 139 deletions with no vendor dir; check every go.mod module has matching h1 and go.mod hashes and no stale hashes remain",
"disposition": "addressed",
"detail": "Spot-checked all four k8s alpha modules, bborbe/k8s v1.14.24, klauspost/compress v1.20.3, golang.org/x/crypto v0.58.0 and structured-merge-diff v7 — each has both h1 and /go.mod lines. Found one unpaired entry (go-openapi/swag v0.27.1, go.sum:70), reported as a nit. Full-graph verification is delegated to CI (`go mod verify` + build in make precommit)."
},
{
"concern": "security: bulk bump (x/crypto, x/net, go-uuid, compress, sentry-go); confirm pure version bump with no checksum or replace-directive tampering and a trustworthy vulncheck claim",
"disposition": "not-an-issue",
"detail": "Read go.mod in full: there is no `replace` and no `exclude` directive, and every version in the diff moves forward. go.sum lines are well-formed h1/base64 entries with no anomalies beyond the one orphan hash reported. Vulnerability status is enforced mechanically in-repo by `make check` (govulncheck with an explicit ignore list + osv-scanner + trivy) in CI, so the PR's vulncheck claim is CI-verified rather than trusted."
},
{
"concern": "security: adopting a prerelease alpha k8s stack means no upstream security backports until v0.38.0 is stable",
"disposition": "addressed",
"detail": "Same comment as the correctness alpha concern (go.mod:87): flagged that an alpha stack receives no backports, asked for provenance confirmation and a changelog note, and suggested pinning to v0.37.x if the alpha is not MVS-forced."
},
{
"concern": "tests: no test files changed; verify the full Ginkgo suite ran green against ginkgo v2.33.1 / gomega v1.44.0 rather than only build+vet",
"disposition": "not-an-issue",
"detail": "Verifier is CI: .github/workflows/ci.yml:33 runs `make precommit`, whose `test` target runs `go test -mod=mod -count=1 -cover` over all packages plus the shell rule-test scripts, on the go-version-file from go.mod. This sandbox has no Go toolchain (`go version` is outside the allowlist), so the suite result comes from CI, not from this review."
},
{
"concern": "tests: CHANGELOG entry lists only 12 bumps while go.mod changes ~50 lines and the PR claims 31 updates; reconcile and note the omitted k8s alpha",
"disposition": "addressed",
"detail": "Comment on CHANGELOG.md:13 with the measured counts (50 added / 62 removed require lines vs 12 named bumps) and the omitted k8s alpha + structured-merge-diff v7 major bump."
},
{
"concern": "performance: sarama, prometheus client and compress bumps can alter runtime defaults; no benchmark evidence for a prod CLI",
"disposition": "not-an-issue",
"detail": "The diff changes only version strings — no code, config, or flag that would alter runtime defaults is touched, so there is no benchmarkable behavioral delta to measure in this PR. Risk is low and the full test suite in CI is the available verifier; noted as unverified-by-benchmark, which does not warrant a finding."
}
]
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated Go toolchain + dependency update.
go vet ./...passes with no source repairs needed; no Dockerfile or vendor dir present, golangci-lint pin v2.14.0 already Go-1.27-compatible.CHANGELOG entry stays under
## Unreleased— the release agent versions and tags on merge.