fix: 💚 Fix cpex build - #862
Conversation
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesGo module dependency update
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This updates indirect Go dependencies for the cpex module. The dependency declarations and checksums are consistent, with no current merge-readiness risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="Running error: context loading failed: no go files to analyze: running 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. Comment |
huang195
left a comment
There was a problem hiding this comment.
Correct and minimal fix. I traced the requirement rather than taking the tidy output on faith:
- gjson is genuinely needed:
cmd/authbridge-cpex/main.goimportsauthlib/pipeline, andauthlib/pipeline/snapshot.goimportsgithub.com/tidwall/gjson.matchandprettyare gjson's own transitive deps. WithGOWORK=offin the cpex Dockerfile and the default-mod=readonly, the missing require is a hard build failure — matches the symptom. sjsonis correctly absent. This was my first suspicion, sinceauthlibrequires both gjson and sjson directly andcmd/authbridge-proxy/cmd/authbridge-envoyeach carry all four indirects. It does not apply here: sjson's only non-test importer in authlib isplugins/toolprune, and cpex'smain.goregisters nine plugins with toolprune not among them.authlib/plugins/cpex/cmf_body_test.godoes import sjson, but tests of dependency modules fall outsideallunder module-graph pruning, so tidy correctly omits it.- Checksums check out. All seven added
go.sumlines are byte-identical to the corresponding entries already inauthbridge/authlib/go.sum, and the versions match the proxy/envoy pins exactly (gjson v1.18.0, match v1.1.1, pretty v1.2.1). The lonepretty v1.2.0/go.modline is ordinary module-graph bookkeeping, not a second version being pulled in.
No blocking issues. Two suggestions below, both about the blast radius rather than this diff: the same gap exists one directory over in cmd/authbridge-praxis, and no check on this PR actually exercises the cpex module graph.
Author: evaline-ju (MEMBER — maintainer)
Areas reviewed: Go module manifests (go.mod/go.sum), CI coverage
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (but see the coverage note — none of it builds cpex)
| github.com/rogpeppe/go-internal v1.15.0 // indirect | ||
| github.com/segmentio/asm v1.2.1 // indirect | ||
| github.com/spiffe/go-spiffe/v2 v2.8.1 // indirect | ||
| github.com/tidwall/gjson v1.18.0 // indirect |
There was a problem hiding this comment.
suggestion: worth closing the hole that let this reach main, because nothing on this PR verifies the fix.
authbridge-cpex appears in exactly one workflow — build.yaml — which triggers only on v* tags, pushes to main, and workflow_dispatch. ci.yaml's Go matrix is [authbridge-proxy, authbridge-envoy]. So every green check here comes from jobs that never resolve the cpex module graph, and the next time authlib gains a dependency this breaks again the same way.
A full cpex build in ci.yaml is genuinely awkward (CGO plus libcpex_ffi.a from the pinned CPEX tarball), but the failure class here is module-graph drift, not compilation — so it can be caught without building anything:
- name: Verify module graph is tidy
run: |
while IFS= read -r -d '' mod; do
(cd "$(dirname "$mod")" && go mod tidy -diff)
done < <(find authbridge -name go.mod -not -path '*/demos/*' -print0 | sort -z)go mod tidy -diff (Go 1.23+; these modules are on 1.26.5) exits non-zero if go.mod/go.sum would change, needs no CGO and no FFI archive, and covers praxis and the lite-tags module for free.
Note the machinery already exists: dependabot-tidy.yml triggers on pull_request and runs go mod tidy across every authbridge/**/go.mod via that same find, but is gated if: github.actor == 'dependabot[bot]' — which is why the tidy check on this PR reads skipping. Ungating a read-only (-diff) variant for all PRs is probably the smallest change that closes the loop.
There was a problem hiding this comment.
follow-up issue filed #863, keeping this PR scoped to just fix
| github.com/spiffe/go-spiffe/v2 v2.8.1 // indirect | ||
| github.com/tidwall/gjson v1.18.0 // indirect | ||
| github.com/tidwall/match v1.1.1 // indirect | ||
| github.com/tidwall/pretty v1.2.1 // indirect |
There was a problem hiding this comment.
suggestion: cmd/authbridge-praxis appears to have the identical gap, and this PR leaves it broken.
The chain is the same one that broke cpex:
cmd/authbridge-praxis/go.mod:37hasreplace github.com/rossoctl/cortex/authbridge/authlib => ../../authlib, so it builds against local authlib despite the pinned pseudo-version above it.cmd/authbridge-praxis/main.goimportsauthlib/praxis;authlib/praxis/praxis.goandauthlib/praxis/policy.goboth importauthlib/pipeline;pipeline/snapshot.goimports gjson.cmd/authbridge-praxis/go.modandgo.sumcontain no tidwall entries at all.- Its Dockerfile sets
ENV GOWORK=offbeforego build, so the workspace can't paper over the missing require the waygo buildfromauthbridge/does locally.
Nobody noticed because praxis is in neither the ci.yaml matrix nor the build.yaml image matrix, so it is built nowhere in CI.
I verified this by reading imports rather than running the praxis image build, so please sanity-check before acting. Either add the same three indirects to cmd/authbridge-praxis/go.mod/go.sum while you're here, or file a follow-up — the go mod tidy -diff gate in my other comment would surface it automatically.
There was a problem hiding this comment.
I see these changes in flight on #855 so I purposely left this 😄
Summary
Main CI build has had issues today starting from https://github.com/rossoctl/cortex/actions/runs/33778911495/job/100727467181 , praxis updates are included in #855
Summary by CodeRabbit