chore: upgrade go modules & 3pp used for building #186
Conversation
WalkthroughToolchain, CI, and dependency versions were upgraded across operator, resolver, and pkg modules. Dockerfiles now use Go 1.24.6. Makefile versions for envtest, kustomize, and golangci-lint were updated. go.mod files were modernized with Go 1.24.x and broad dependency bumps, including Kubernetes, Prometheus, and Sentry. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
resolver/Dockerfile (2)
25-25: Make the build reproducible and smaller (-trimpath, -ldflags).
Add flags for deterministic, smaller binaries.-RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o resolver cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ + go build -trimpath -ldflags="-s -w" -o resolver cmd/main.go
28-31: Pin final image and run as non-root; avoid alpine:latest and ensure CA certs.
Current image is mutable and runs as root; HTTPS may fail without CA bundle.Option A (preferred, smallest):
-FROM alpine:latest -COPY --from=builder /workspace/resolver . -EXPOSE 8012 -CMD ["./resolver"] +FROM gcr.io/distroless/static:nonroot +COPY --from=builder /workspace/resolver /resolver +USER 65532:65532 +EXPOSE 8012 +ENTRYPOINT ["/resolver"]Option B (keep Alpine, but pin and harden):
-FROM alpine:latest +FROM alpine:3.20 +RUN apk add --no-cache ca-certificates tzdata && \ + adduser -D -H -u 65532 appuser -COPY --from=builder /workspace/resolver . +COPY --from=builder /workspace/resolver /usr/local/bin/resolver +USER 65532 -EXPOSE 8012 -CMD ["./resolver"] +EXPOSE 8012 +ENTRYPOINT ["/usr/local/bin/resolver"]operator/Dockerfile (2)
27-27: Add -trimpath and -ldflags for smaller, reproducible binaries.-RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o manager cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ + go build -trimpath -ldflags="-s -w" -o manager cmd/main.go
31-31: Pin distroless image by digest.
Locks the runtime base for reproducibility.operator/config/crd/bases/elasti.truefoundry.com_elastiservices.yaml (1)
77-81: Enum for spec.scaleTargetRef.kind likely needs singular Kinds.
Use Deployment/Rollout, not deployments/rollouts.- enum: - - deployments - - rollouts + enum: + - Deployment + - Rollout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumoperator/go.sumis excluded by!**/*.sumpkg/go.sumis excluded by!**/*.sumresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
charts/elasti/templates/elastiservice-crd.yaml(5 hunks)operator/Dockerfile(1 hunks)operator/Makefile(2 hunks)operator/config/crd/bases/elasti.truefoundry.com_elastiservices.yaml(2 hunks)operator/go.mod(1 hunks)pkg/go.mod(1 hunks)resolver/Dockerfile(1 hunks)resolver/go.mod(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Kuttl E2E Tests
🔇 Additional comments (10)
resolver/Dockerfile (1)
2-2: Base image bump to Go 1.25.1 looks good.
Aligned with go.mod upgrades.operator/Dockerfile (1)
2-2: Base image bump to Go 1.25.1 looks good.
Consistent with module toolchain.charts/elasti/templates/elastiservice-crd.yaml (1)
6-6: controller-gen annotation upgrade to v0.19.0 is fine.
Matches Makefile/tooling changes.operator/config/crd/bases/elasti.truefoundry.com_elastiservices.yaml (1)
6-6: controller-gen annotation bump acknowledged.operator/Makefile (2)
151-154: Tool version bumps (kustomize/controller-gen/envtest) look consistent.
Matches CRD annotation and module upgrades.
4-4: ```shell
#!/bin/bash
versions=('v1.26.0-alpha.1' 'v1.26.0-beta.2' 'v0.18.3-rc.1' 'v1.20.0+incompatible' 'v1.19.0')
echo "Testing AWK split and printf for various version strings:"
for v in "${versions[@]}"; do
fields=$(echo "$v" | awk -F'[v.]' '{for(i=1;i<=NF;i++) printf("[%s]", $i); print ""}')
output=$(echo "$v" | awk -F'[v.]' '{printf("1.%d\n", $3)}')
printf "%s -> fields: %s, printing: %s\n" "$v" "$fields" "$output"
done</blockquote></details> <details> <summary>pkg/go.mod (2)</summary><blockquote> `60-66`: **Very new pseudo-versions (kube-openapi/json/utils).** Confirm CI reproducibility and that proxies mirror these commits. --- `3-3`: **Ignore the suggested change: `go 1.25.1` is a valid go directive and already implies a `toolchain go1.25.1`.** ([go.dev](https://go.dev/ref/mod?utm_source=openai), [tip.golang.org](https://tip.golang.org/doc/toolchain?utm_source=openai)) > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>operator/go.mod (1)</summary><blockquote> `6-18`: **Deps upgraded coherently (K8s v0.34.0, controller-runtime v0.22.0).** </blockquote></details> <details> <summary>resolver/go.mod (1)</summary><blockquote> `9-9`: **testify v1.11.1 bump—verify license and Go 1.25 support.** </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/lint-and-test.yaml (2)
39-43: Cache misses in multi-module matrix; scope cache to each module’s go.sum.
cache: trueat repo root won’t detect${{ matrix.modules }}/go.sum. Point the dependency path to the module being linted to improve cache hit rate and speed.- name: Setup go uses: actions/setup-go@v5 with: go-version: ${{ env.GO_VERSION }} - cache: true + cache: true + cache-dependency-path: ${{ matrix.modules }}/go.sum
64-68: Mirror cache fix in test job.Same caching concern applies to the test matrix.
- name: Setup go uses: actions/setup-go@v5 with: go-version: ${{ env.GO_VERSION }} - cache: true + cache: true + cache-dependency-path: ${{ matrix.modules }}/go.sum
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/lint-and-test.yaml(3 hunks)
🔇 Additional comments (2)
.github/workflows/lint-and-test.yaml (2)
26-26: No-op formatting change.Whitespace-only; safe to keep.
12-12: Go 1.25.1 in CI: verify go.mod/toolchain alignment across all modules.If any go.mod still has an older
goortoolchaindirective,GOTOOLCHAINmay fetch a different Go, making CI inconsistent. Add a quick verification step or update all module files accordingly.Run this to confirm all module directives are aligned:
#!/bin/bash # List go and toolchain directives across all modules git ls-files '**/go.mod' | while read -r f; do echo "== $f =="; rg -nP '^\s*(go|toolchain)\s+' -n "$f" || true done # Also show the Go tool actually used at runtime go version && go env GOTOOLCHAIN
|
Need to postpone that: GitHub CLI is running on golang v1.24.6, which blocks codeql and golangci-lint |
|
Hi @rethil It seems this PR has stalled. I suggest we close it for now and coordinate upgrade work through the roadmap instead. During the last community call we decided to pursue CNCF Sandbox and to
If you plan to work on it, we can keep it open. : ) CC: @shubhamrai1993 PS: if you wanna join the next event, you can add it to your calendar. -> https://calendar.google.com/calendar/u/0?cid=Y18xNDMzZTE2ZjZkMzM4ZWRhZGViOGNhYjAxNGE1M2IxZmQ2MzZiMjdkOWJlMWNlOTc4Mjk4ODBjMmE5YjEyZDkxQGdyb3VwLmNhbGVuZGFyLmdvb2dsZS5jb20 |
|
Hey @ramantehlan, |
rethil
force-pushed
the
chore_upgrade_go
branch
from
852877d to
58ce723
Compare
|
@coderabbitai full review |
|
Hey @ramantehlan, could you retriger e2es & review this one? |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
charts/elasti/templates/elastiservice-crd.yaml (1)
90-92: Addtype: objectfor structural schema compliance.
metadataunder triggers usesx-kubernetes-preserve-unknown-fieldsbut lacks an explicit type; this breaks structural schema validation.Apply this diff:
properties: metadata: + type: object x-kubernetes-preserve-unknown-fields: trueresolver/Dockerfile (1)
25-25: Make builds reproducible and stricter.Add -trimpath, strip symbols, disable VCS stamping, and prevent implicit mod writes.
-RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o resolver cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ + go build -mod=readonly -trimpath -buildvcs=false \ + -ldflags="-s -w -buildid=" \ + -o resolver cmd/main.gooperator/Dockerfile (1)
27-27: Harden and reproduce operator build.-RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o manager cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ + go build -mod=readonly -trimpath -buildvcs=false \ + -ldflags="-s -w -buildid=" \ + -o manager cmd/main.go.github/workflows/lint-and-test.yaml (1)
44-46: Avoid go mod tidy in CI; download deps and enforce clean tree.- - name: Install dependencies - run: go mod tidy - working-directory: ${{ matrix.modules }} + - name: Download dependencies + run: go mod download + working-directory: ${{ matrix.modules }} + + - name: Verify no uncommitted changes + run: git diff --exit-code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumoperator/go.sumis excluded by!**/*.sumpkg/go.sumis excluded by!**/*.sumresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/lint-and-test.yaml(3 hunks)charts/elasti/templates/elastiservice-crd.yaml(3 hunks)operator/Dockerfile(1 hunks)operator/Makefile(2 hunks)operator/go.mod(1 hunks)pkg/go.mod(1 hunks)resolver/Dockerfile(1 hunks)resolver/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T11:01:35.692Z
Learnt from: ramantehlan
PR: truefoundry/KubeElasti#184
File: tests/e2e/manifest/global/values-elasti.yaml:19-21
Timestamp: 2025-09-09T11:01:35.692Z
Learning: Test files (like those in tests/e2e/manifest/) can use hardcoded image tags like "v1alpha1" for consistency and predictability in test environments, even when production code might require dynamic tag injection.
Applied to files:
operator/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: golangci-lint (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: golangci-lint (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: Analyze (go)
- GitHub Check: Run Kuttl E2E Tests
🔇 Additional comments (16)
charts/elasti/templates/elastiservice-crd.yaml (7)
49-51: Formatting-only change; no semantic impact. LGTM.
53-55: Formatting-only change; no semantic impact. LGTM.
74-76: Formatting-only change; no semantic impact. LGTM.
94-96: Formatting-only change; no semantic impact. LGTM.
97-99: Formatting-only change; no semantic impact. LGTM.
100-103: Upgrade safety: required fields can break existing CRs.
Ifspec.scaleTargetRefandspec.servicebecame required in this PR series, provide defaults or a conversion/defaulting webhook and update examples/values.
79-81: Fix enum: Kubernetes Kind must be singular, CamelCase (Deployment/Rollout).
Using "deployments"/"rollouts" will invalidate CRs and block admission.Apply this diff:
kind: enum: - - deployments - - rollouts + - Deployment + - Rollout type: stringoperator/Makefile (2)
151-155: LGTM on tool bumps; consistent with CI.Confirm controller-gen v0.19.0 and kustomize v5.7.1 don’t introduce breaking changes in generated CRDs for your APIs.
4-4: Verify ENVTEST_K8S_VERSION derivation and setup‑envtest assetsoperator/Makefile: k8s.io/api=v0.34.1 → ENVTEST_K8S_VERSION=1.34; verification failed because the script looked for bin/setup-envtest-release-0.22 (not created — go install produces bin/setup-envtest). Re-run with:
GOBIN=${PWD}/bin go install sigs.k8s.io/controller-runtime/tools/[email protected] && ./bin/setup-envtest use 1.34 -p list
Confirm setup-envtest release-0.22 publishes assets for 1.34; if not, update the Makefile or use a setup-envtest release that includes 1.34.operator/go.mod (1)
3-5: go/toolchain directives look good.pkg/go.mod (2)
3-6: go/toolchain directives look good.
38-39: Critical: Replace 'go.yaml.in/yaml/*' in pkg/go.mod with correct gopkg.in paths (or remove) — verification incomplete.
rg and go mod tidy reported no repo context (no files searched; go.mod not found). Re-run verification from the repository root and paste outputs:git rev-parse --show-toplevel sed -n '1,160p' pkg/go.mod rg -n --hidden -S 'go\.yaml\.in/yaml' -g '!**/vendor/**' rg --debug -n --hidden -S 'go\.yaml\.in/yaml' -g '!**/vendor/**' # if previous rg returns no files go env GOMOD go mod tidyresolver/go.mod (2)
3-6: go/toolchain directives look good.
50-51: Critical: remove go.yaml.in/yaml/ entries from resolver/go.mod*Typo / supply‑chain risk — let gopkg.in/yaml resolve normally. File: resolver/go.mod (≈ lines 50–51).
- go.yaml.in/yaml/v2 v2.4.2 // indirect - go.yaml.in/yaml/v3 v3.0.4 // indirectSandbox verification failed (rg: "No files were searched"; go mod tidy: "go.mod file not found"). Verify locally with:
rg -n 'go.yaml.in/yaml' -g '!/vendor/' && go mod tidyresolver/Dockerfile (1)
2-2: Pin base images by digest to reduce supply‑chain risk.Use immutable digests for both stages.
Example:
-FROM golang:1.24.6 AS builder +FROM golang:1.24.6@sha256:<digest> AS builder ... -FROM gcr.io/distroless/static:nonroot +FROM gcr.io/distroless/static:nonroot@sha256:<digest>Also applies to: 29-31
.github/workflows/lint-and-test.yaml (1)
24-26: Module discovery may miss submodules; use go.work or git enumeration.- - id: set-modules - run: echo "modules=$(go list -m -json | jq -s '.' | jq -c '[.[].Dir]')" >> $GITHUB_OUTPUT + - id: set-modules + run: | + if [ -f go.work ]; then + modules=$(go work edit -json | jq -c '[.Use[].DiskPath]') + else + modules=$(git ls-files '**/go.mod' | xargs -n1 dirname | jq -R -s -c 'split("\n")[:-1]') + fi + echo "modules=$modules" >> "$GITHUB_OUTPUT"
rethil
force-pushed
the
chore_upgrade_go
branch
from
828efd8 to
a8d4a54
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
resolver/go.mod (1)
3-3: Fix invalid go directive (must be major.minor).Use “go 1.24”, not “1.24.0”, to keep
go modhappy.-go 1.24.0 +go 1.24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumoperator/go.sumis excluded by!**/*.sumpkg/go.sumis excluded by!**/*.sumresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/lint-and-test.yaml(3 hunks)operator/Dockerfile(1 hunks)operator/Makefile(2 hunks)operator/go.mod(1 hunks)pkg/go.mod(1 hunks)resolver/Dockerfile(1 hunks)resolver/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T19:32:11.193Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:60-61
Timestamp: 2025-09-24T19:32:11.193Z
Learning: go.yaml.in/yaml/v2 and go.yaml.in/yaml/v3 can be legitimate indirect dependencies in Go modules, particularly when using Kubernetes packages like k8s.io/apimachinery/pkg/runtime and k8s.io/client-go/kubernetes. These should not be automatically flagged as typos of gopkg.in/yaml entries without proper verification of the dependency chain.
Applied to files:
operator/go.mod
📚 Learning: 2025-09-24T19:42:56.173Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:75-75
Timestamp: 2025-09-24T19:42:56.173Z
Learning: When k8s.io/apiextensions-apiserver appears at a different patch version than other k8s.io modules in a go.mod file, this is typically correct behavior when it's an indirect dependency constrained by controller-runtime. The apiextensions-apiserver module maintains internal consistency with its own k8s.io dependency versions, and manual version alignment attempts should be avoided.
Applied to files:
operator/go.mod
📚 Learning: 2025-09-24T19:32:11.193Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:60-61
Timestamp: 2025-09-24T19:32:11.193Z
Learning: go.yaml.in/yaml/v2 and go.yaml.in/yaml/v3 are legitimate Go modules that can appear as indirect dependencies in projects using Kubernetes packages. These modules have valid checksums in go.sum files and should not be flagged as typos of gopkg.in/yaml entries. The go.yaml.in domain is a valid module source that can coexist with gopkg.in yaml modules in the same project.
Applied to files:
operator/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: Run Kuttl E2E Tests
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
operator/Dockerfile (1)
2-2: Go version upgrade looks good.The upgrade from
golang:1.23togolang:1.24.6aligns with the PR objectives and is consistent with changes across other files.resolver/Dockerfile (1)
2-2: Go version upgrade looks good.The upgrade from
golang:1.23togolang:1.24.6aligns with the PR objectives and is consistent with the operator Dockerfile..github/workflows/lint-and-test.yaml (1)
12-12: Go version upgrade looks good.The upgrade to Go 1.24.6 is consistent with the Dockerfiles and Makefile changes across the PR.
operator/Makefile (2)
4-4: Excellent: ENVTEST_K8S_VERSION now dynamically tracks k8s.io/api.The change from a hardcoded value to
$(shell go list -m -f "{{ .Version }}" k8s.io/api | awk -F'[v.]' '{printf "1.%d", $$3}')ensures the envtest Kubernetes version stays in sync with the k8s.io/api module version. This reduces manual maintenance and prevents version skew.
151-154: Tool version upgrades look good.The upgrades align with the PR objectives:
- kustomize: v5.3.0 → v5.7.1
- envtest: release-0.17 → release-0.22
- golangci-lint: v1.57.2 → v1.64.8
operator/go.mod (3)
3-5: Go toolchain upgrade looks good.The upgrade to
go 1.24.0withtoolchain go1.24.6is consistent with the Dockerfile and CI workflow changes. The syntax follows Go's module versioning conventions correctly.
23-81: Indirect dependency upgrades appear consistent.The broad set of indirect dependency updates reflects the transitive impact of upgrading the primary dependencies. The version choices align with the Kubernetes, Prometheus, and testing ecosystem upgrades.
7-19: Verify controller-runtime minor-release breaking changes
- v0.21.0 stopped enabling the client-side rate limiter by default—ensure you explicitly set QPS and Burst on your REST config (
⚠️ #3119) (newreleases.io)- v0.21.0’s NewUnmanaged/NewTypedUnmanaged no longer require a manager—update any calls accordingly (
⚠️ #3141) (newreleases.io)- v0.21.0 deprecated reconcile.Result.Requeue—replace with RequeueAfter or the new pattern (
⚠️ #3107) (newreleases.io)- v0.22.0 changed default behavior of MatchingLabelsSelector/MatchingFieldsSelector to “Nothing” if nil—verify all selectors are set explicitly (
⚠️ #3279) (github.com)resolver/go.mod (1)
14-16: All Kubernetes dependencies aligned at v0.34.1 across modules. Verified operator/go.mod, pkg/go.mod, and resolver/go.mod entries; no mismatches found.pkg/go.mod (2)
51-56: k8s pseudo-versions are consistent across operator, pkg, and resolver modules.
16-57: YAML dependencies are only indirect – no action required.All instances of go.yaml.in/yaml/{v2,v3}, gopkg.in/yaml.v3, and sigs.k8s.io/yaml are transitive dependencies with no direct imports. Let Go modules manage them.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docs/overrides/main.html(1 hunks)docs/src/adopters.md(1 hunks)mkdocs.yml(1 hunks)operator/internal/controller/opsModes.go(2 hunks)tests/e2e/manifest/send_traffic.sh(5 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/05-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/07-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/10-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/12-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/15-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/17-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/20-assert.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/03-assert.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/06-scale-to-1-resolver.yaml(1 hunks)tests/e2e/tests/07-scale-down-with-filter/05-assert.yaml(0 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/06-assert-mode-proxy.yaml(0 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/08-assert-mode-serve.yaml(0 hunks)tests/e2e/tests/09-statefulset-serve-via-traffic/02-assert.yaml(0 hunks)tests/e2e/tests/09-statefulset-serve-via-traffic/05-assert.yaml(0 hunks)
💤 Files with no reviewable changes (9)
- tests/e2e/tests/03-enable-serve-via-manual/03-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/15-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/05-assert.yaml
- tests/e2e/tests/09-statefulset-serve-via-traffic/05-assert.yaml
- tests/e2e/tests/09-statefulset-serve-via-traffic/02-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/10-assert.yaml
- tests/e2e/tests/07-scale-down-with-filter/05-assert.yaml
- tests/e2e/tests/08-resolver-multiple-endpoints/08-assert-mode-serve.yaml
- tests/e2e/tests/08-resolver-multiple-endpoints/06-assert-mode-proxy.yaml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
tests/e2e/manifest/send_traffic.sh
[info] 126-126: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 130-130: echo may not expand escape sequences. Use printf.
(SC2028)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: golangci-lint (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: Analyze (go)
- GitHub Check: Run Kuttl E2E Tests
🔇 Additional comments (11)
tests/e2e/tests/02-enable-serve-via-traffic/20-assert.yaml (1)
1-6: LGTM: final state asserts serve mode.Manifest is valid for a partial-status assert of target-elastiservice.
tests/e2e/tests/02-enable-serve-via-traffic/17-assert.yaml (1)
1-6: LGTM: proxy mode assertion.Structure and fields look correct for the e2e assert.
tests/e2e/tests/02-enable-serve-via-traffic/12-assert.yaml (1)
6-6: Confirm intended behavior: mode switched to proxy at step 12.Change aligns with earlier proxy asserts and later serve at step 20. Please confirm this reflects the desired transition timing in the scenario.
tests/e2e/tests/02-enable-serve-via-traffic/07-assert.yaml (1)
1-6: LGTM: proxy mode assert.Valid minimal assert for target-elastiservice status.
tests/e2e/tests/02-enable-serve-via-traffic/02-assert.yaml (1)
1-6: LGTM: initial proxy state assertion.Looks correct for the early-step expected status.
operator/internal/controller/opsModes.go (2)
58-58: TODO flags missing transactional semantics.The lack of atomicity means that if EndpointSlice creation/update succeeds but a subsequent step (e.g.,
watchPublicService) fails, the system may be left in an inconsistent state with no rollback.Please verify that error paths properly clean up partial changes or that the system can safely recover from partial enablement.
74-83: Verify no race between EndpointSlice creation and public service watch. Confirm that callingcreateOrUpdateEndpointsliceToResolverbeforewatchPublicServicecannot allow traffic to route before the watch is registered, and review error paths to ensure any partially created resources (EndpointSlices or watches) are cleaned up or rolled back.tests/e2e/manifest/send_traffic.sh (3)
64-92: Time-bounded log collection is a solid improvement.Switching from tail-based (
--tail) to time-bounded (--since-time) log retrieval provides more accurate debugging context for failures. Using--all-pods=truefor the resolver ensures logs from all replicas are captured.
126-128: Clarify the rationale for the initial delay.The 5-second delay before the first two requests suggests a timing dependency or startup issue. If this is working around a real problem (e.g., service not ready), consider adding an explicit readiness check instead.
Please verify whether this delay is:
- A workaround for a known race condition that should be fixed
- Necessary wait time for service startup that should be explicit
- A hedge against test flakiness that masks underlying issues
If it's a workaround, please add a comment explaining why it's needed.
133-143: Curl retry logic improves test resilience.Adding
--retry 2and--retry-delay 1makes the test more robust against transient network issues without masking real failures (since only 2 retries are attempted).mkdocs.yml (1)
144-144: LGTM!The new "Adopters" navigation entry is correctly configured and points to the corresponding documentation page.
ramantehlan
force-pushed
the
chore_upgrade_go
branch
from
cf8211a to
a8d4a54
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/e2e/tests/06-resolver-endpoint-sync/06-scale-to-1-resolver.yaml (1)
3-5: Extend the KUTTL command timeout
kubectl rollout status --timeout=400scan’t run that long under KUTTL’s default 30 s per-command timeout, so the suite will kill it and the step will flake. Add an explicit timeout override (e.g.timeout: 420on the command or step) or move the wait into an assert so the harness controls the wait budget.Apply this diff to patch the command timeout:
commands: - - command: kubectl scale deployment/elasti-resolver -n elasti --replicas=1 - - command: kubectl rollout status deployment/elasti-resolver -n elasti --timeout=400s + - command: kubectl scale deployment/elasti-resolver -n elasti --replicas=1 + - command: kubectl rollout status deployment/elasti-resolver -n elasti --timeout=400s + timeout: 420docs/overrides/main.html (1)
12-14: Trim the trailing whitespace on the blank line.Line 14 still carries stray spaces. Please strip them so the line is empty.
- +tests/e2e/manifest/send_traffic.sh (2)
126-126: Shellcheck: Quote the variable to prevent word splitting.Shellcheck flags that
$ishould be quoted to prevent globbing and word splitting.
130-130: Shellcheck: Use printf instead of echo for escape sequences.Shellcheck warns that
echomay not expand escape sequences reliably. Useprintfinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docs/overrides/main.html(1 hunks)docs/src/adopters.md(1 hunks)mkdocs.yml(1 hunks)operator/internal/controller/opsModes.go(2 hunks)tests/e2e/manifest/send_traffic.sh(5 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/05-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/07-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/10-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/12-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/15-assert.yaml(0 hunks)tests/e2e/tests/02-enable-serve-via-traffic/17-assert.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/20-assert.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/03-assert.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/06-scale-to-1-resolver.yaml(1 hunks)tests/e2e/tests/07-scale-down-with-filter/05-assert.yaml(0 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/06-assert-mode-proxy.yaml(0 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/08-assert-mode-serve.yaml(0 hunks)tests/e2e/tests/09-statefulset-serve-via-traffic/02-assert.yaml(0 hunks)tests/e2e/tests/09-statefulset-serve-via-traffic/05-assert.yaml(0 hunks)
💤 Files with no reviewable changes (9)
- tests/e2e/tests/08-resolver-multiple-endpoints/08-assert-mode-serve.yaml
- tests/e2e/tests/09-statefulset-serve-via-traffic/05-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/05-assert.yaml
- tests/e2e/tests/07-scale-down-with-filter/05-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/10-assert.yaml
- tests/e2e/tests/02-enable-serve-via-traffic/15-assert.yaml
- tests/e2e/tests/08-resolver-multiple-endpoints/06-assert-mode-proxy.yaml
- tests/e2e/tests/03-enable-serve-via-manual/03-assert.yaml
- tests/e2e/tests/09-statefulset-serve-via-traffic/02-assert.yaml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
tests/e2e/manifest/send_traffic.sh
[info] 126-126: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 130-130: echo may not expand escape sequences. Use printf.
(SC2028)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: Run Kuttl E2E Tests
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
tests/e2e/manifest/send_traffic.sh (2)
64-86: LGTM! Time-bounded log retrieval improves traceability.The updated
log_failure_detailsfunction now accepts astart_timeargument and uses--since-timeto retrieve logs, which provides precise temporal filtering for post-mortem tracing in e2e scenarios. This is a significant improvement over tail-based retrieval.
138-139: LGTM! Curl retry options improve test robustness.Adding
--retry 2and--retry-delay 1to the curl command enhances the resilience of the test by allowing transient failures to recover automatically.tests/e2e/tests/02-enable-serve-via-traffic/20-assert.yaml (1)
1-6: LGTM! Standard e2e test manifest.This ElastiService manifest with
status.mode: serveis a standard e2e test resource that aligns with the serve-via-traffic test flow.tests/e2e/tests/02-enable-serve-via-traffic/12-assert.yaml (1)
1-6: LGTM! Mode changed to proxy for test flow.The
status.modechange toproxyaligns with the test flow for enabling serve-via-traffic scenarios. The removal of the TestAssert block indicates a shift toward resource-driven assertions.tests/e2e/tests/02-enable-serve-via-traffic/17-assert.yaml (1)
1-6: LGTM! Standard e2e test manifest.This ElastiService manifest with
status.mode: proxyis a standard e2e test resource that aligns with the serve-via-traffic test flow.tests/e2e/tests/02-enable-serve-via-traffic/07-assert.yaml (1)
1-6: LGTM! Standard e2e test manifest.This ElastiService manifest with
status.mode: proxyis a standard e2e test resource that aligns with the serve-via-traffic test flow.tests/e2e/tests/02-enable-serve-via-traffic/02-assert.yaml (1)
1-6: LGTM! Standard e2e test manifest.This ElastiService manifest with
status.mode: proxyis a standard e2e test resource that aligns with the serve-via-traffic test flow.operator/internal/controller/opsModes.go (1)
74-83: No impact on ordering detected.createOrUpdateEndpointsliceToResolvercompletes synchronously before returning, andwatchPublicServiceonly registers its informer viasync.Once, so the new sequence cannot race or cause transient routing failures.
ramantehlan
force-pushed
the
chore_upgrade_go
branch
from
96a4940 to
a8d4a54
Compare
ramantehlan
force-pushed
the
chore_upgrade_go
branch
from
a8d4a54 to
3b5d39e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
operator/Dockerfile (1)
2-33: Pin builder & runtime images by digest to reduce supply-chain risk. Tag-only references are mutable and break reproducibility; please resolve by pinning bothgolang:1.24.6andgcr.io/distroless/static:nonrootto immutable digests.resolver/Dockerfile (2)
1-33: Fix typo in header comment. Please spell “binary” correctly.
2-31: Pin builder & runtime images by digest to reduce supply-chain risk. Tag-only references are mutable; please switch bothgolang:1.24.6andgcr.io/distroless/static:nonrootstages to digest-pinned images.pkg/go.mod (1)
3-5: Invalidgodirective — must bego 1.24. Including the patch in thegoline makesgo modfail. Please drop the patch component while keeping the toolchain pin.-go 1.24.0 +go 1.24 toolchain go1.24.6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumoperator/go.sumis excluded by!**/*.sumpkg/go.sumis excluded by!**/*.sumresolver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/lint-and-test.yaml(3 hunks)operator/Dockerfile(1 hunks)operator/Makefile(2 hunks)operator/go.mod(1 hunks)pkg/go.mod(1 hunks)resolver/Dockerfile(1 hunks)resolver/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T19:32:11.193Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:60-61
Timestamp: 2025-09-24T19:32:11.193Z
Learning: go.yaml.in/yaml/v2 and go.yaml.in/yaml/v3 can be legitimate indirect dependencies in Go modules, particularly when using Kubernetes packages like k8s.io/apimachinery/pkg/runtime and k8s.io/client-go/kubernetes. These should not be automatically flagged as typos of gopkg.in/yaml entries without proper verification of the dependency chain.
Applied to files:
operator/go.modresolver/go.mod
📚 Learning: 2025-09-24T19:42:56.173Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:75-75
Timestamp: 2025-09-24T19:42:56.173Z
Learning: When k8s.io/apiextensions-apiserver appears at a different patch version than other k8s.io modules in a go.mod file, this is typically correct behavior when it's an indirect dependency constrained by controller-runtime. The apiextensions-apiserver module maintains internal consistency with its own k8s.io dependency versions, and manual version alignment attempts should be avoided.
Applied to files:
operator/go.mod
📚 Learning: 2025-09-24T19:32:11.193Z
Learnt from: rethil
PR: truefoundry/KubeElasti#186
File: operator/go.mod:60-61
Timestamp: 2025-09-24T19:32:11.193Z
Learning: go.yaml.in/yaml/v2 and go.yaml.in/yaml/v3 are legitimate Go modules that can appear as indirect dependencies in projects using Kubernetes packages. These modules have valid checksums in go.sum files and should not be flagged as typos of gopkg.in/yaml entries. The go.yaml.in domain is a valid module source that can coexist with gopkg.in yaml modules in the same project.
Applied to files:
operator/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: Run Kuttl E2E Tests
- GitHub Check: Analyze (go)
Description
Upgrade:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit
Chores
Tests