Conversation
WalkthroughUpdated Go toolchain declaration to be derived from go.mod; GitHub Actions setup-go steps now use go-version-file and enable caching; controller-gen bumped to v0.18.0 and CRD YAML metadata/OpenAPI descriptions updated; embedded CRD bundle constants and their SHA256 digests refreshed. No functional code changes. Changes
Sequence Diagram(s)No sequence diagram: changes are CI/tooling versions, CRD metadata, and embedded data; runtime control flow is unaffected. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
888aa0a to
2e72542
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/run_kms_azure_vault_test.yml (1)
16-16: Keep CI in sync with module Go version and enable cachingSame note as in testing.yml: prefer go-version-file to avoid hardcoding. If staying explicit, use 1.24.x to pick up patches.
Option A:
- go-version: "1.24" + go-version-file: "go.mod" + cache: trueOption B:
- go-version: "1.24" + go-version: "1.24.x" + cache: true.github/workflows/manual-build.yml (1)
24-24: Avoid version drift: read Go version from go.mod (or use 1.24.x) and cache modulesThis reduces maintenance across many workflows and speeds up builds.
Option A:
- go-version: "1.24" + go-version-file: "go.mod" + cache: trueOption B:
- go-version: "1.24" + go-version: "1.24.x" + cache: true.github/workflows/run_kms_tls_sa_test.yml (1)
16-16: Consistently derive Go version from go.mod (or use 1.24.x) + enable cacheKeeps CI aligned with the module and benefits from caching.
Option A:
- go-version: "1.24" + go-version-file: "go.mod" + cache: trueOption B:
- go-version: "1.24" + go-version: "1.24.x" + cache: true.github/workflows/core-config-map-tests.yml (1)
21-21: Track Go from go.mod (or bump to 1.24.x) and enable cachingSame rationale as other workflows: reduce drift and speed up CI.
Option A:
- go-version: "1.24" + go-version-file: "go.mod" + cache: trueOption B:
- go-version: "1.24" + go-version: "1.24.x" + cache: true
🧹 Nitpick comments (16)
.github/workflows/run_kms_rotate_test.yml (1)
16-16: Prefer sourcing the Go version from go.mod to avoid drift.
Instead of hardcoding "1.24" in every workflow, rely on go-version-file and optionally enable caching.Apply:
- go-version: "1.24" + go-version-file: go.mod + check-latest: true + cache: true + cache-dependency-path: | + **/go.sumThis keeps CI aligned with the module’s declared version and speeds up builds.
.github/workflows/run_kms_ibm_kp_test.yml (1)
19-19: Reduce maintenance by reading Go version from go.mod.
Same suggestion as other workflows: use go-version-file and enable caching.- go-version: "1.24" + go-version-file: go.mod + check-latest: true + cache: true + cache-dependency-path: | + **/go.sum.github/workflows/run_kms_kmip_test.yml (1)
16-16: Centralize Go version and enable cache.
Mirror the same improvement to pull version from go.mod and cache modules.- go-version: "1.24" + go-version-file: go.mod + check-latest: true + cache: true + cache-dependency-path: | + **/go.sum.github/workflows/operator-tests.yml (1)
21-21: Avoid hardcoding Go version; derive from go.mod and cache dependencies.
Keeps CI and local dev in sync and speeds runs.- go-version: "1.24" + go-version-file: go.mod + check-latest: true + cache: true + cache-dependency-path: | + **/go.sum.github/workflows/run_cosi_test.yaml (1)
23-23: Apply the same go-version-file + cache pattern here.
Future-proofs this workflow when re-enabled.- go-version: "1.24" + go-version-file: go.mod + check-latest: true + cache: true + cache-dependency-path: | + **/go.sum.github/workflows/testing.yml (1)
16-16: Sync Go version across workflows via go.mod and enable module caching
I found 17 CI workflows still hard-pinninggo-version: "1.24". To keep them in sync with yourgo.modand speed up your jobs, please update each step as follows:Option A (preferred – track
go.mod):- with: - go-version: "1.24" + with: + go-version-file: "go.mod" + cache: trueOption B (alternative – pin to latest patch of 1.24):
- go-version: "1.24" + go-version: "1.24.x" + cache: trueWorkflows needing updates (.github/workflows/*.yml):
- testing.yml
- run_kms_tls_sa_test.yml
- run_kms_tls_token_test.yml
- run_kms_rotate_test.yml
- run_kms_kmip_test.yml
- run_kms_ibm_kp_test.yml
- run_kms_dev_test.yml
- run_kms_azure_vault_test.yml
- run_hac_test.yml
- run_cnpg_deployment_test.yml
- operator-tests.yml
- operator-olm-tests.yml
- golangci-lint.yml
- run_admission_test.yml
- manual-build.yml
- cli-tests.yml
- core-config-map-tests.yml
This change will ensure your workflows always match the Go version declared in
go.modand leverage the module cache for faster CI runs..github/workflows/operator-olm-tests.yml (1)
24-24: Prefer deriving Go version from go.mod to avoid driftUsing go-version-file keeps CI in sync with the module and reduces future touch points; enable cache for faster runs.
Apply this diff:
- go-version: "1.24" + go-version-file: "go.mod" + cache: true.github/workflows/run_hac_test.yml (1)
16-16: Use go-version-file and enable caching for consistency and speedDerive the version from go.mod and enable module cache.
- go-version: "1.24" + go-version-file: "go.mod" + cache: true.github/workflows/run_admission_test.yml (1)
23-23: Avoid hardcoding Go version; derive from go.mod and cache modulesPrevents future drift across many workflows; speeds up build.
- go-version: "1.24" + go-version-file: "go.mod" + cache: true.github/workflows/run_cnpg_deployment_test.yml (1)
22-22: Recommend go-version-file + cache for maintainabilityKeeps the workflow synced with go.mod and benefits from caching.
- go-version: "1.24" + go-version-file: "go.mod" + cache: true.github/workflows/run_kms_dev_test.yml (1)
16-16: Switch to go-version-file and enable cachingSimplifies updates across workflows and improves CI time.
- go-version: "1.24" + go-version-file: "go.mod" + cache: true.github/workflows/upgrade-tests-workflow.yaml (1)
26-26: Prefer referencing go.mod over hardcoding the version.Using go-version-file keeps CI in lockstep with go.mod and avoids future drift across workflows.
Apply this minimal change:
- go-version: "1.24" + go-version-file: "go.mod".github/workflows/cli-tests.yml (1)
21-21: Optional: use go-version-file for consistency with go.mod.- go-version: "1.24" + go-version-file: "go.mod".github/workflows/golangci-lint.yml (2)
16-16: Trim trailing whitespace to satisfy yamllint.Yamllint flagged trailing spaces on this line.
- go-version: "1.24" + go-version: "1.24"
16-16: Optional: derive Go version from go.mod to avoid drift.- go-version: "1.24" + go-version-file: "go.mod".github/workflows/run_kms_tls_token_test.yml (1)
16-16: Optional: read version from go.mod for maintainability.- go-version: "1.24" + go-version-file: "go.mod"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
.github/workflows/cli-tests.yml(1 hunks).github/workflows/core-config-map-tests.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/manual-build.yml(1 hunks).github/workflows/operator-olm-tests.yml(1 hunks).github/workflows/operator-tests.yml(1 hunks).github/workflows/run_admission_test.yml(1 hunks).github/workflows/run_cnpg_deployment_test.yml(1 hunks).github/workflows/run_cosi_test.yaml(1 hunks).github/workflows/run_hac_test.yml(1 hunks).github/workflows/run_kms_azure_vault_test.yml(1 hunks).github/workflows/run_kms_dev_test.yml(1 hunks).github/workflows/run_kms_ibm_kp_test.yml(1 hunks).github/workflows/run_kms_kmip_test.yml(1 hunks).github/workflows/run_kms_rotate_test.yml(1 hunks).github/workflows/run_kms_tls_sa_test.yml(1 hunks).github/workflows/run_kms_tls_token_test.yml(1 hunks).github/workflows/testing.yml(1 hunks).github/workflows/upgrade-tests-workflow.yaml(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1662
File: .github/workflows/manual-upgrade-tests.yaml:0-0
Timestamp: 2025-08-07T13:17:18.101Z
Learning: Go 1.23 was released on August 13, 2024, and is a stable version. As of August 2025, Go 1.24 is the latest major version, but Go 1.23 remains actively supported and widely used.
📚 Learning: 2025-08-07T13:17:18.101Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1662
File: .github/workflows/manual-upgrade-tests.yaml:0-0
Timestamp: 2025-08-07T13:17:18.101Z
Learning: Go 1.23 was released on August 13, 2024, and is a stable version. As of August 2025, Go 1.24 is the latest major version, but Go 1.23 remains actively supported and widely used.
Applied to files:
.github/workflows/testing.yml.github/workflows/golangci-lint.yml.github/workflows/manual-build.yml.github/workflows/operator-tests.yml
📚 Learning: 2025-06-25T15:42:04.436Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
Applied to files:
.github/workflows/golangci-lint.yml
🪛 YAMLlint (1.37.1)
.github/workflows/golangci-lint.yml
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ 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). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
🔇 Additional comments (17)
.github/workflows/run_kms_rotate_test.yml (2)
16-16: Go 1.24 bump in CI looks good.
Using actions/setup-go@v5 with "1.24" will resolve the latest patch on the runner.
16-16: Review complete – no Go 1.23 references detected
- .github/workflows: no
go-version: 1.23entries- Dockerfiles/base images: no
golang:1.23pins- go.mod declares
go 1.24and contains notoolchaindirective- No
GOTOOLCHAINoverrides foundPlease give this a quick manual once-over to confirm there are no lingering 1.23 pins before merging.
.github/workflows/run_kms_ibm_kp_test.yml (1)
19-19: Go 1.24 bump in CI looks good.
This aligns the IBM KP workflow with the repo’s target Go version..github/workflows/run_kms_kmip_test.yml (1)
16-16: Go 1.24 bump is consistent.
No issues with actions/setup-go@v5 consuming the latest 1.24 patch..github/workflows/operator-tests.yml (1)
21-21: LGTM on the version bump.
Unit tests will now run against Go 1.24 in CI..github/workflows/run_cosi_test.yaml (1)
23-23: Go 1.24 bump acknowledged.
Even though this workflow is currently disabled (on: []), the toolchain selection is consistent..github/workflows/operator-olm-tests.yml (2)
24-24: LGTM: CI pinned to Go 1.24The setup-go pin to 1.24 aligns with the repo-wide upgrade. No other workflow logic affected.
24-24: No leftover Go 1.23 references detected
All checks confirm the bump to Go 1.24 is complete and no toolchain directive remains:
- Workflows: no
go-version: 1.23pins found- go.mod: uses
go 1.24; nogo 1.23entries- No
toolchaindirective present in any go.mod.github/workflows/run_hac_test.yml (1)
16-16: Go 1.24 bump looks goodMatches the repository upgrade; rest of the workflow remains unchanged.
.github/workflows/run_admission_test.yml (1)
23-23: Go 1.24 setup pin is appropriateAligns with module/toolchain update; no concerns on this step.
.github/workflows/run_cnpg_deployment_test.yml (1)
22-22: CI now uses Go 1.24 — OKChange is minimal and in line with the PR objective.
.github/workflows/run_kms_dev_test.yml (1)
16-16: Go 1.24 pin LGTMConsistent with the repo upgrade; no other changes noted.
.github/workflows/upgrade-tests-workflow.yaml (2)
26-26: Go 1.24 bump looks good.This aligns with the repo-wide upgrade and go.mod change. No issues spotted in this workflow.
26-26: CI workflows clean: no Go 1.23 pins detected• Checked all files under
.github/workflows/for any “1.23” references – none found
• Verified everygo.mod– nogo 1.23ortoolchain go1.23entriesAll CI workflows and module files now consistently use Go 1.24. ✅
.github/workflows/cli-tests.yml (1)
21-21: LGTM on switching CI to Go 1.24.Matches the module update; nothing else in this job depends on the minor version specifics.
.github/workflows/golangci-lint.yml (1)
16-16: LGTM: linter job now uses Go 1.24.No behavioral changes expected from this bump for golangci-lint invocation.
.github/workflows/run_kms_tls_token_test.yml (1)
16-16: Go 1.24 bump looks fine.Build and test steps should remain unaffected by the minor-version upgrade.
2e72542 to
1248a1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/golangci-lint.yml (2)
16-16: Remove trailing whitespace to satisfy YAML linters.There's a trailing space after the Go version which YAML linters flag and some CI parsers can be picky about.
Apply this diff:
- go-version: "1.24" + go-version: "1.24"
14-19: Future-proof the Go version setup by sourcing from go.mod.Hard-coding "1.24" works now, but using actions/setup-go’s go-version-file keeps CI in lockstep with the module directive and avoids drift when the next bump lands.
Apply this diff:
- - uses: actions/setup-go@v5 - with: - go-version: "1.24" + - uses: actions/setup-go@v5 + with: + go-version-file: go.modgo.mod (1)
3-3: Go 1.24 bump is clean – no leftover 1.23 references foundThe scan you ran returned no matches for Go 1.23 in code, Dockerfiles, or workflows. The upgrade to
go 1.24aligns with CI and can be approved as-is.
- If you’d like fully reproducible builds, you can optionally pin the patch-level by re-adding a
toolchain go1.24.xdirective ingo.mod(e.g.toolchain go1.24.7).Makefile (1)
313-321: Harden controller-gen version detection to avoid perpetual reinstall.
controller-gen --versioncommonly prints likeVersion: v0.18.0(note the colon and leading space). The current awk split returns a value with leading whitespace, so the string comparison can fail and trigger an install every run.Apply this diff to trim whitespace and compare only the semantic version:
-ifneq ($(CONTROLLER_GEN_VERSION), $(shell controller-gen --version | awk -F ":" '{print $2}')) +ifneq ($(CONTROLLER_GEN_VERSION), $(shell controller-gen --version 2>/dev/null | grep -oE 'v[0-9]+\.[0-9]+\.[0-9]+' || echo '')) @{ \ echo "Installing controller-gen@$(CONTROLLER_GEN_VERSION)" ;\ set -e ;\ go install -mod=readonly sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION) ;\ echo "Installed controller-gen@$(CONTROLLER_GEN_VERSION)" ;\ } CONTROLLER_GEN=$(GOBIN)/controller-genThis mirrors the robust approach already used for golangci-lint in the same Makefile.
deploy/crds/noobaa.io_bucketclasses.yaml (1)
130-132: Tighten wording in the schema description (docs-only).Minor copy tweak to make the field description crisper for users reading the OpenAPI schema.
Apply this diff:
- description: BackingStoreName is just a name-reference - to a BackingStore + description: Name of a BackingStore resource.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/cli-tests.yml(1 hunks).github/workflows/core-config-map-tests.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/manual-build.yml(1 hunks).github/workflows/operator-olm-tests.yml(1 hunks).github/workflows/operator-tests.yml(1 hunks).github/workflows/run_admission_test.yml(1 hunks).github/workflows/run_cnpg_deployment_test.yml(1 hunks).github/workflows/run_cosi_test.yaml(1 hunks).github/workflows/run_hac_test.yml(1 hunks).github/workflows/run_kms_azure_vault_test.yml(1 hunks).github/workflows/run_kms_dev_test.yml(1 hunks).github/workflows/run_kms_ibm_kp_test.yml(1 hunks).github/workflows/run_kms_kmip_test.yml(1 hunks).github/workflows/run_kms_rotate_test.yml(1 hunks).github/workflows/run_kms_tls_sa_test.yml(1 hunks).github/workflows/run_kms_tls_token_test.yml(1 hunks).github/workflows/testing.yml(1 hunks).github/workflows/upgrade-tests-workflow.yaml(1 hunks)Makefile(1 hunks)deploy/crds/noobaa.io_backingstores.yaml(1 hunks)deploy/crds/noobaa.io_bucketclasses.yaml(2 hunks)deploy/crds/noobaa.io_namespacestores.yaml(1 hunks)deploy/crds/noobaa.io_noobaaaccounts.yaml(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(1 hunks)go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- deploy/crds/noobaa.io_namespacestores.yaml
- deploy/crds/noobaa.io_noobaaaccounts.yaml
- deploy/crds/noobaa.io_backingstores.yaml
- deploy/crds/noobaa.io_noobaas.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- .github/workflows/run_cnpg_deployment_test.yml
- .github/workflows/run_kms_azure_vault_test.yml
- .github/workflows/run_kms_kmip_test.yml
- .github/workflows/run_cosi_test.yaml
- .github/workflows/run_admission_test.yml
- .github/workflows/operator-tests.yml
- .github/workflows/run_hac_test.yml
- .github/workflows/operator-olm-tests.yml
- .github/workflows/upgrade-tests-workflow.yaml
- .github/workflows/cli-tests.yml
- .github/workflows/run_kms_tls_sa_test.yml
- .github/workflows/manual-build.yml
- .github/workflows/run_kms_ibm_kp_test.yml
- .github/workflows/run_kms_dev_test.yml
- .github/workflows/run_kms_rotate_test.yml
- .github/workflows/run_kms_tls_token_test.yml
- .github/workflows/core-config-map-tests.yml
- .github/workflows/testing.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-07T13:17:18.101Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1662
File: .github/workflows/manual-upgrade-tests.yaml:0-0
Timestamp: 2025-08-07T13:17:18.101Z
Learning: Go 1.23 was released on August 13, 2024, and is a stable version. As of August 2025, Go 1.24 is the latest major version, but Go 1.23 remains actively supported and widely used.
Applied to files:
.github/workflows/golangci-lint.yml
📚 Learning: 2025-06-25T15:42:04.436Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
Applied to files:
.github/workflows/golangci-lint.yml
🪛 YAMLlint (1.37.1)
.github/workflows/golangci-lint.yml
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ 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). (13)
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-cli-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: golangci-lint
- GitHub Check: run-hac-test
- GitHub Check: run-kms-key-rotate-test
🔇 Additional comments (2)
Makefile (1)
27-27: Controller-gen v0.18.0 bump LGTM.Matches the CRD annotation updates under deploy/crds and is compatible with the current k8s/controller-runtime stack in this repo.
deploy/crds/noobaa.io_bucketclasses.yaml (1)
6-6: Annotation bump to controller-gen v0.18.0 LGTM.Consistent with the Makefile upgrade and other CRD files in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/testing.yml (1)
16-20: Setup Go via go-version-file + caching looks goodUsing go-version-file and cache is aligned with the repo-wide move to Go 1.24. No functional issues spotted here.
If you use Go workspaces at any point, consider adding go.work.sum to the cache key to improve hit rate:
cache-dependency-path: | - **/go.sum + **/go.sum + **/go.work.sumDo you want deterministic patch versions in CI? With check-latest: true the patch release can float between runs. If you need reproducibility, set it to false.
.github/workflows/run_hac_test.yml (1)
16-20: Consistent Go 1.24 resolution and module cache across jobsThe switch to go-version-file with caching is correct and consistent with the rest of the workflows.
Optionally include go.work.sum in the cache dependency path if/when workspaces are used:
cache-dependency-path: | - **/go.sum + **/go.sum + **/go.work.sumConfirm that allowing check-latest: true aligns with your CI reproducibility policy.
.github/workflows/operator-olm-tests.yml (1)
24-28: LGTM: setup-go derives version from go.mod with cachingLooks correct; matches the pattern used elsewhere in the PR.
As above, consider including go.work.sum for workspace-based builds:
cache-dependency-path: | - **/go.sum + **/go.sum + **/go.work.sum.github/workflows/run_cosi_test.yaml (1)
23-27: Point go-version-file to the checked-out subdirectory to avoid driftThis workflow checks out the repository into noobaa-operator/ and then performs most actions inside that directory. Using go-version-file: go.mod will read the root go.mod, which may diverge from the subdirectory’s go.mod. To make the version source unambiguous, point setup-go at the file inside noobaa-operator/.
Apply:
- go-version-file: go.mod + go-version-file: noobaa-operator/go.mod check-latest: true cache: true cache-dependency-path: | - **/go.sum + **/go.sum + **/go.work.sumIf both root and noobaa-operator/ contain go.mod, validate they declare the same Go version to prevent accidental mismatches between the toolchain used by setup-go and the module being built.
.github/workflows/cli-tests.yml (1)
21-25: Setup-Go via go.mod: good upgrade; add go.work.sum to cache pathsThe switch to go-version-file with caching is solid. To avoid cache misses in case the repo uses workspaces, include go.work.sum.
cache-dependency-path: | **/go.sum + **/go.work.sumAlso note: with check-latest: true, CI will always pick the latest 1.24.x—good for security, but ensure local devs are on the same patch to minimize “works on CI only” drift.
.github/workflows/run_cnpg_deployment_test.yml (1)
22-26: Deriving Go from go.mod is correct; consider caching workspace sumsThe caching config is correct. If/when you adopt Go workspaces, add go.work.sum to keep cache hits stable across modules.
cache-dependency-path: | **/go.sum + **/go.work.sumOptional: If reproducibility across contributor machines is a concern, consider re-introducing a toolchain directive in go.mod (e.g., toolchain go1.24.x) to align local builds with CI’s latest patch chosen by check-latest.
.github/workflows/upgrade-tests-workflow.yaml (1)
26-30: LGTM; add go.work.sum to cache for workspace-aware buildsSame note as the other workflows—include go.work.sum to avoid cache fragmentation in multi-module setups.
cache-dependency-path: | **/go.sum + **/go.work.sum
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
.github/workflows/cli-tests.yml(1 hunks).github/workflows/core-config-map-tests.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/manual-build.yml(1 hunks).github/workflows/operator-olm-tests.yml(1 hunks).github/workflows/operator-tests.yml(1 hunks).github/workflows/run_admission_test.yml(1 hunks).github/workflows/run_cnpg_deployment_test.yml(1 hunks).github/workflows/run_cosi_test.yaml(1 hunks).github/workflows/run_hac_test.yml(1 hunks).github/workflows/run_kms_azure_vault_test.yml(1 hunks).github/workflows/run_kms_dev_test.yml(1 hunks).github/workflows/run_kms_ibm_kp_test.yml(1 hunks).github/workflows/run_kms_kmip_test.yml(1 hunks).github/workflows/run_kms_rotate_test.yml(1 hunks).github/workflows/run_kms_tls_sa_test.yml(1 hunks).github/workflows/run_kms_tls_token_test.yml(1 hunks).github/workflows/testing.yml(1 hunks).github/workflows/upgrade-tests-workflow.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- .github/workflows/run_kms_dev_test.yml
- .github/workflows/manual-build.yml
- .github/workflows/core-config-map-tests.yml
- .github/workflows/run_kms_ibm_kp_test.yml
- .github/workflows/run_admission_test.yml
- .github/workflows/run_kms_tls_token_test.yml
- .github/workflows/operator-tests.yml
- .github/workflows/run_kms_rotate_test.yml
- .github/workflows/run_kms_kmip_test.yml
- .github/workflows/golangci-lint.yml
- .github/workflows/run_kms_azure_vault_test.yml
⏰ 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). (12)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
🔇 Additional comments (2)
.github/workflows/run_kms_tls_sa_test.yml (1)
16-20: Setup Go step updated correctlyDeriving the version from go.mod and enabling the cache is appropriate.
Minor heads-up unrelated to this hunk but relevant to Go 1.24 toolchains: later in this job you mix Ginkgo v2 and v1 install paths:
- go get -v github.com/onsi/ginkgo/v2
- go install -mod=mod -v github.com/onsi/ginkgo/ginkgo
If you intend to use Ginkgo v2, the install path should be github.com/onsi/ginkgo/v2/ginkgo@latest. Otherwise you’ll get the v1 CLI. Confirm the intended version before adjusting.
.github/workflows/upgrade-tests-workflow.yaml (1)
26-30: ✅ All Set: go.mod Version VerifiedThe
go.modfile correctly declaresgo 1.24(line 3), and there is no conflictingtoolchaindirective. Nogo.workorgo.work.sumfiles were found, so no additional caching configuration is needed.
- ran `go mod tidy -go=1.24` - updated controller-gen version to v0.18.0 Signed-off-by: Danny Zaken <dannyzaken@gmail.com> deploy.go Signed-off-by: Danny Zaken <dannyzaken@gmail.com>
c6ba923 to
20b5770
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
313-325: Make controller-gen detection more robust (handle missing binary and version formats).Current check parses
controller-gen --versionwithawk -F ":" '{print $2}'. If controller-gen is missing or its output format changes, Make will emit noisy errors or mis-compare.Recommend:
- First check for existence with
command -v controller-gen.- If present, parse the version with a tolerant regex.
- Only install when different.
Apply this diff for a resilient detection block:
controller-gen: -ifneq ($(CONTROLLER_GEN_VERSION), $(shell controller-gen --version | awk -F ":" '{print $2}')) - @{ \ - echo "Installing controller-gen@$(CONTROLLER_GEN_VERSION)" ;\ - set -e ;\ - go install -mod=readonly sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION) ;\ - echo "Installed controller-gen@$(CONTROLLER_GEN_VERSION)" ;\ - } -CONTROLLER_GEN=$(GOBIN)/controller-gen -else -CONTROLLER_GEN=$(shell which controller-gen) -endif + @{ \ + set -e ;\ + CURRENT=""; \ + if command -v controller-gen >/dev/null 2>&1; then \ + CURRENT="$$(controller-gen --version 2>/dev/null | sed -nE 's/.*Version:[[:space:]]*([v0-9\.]+).*/\1/p')" ;\ + fi ;\ + if [ "$(CONTROLLER_GEN_VERSION)" != "$$CURRENT" ]; then \ + echo "Installing controller-gen@$(CONTROLLER_GEN_VERSION) (current: $$CURRENT)" ;\ + go install -mod=readonly sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION) ;\ + echo "Installed controller-gen@$(CONTROLLER_GEN_VERSION)" ;\ + fi ;\ + } +CONTROLLER_GEN=$(GOBIN)/controller-gen
🧹 Nitpick comments (3)
.github/workflows/run_cnpg_deployment_test.yml (1)
22-26: Good move: derive Go from go.mod and enable caching.
- Using go-version-file with check-latest ensures latest 1.24.x is installed as go.mod evolves.
- cache + cache-dependency-path on **/go.sum is correct for monorepos.
Minor note: check-latest can occasionally cause new patch adoption mid-day and cache misses; acceptable for CI, but be aware.
.github/workflows/manual-build.yml (1)
24-28: Guardrail suggestion for cross-branch builds.If someone triggers this against a much older branch (with an older Go), this step will dutifully install that version. If that’s undesirable, add a small gate to fail when go.mod’s “go” line is less than the minimum we support.
Would you like a short step snippet that enforces a minimum Go version (e.g., 1.22+)?
pkg/bundle/deploy.go (1)
286-286: Optional: Guard against future digest drift in CI.To keep these bundles stable, consider adding a lightweight CI check that recomputes SHA256 over each File_deploy_crds_* constant and asserts equality with its companion Sha256_* variable. This prevents silent mismatches on future controller-gen bumps.
Example Makefile target (pseudo):
+.PHONY: verify-bundle-sha +verify-bundle-sha: + @bash hack/verify-bundle-sha.shAnd wire the script from my verification comment into hack/verify-bundle-sha.sh.
Also applies to: 657-657, 914-914, 1252-1252, 1428-1428
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
.github/workflows/cli-tests.yml(1 hunks).github/workflows/core-config-map-tests.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/manual-build.yml(1 hunks).github/workflows/operator-olm-tests.yml(1 hunks).github/workflows/operator-tests.yml(1 hunks).github/workflows/run_admission_test.yml(1 hunks).github/workflows/run_cnpg_deployment_test.yml(1 hunks).github/workflows/run_cosi_test.yaml(1 hunks).github/workflows/run_hac_test.yml(1 hunks).github/workflows/run_kms_azure_vault_test.yml(1 hunks).github/workflows/run_kms_dev_test.yml(1 hunks).github/workflows/run_kms_ibm_kp_test.yml(1 hunks).github/workflows/run_kms_kmip_test.yml(1 hunks).github/workflows/run_kms_rotate_test.yml(1 hunks).github/workflows/run_kms_tls_sa_test.yml(1 hunks).github/workflows/run_kms_tls_token_test.yml(1 hunks).github/workflows/testing.yml(1 hunks).github/workflows/upgrade-tests-workflow.yaml(1 hunks)Makefile(1 hunks)deploy/crds/noobaa.io_backingstores.yaml(1 hunks)deploy/crds/noobaa.io_bucketclasses.yaml(2 hunks)deploy/crds/noobaa.io_namespacestores.yaml(1 hunks)deploy/crds/noobaa.io_noobaaaccounts.yaml(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(1 hunks)go.mod(1 hunks)pkg/bundle/deploy.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/crds/noobaa.io_namespacestores.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- .github/workflows/run_cosi_test.yaml
- .github/workflows/run_kms_tls_sa_test.yml
- .github/workflows/run_hac_test.yml
- .github/workflows/run_kms_ibm_kp_test.yml
- .github/workflows/operator-tests.yml
- go.mod
- .github/workflows/run_kms_rotate_test.yml
- .github/workflows/run_admission_test.yml
- .github/workflows/core-config-map-tests.yml
- .github/workflows/run_kms_azure_vault_test.yml
- .github/workflows/upgrade-tests-workflow.yaml
- .github/workflows/run_kms_kmip_test.yml
- .github/workflows/golangci-lint.yml
- .github/workflows/operator-olm-tests.yml
- .github/workflows/cli-tests.yml
- deploy/crds/noobaa.io_noobaaaccounts.yaml
- .github/workflows/testing.yml
- .github/workflows/run_kms_tls_token_test.yml
- .github/workflows/run_kms_dev_test.yml
- deploy/crds/noobaa.io_noobaas.yaml
- deploy/crds/noobaa.io_bucketclasses.yaml
⏰ 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). (12)
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: golangci-lint
🔇 Additional comments (8)
deploy/crds/noobaa.io_backingstores.yaml (2)
6-6: Annotation bump to controller-gen v0.18.0 looks good.CRD content is otherwise unchanged in this file. This aligns with the Makefile’s CONTROLLER_GEN_VERSION update.
6-6: Double-check CRD regeneration consistency and compatibility.
- Ensure all CRDs were regenerated with controller-gen v0.18.0 and that pkg/bundle/deploy.go digests match (CI target gen-api-fail-if-dirty should pass).
- v0.18.0 may subtly change OpenAPI defaults/struct tags. Please verify no breaking validation changes for existing BackingStore CRs across supported clusters.
Would you like me to add a CI step that runs
make gen-api-fail-if-dirtyto catch drift automatically?.github/workflows/manual-build.yml (1)
24-28: Solid: setup-go now reads version from the checked-out branch’s go.mod.This workflow checks out an arbitrary branch via input; resolving Go from that branch’s go.mod is exactly what we want. Caching on **/go.sum is also appropriate.
Makefile (2)
27-27: Version bump to controller-gen v0.18.0 acknowledged.Targets that depend on controller-gen will now generate CRDs with the updated toolchain.
130-133: Keep an eye on controller-gen flags compatibility.
crd:generateEmbeddedObjectMeta=trueis still valid in v0.18.0, but upstream flags occasionally change semantics. If you see diffs beyond annotations in the generated CRDs, verify this flag’s behavior against v0.18.0 docs.pkg/bundle/deploy.go (3)
788-791: BucketClass schema doc improvement looks good.The added description under spec.placementPolicy.tiers.backingStores items clarifies the value is a BackingStore name reference. This aligns with existing controller behavior and helps users authoring CRs. No action needed.
293-293: Verify CRD schema compatibility after bump to v0.18.0We’ve confirmed there are 5 occurrences of the updated annotation in
pkg/bundle/deploy.go(lines 293, 664, 921, 1259, 1435). As a sanity check before merging, please ensure:
- No structural JSON-schema changes from controller-gen v0.16.x → v0.18.0 introduce validation drift for existing in-cluster CR instances.
- Your downstream Kubernetes/OpenShift target versions accept the v0.18.0-generated schemas (in particular, review any changes in
x-kubernetes-*markers).You can verify by diffing the CRD YAMLs pre- and post-bump and/or applying them to a test cluster running your minimum supported platform versions.
286-286: Fix YAML extraction logic in the verification script
Our current AWK test ($4=="\"`) never matches lines likeconst File_deploy_crds_noobaa_io_backingstores_yaml = `---because
$4contains “`---”. This causes the entire literal to be skipped and yields the empty‐string hash.Please update the
check_pairroutine to use a sed‐range (or a regex‐aware awk) that correctly captures everything between the opening and closing backticks. For example:# inside check_pair(), replace the awk pipeline with: actual=$( sed -n "/const File_deploy_crds_${name}_yaml = \`/,/^\`$/p" "$file" \ | sed '1d;$d' \ | compute_sha )Re-run the verification script with this change to confirm each
Sha256_deploy_crds_*constant truly matches its embedded YAML.
Signed-off-by: Danny Zaken <dannyzaken@gmail.com>
20b5770 to
1c61c8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/run_kms_tls_token_test.yml (1)
40-42: Fix Ginkgo install: wrong module path and avoid mutating go.mod during CI
go get -v github.com/onsi/ginkgo/v2mutates go.mod in CI; not desirable.- The CLI path should be
github.com/onsi/ginkgo/v2/ginkgo(note the/v2/), and with modern Go you should usego install <module>@<version>for reproducible builds.Replace these lines as follows:
Apply this diff:
- go get -v github.com/onsi/ginkgo/v2 - go install -mod=mod -v github.com/onsi/ginkgo/ginkgo + # Install the Ginkgo v2 CLI without modifying go.mod + # Pin to a version for reproducibility; fall back to @latest if you prefer freshness. + GINKGO_VERSION="${GINKGO_VERSION:-@latest}" + go install -v github.com/onsi/ginkgo/v2/ginkgo${GINKGO_VERSION}Optionally add a quick sanity check right after:
ginkgo versionIf you want this pinned, set
GINKGO_VERSIONin the workflow env or repository secrets (e.g.,@v2.19.0).
🧹 Nitpick comments (3)
.github/workflows/run_kms_tls_token_test.yml (1)
16-20: Broaden cache key to include go.work.sum (if you ever enable workspaces)If the repo switches to Go workspaces later, caching won’t invalidate on
go.work.sumchanges. It’s a low-effort hardening now:Apply this diff:
with: go-version-file: go.mod check-latest: true cache: true cache-dependency-path: | **/go.sum + go.work.sum.github/workflows/run_admission_test.yml (2)
26-27: Include go.work.sum in cache key for workspace setupsIf you ever use a Go workspace, go.work.sum should also participate in the cache key to avoid stale dependency caches when the workspace graph changes.
Suggested tweak:
cache-dependency-path: | - **/go.sum + **/go.sum + go.work.sum
23-25: Pin Go patch version or removecheck-latestfor reproducible CIThe workflow’s
Setup Gostep reads thegodirective from the operator’sgo.mod(which declares Go 1.24) and, withcheck-latest: true, will float to whatever the newest 1.24.x patch is at runtime. That can introduce surprising failures as new Go patch releases roll out.Please choose one of the following:
• Remove (or set to
false) thecheck-latestflag so that the exact version inferred fromgo.modis used every run.
• Pin a specific patch release via a Go toolchain directive (e.g. in ago.workfile) or by hard-coding ago-versioninput:Locations to update:
- .github/workflows/run_admission_test.yml, lines 23–25:
Suggested diff:
- name: Setup Go uses: actions/setup-go@v5 with: go-version-file: ./noobaa-operator/go.mod - check-latest: true # floats to newest patch of the minor version + check-latest: false # ensures the exact version from go.mod is installed cache: trueOptional: to pin patch in a
go.work(create alongside your modules):+ go 1.24 + toolchain go1.24.7 # pin to patch v1.24.7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
.github/workflows/cli-tests.yml(1 hunks).github/workflows/core-config-map-tests.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/manual-build.yml(1 hunks).github/workflows/operator-olm-tests.yml(1 hunks).github/workflows/operator-tests.yml(1 hunks).github/workflows/run_admission_test.yml(1 hunks).github/workflows/run_cnpg_deployment_test.yml(1 hunks).github/workflows/run_cosi_test.yaml(1 hunks).github/workflows/run_hac_test.yml(1 hunks).github/workflows/run_kms_azure_vault_test.yml(1 hunks).github/workflows/run_kms_dev_test.yml(1 hunks).github/workflows/run_kms_ibm_kp_test.yml(1 hunks).github/workflows/run_kms_kmip_test.yml(1 hunks).github/workflows/run_kms_rotate_test.yml(1 hunks).github/workflows/run_kms_tls_sa_test.yml(1 hunks).github/workflows/run_kms_tls_token_test.yml(1 hunks).github/workflows/testing.yml(1 hunks).github/workflows/upgrade-tests-workflow.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- .github/workflows/cli-tests.yml
- .github/workflows/upgrade-tests-workflow.yaml
- .github/workflows/core-config-map-tests.yml
- .github/workflows/run_cosi_test.yaml
- .github/workflows/run_cnpg_deployment_test.yml
- .github/workflows/golangci-lint.yml
- .github/workflows/operator-olm-tests.yml
- .github/workflows/run_kms_ibm_kp_test.yml
- .github/workflows/testing.yml
- .github/workflows/run_kms_azure_vault_test.yml
- .github/workflows/operator-tests.yml
- .github/workflows/run_kms_dev_test.yml
- .github/workflows/run_kms_rotate_test.yml
- .github/workflows/run_kms_tls_sa_test.yml
- .github/workflows/run_hac_test.yml
- .github/workflows/manual-build.yml
- .github/workflows/run_kms_kmip_test.yml
⏰ 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). (13)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-admission-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
- GitHub Check: run-azure-vault-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-dev-test
🔇 Additional comments (3)
.github/workflows/run_kms_tls_token_test.yml (2)
16-20: LGTM: using go-version-file with caching is the right moveDynamic Go version resolution from go.mod, plus
check-latest: trueand dependency caching, is aligned with the PR goal and should reduce maintenance.
16-20: Double-check thecheck-latesttrade-off for CI stability
check-latest: truefetches the newest 1.24.x on each run. Great for security/patch uptake, but it can cause surprise CI breaks on patch-day. If you value reproducibility over freshness on long-lived release branches, consider toggling it off there.Do you want me to open a follow-up to conditionally set this based on branch (e.g., master=true, release/*=false)?
.github/workflows/run_admission_test.yml (1)
23-27: Deriving Go from go.mod + enabling cache looks goodSwitching to go-version-file and turning on the setup-go cache keyed by go.sum aligns the workflow with the repo’s declared toolchain and should speed up CI. Nice improvement.
Explain the changes
go mod tidy -go=1.24Summary by CodeRabbit