Upgrade to go 1.24 #498
Upgrade to go 1.24 #498
Conversation
WalkthroughProject-wide upgrades moving Go toolchain and related tooling from 1.22 → 1.24 across devcontainer, devspace, Containerfile builder, CI workflows, README, and go.mod; go.mod also upgrades and removes multiple dependencies and adds a replace for controller-runtime. A devspace profile was added to opt back into Go 1.22. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are version/tooling upgrades without control-flow or feature modifications.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
README.md (4)
7-9: Clarify reference to KServe vs. ODH KServe forkGiven the dependency swap to the ODH-maintained KServe fork, consider noting that here so users know which KServe distribution/CRDs this controller targets.
3-3: Fill the overview TODOA concise 2–3 sentence overview helps new contributors and users. Happy to suggest copy if you share the positioning you want.
110-110: Contributing section TODOConsider adding contribution flow (issue triage, PR labels, DCO/signoff, commit message style), local test instructions, and supported Go/K8s versions.
21-24: Align Kubernetes prerequisites in README with go.modThe k8s.io modules in go.mod are at v0.33.1, so we should bump the documented kubectl/cluster versions from v1.11.3+ to v1.33.1+ to match.
• Update README.md (lines 21–24):
@@ README.md:21-24 - podman version v5.2+. - kubectl version v1.11.3+. - Access to a Kubernetes v1.11.3+ cluster. + podman version v5.2+. + kubectl version v1.33.1+. + Access to a Kubernetes v1.33.1+ cluster..github/workflows/test-e2e.yml (2)
19-19: Prefer go-version-file over hardcoding the versionUsing go-version-file keeps Actions auto-synced with go.mod/toolchain and avoids drift during future upgrades.
Apply this diff:
- go-version: '~1.24' + go-version-file: 'go.mod'
21-26: Avoid floating 'latest' Kind; pin Kind and node image for reproducibilityFloating latest can break CI unexpectedly when Kind or Kubernetes releases. Pin both the Kind binary and the node image to a known-good combo (matching your k8s modules).
Apply this diff with example pins (adjust versions to what you validate in CI):
- - name: Install the latest version of kind - run: | - curl -Lo ./kind https://kind.sigs.k8s.io/dl/latest/kind-linux-amd64 - chmod +x ./kind - sudo mv ./kind /usr/local/bin/kind + - name: Install pinned Kind + env: + KIND_VERSION: v0.24.0 + run: | + curl -fsSL -o ./kind https://github.com/kubernetes-sigs/kind/releases/download/${KIND_VERSION}/kind-linux-amd64 + chmod +x ./kind + sudo mv ./kind /usr/local/bin/kind @@ - - name: Create kind cluster - run: kind create cluster + - name: Create kind cluster (pinned node image) + env: + # Choose a node image compatible with your k8s.io/* module minor version + KIND_NODE_IMAGE: kindest/node:v1.30.2 + run: kind create cluster --image "${KIND_NODE_IMAGE}"If you want, I can generate a matrix of tested Kind/Kubernetes image versions based on your go.mod k8s minor to eliminate guesswork.
.github/workflows/test.yml (2)
18-18: Use go-version-file for single-source-of-truthKeeps workflow aligned with go.mod/toolchain without touching YAML on every upgrade.
Apply this diff:
- go-version: '~1.24' + go-version-file: 'go.mod'
20-25: Optional: enable module cache for faster CIactions/setup-go supports caching the module download. This can cut minutes off CI on cold runners.
Apply this diff (requires actions/setup-go caching support):
- name: Running Tests run: | go mod tidy git diff --exit-code || exit 1 make test + - name: (Optional) Cache Go modules + if: ${{ false }} # Move above and enable when ready; shown here to avoid changing job order + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod' + cache: trueIf you prefer not to duplicate setup-go, move the cache: true into the existing Setup Go step instead.
.devcontainer/devcontainer.json (1)
3-3: Optional: consider aligning devcontainer base with builder image for parityUsing the same base family as the Containerfile builder (ubi9/go-toolset:1.24) can reduce "works on my machine" drift when debugging CGO or OS-level differences. If you prefer the upstream golang image for convenience, this is fine to keep.
Containerfile (1)
25-25: Optional: slimmer, reproducible binariesTrim path info and strip symbols to reduce image size; keeps behavior unchanged.
Apply this diff:
-RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} go build -a -o manager cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} go build -trimpath -ldflags="-s -w" -a -o manager cmd/main.goIf you embed version info via ldflags, I can wire that up as well (commit, tag, date).
.github/workflows/lint.yml (2)
15-19: Align setup-go with go.mod toolchain to prevent implicit downloadsYou pin
toolchain go1.24.5in go.mod but use a floating~1.24here. If the runner picks a lower patch (e.g., 1.24.0), the Go toolchain may try to auto-download 1.24.5 at build time. In restricted CI environments, that can fail.Consider reading the version from go.mod so CI always matches the module’s toolchain.
Apply this diff:
- - name: Setup Go - uses: actions/setup-go@v5 - with: - go-version: '~1.24' + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod'
21-24: golangci-lint upgrade looks good; consider a longer timeout for larger reposAction v6 with linter v2.4.0 is fine. To reduce intermittent CI flakes on larger codebases, set an explicit timeout.
Optional addition:
- name: Run linter uses: golangci/golangci-lint-action@v6 with: version: v2.4.0 + args: --timeout=5mgo.mod (4)
3-6: Pinning toolchain to go1.24.5: verify CI/build images match or allow downloads
toolchain go1.24.5enforces that patch in environments where GOTOOLCHAIN=auto (default). If your CI images or developer machines run an older 1.24.x and outbound downloads are blocked, builds will fail.
- If you want CI to always use 1.24.5, switch actions/setup-go to read from go.mod (see workflow suggestion) or pin it to 1.24.5.
- If you prefer flexibility, consider removing or relaxing the toolchain pin and rely on the CI/go toolchain management instead.
Would you like me to raise a follow-up PR to align all workflows and the Containerfile to the same patch version?
121-127: OpenTelemetry module versions are mixed; prefer a single minor across OTEL modulesCore OTEL modules are v1.35.0 while otlptrace exporters are v1.33.0. OTEL recommends aligning module minors to avoid interface/ABI drift.
Apply this diff if compatible with your code:
- go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.33.0 // indirect - go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.33.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.35.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.35.0 // indirectIf 1.35.0 isn’t available for these submodules, align all OTEL modules to 1.33.x instead.
Also applies to: 122-123
37-38: controller-runtime version mismatch between require and replace is confusingYou require
sigs.k8s.io/controller-runtime v0.20.4but replace it withv0.19.1. The build will use 0.19.1, but the mismatch misleads readers/tools and can cause IDE resolution confusion.Option A (recommended): Align require with the pinned version and keep the context comment.
- sigs.k8s.io/controller-runtime v0.20.4 + sigs.k8s.io/controller-runtime v0.19.1You may then drop the replace (since require already pins), or keep it with the rationale comment if other transitive deps attempt to upgrade it.
Also applies to: 166-169
170-173: No direct ODH fork imports detected; please document the fork rationale in go.modA scan of the Go source shows no imports of
github.com/opendatahub-io/kserve—all code still imports
github.com/kserve/kserve, and thereplacedirective at lines 170–173
in go.mod will transparently redirect to the ODH fork.• Confirmed no direct references to the fork path in any Go files.
• Add a short comment alongside thereplacein go.mod that explains:
– Why we’re using the ODH fork (which patches/features it provides)
– The plan or criteria for switching back to upstream kserve in the future
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.devcontainer/devcontainer.json(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test-e2e.yml(1 hunks).github/workflows/test.yml(1 hunks)Containerfile(1 hunks)README.md(1 hunks)go.mod(6 hunks)
🔇 Additional comments (5)
README.md (1)
20-20: Go prerequisite bumped to 1.24.0+: aligned with CI and imagesDocs now reflect the toolchain update. Looks good.
.devcontainer/devcontainer.json (1)
3-3: Devcontainer image bumped to golang:1.24Good alignment with the repo toolchain.
Containerfile (1)
2-2: Builder updated to go-toolset 1.24Consistent with the project-wide Go upgrade. Looks good.
go.mod (2)
29-33: Kubernetes 0.33.x bump: watch for transitive API changesUpgrading k8s.io/{api,apimachinery,apiserver,client-go} to v0.33.1 can introduce subtle API behavior changes (e.g., context handling, serializer defaults, scheme registrations). Ensure e2e and controller-runtime integration tests cover leader election, client config, and RESTMapper behavior.
If not already, run your e2e suite against a 1.30/1.31 cluster matching k8s 0.33 client libraries.
81-84: Large indirect updates: run a tidy and validate minimal version selectionGiven the breadth of indirect bumps (google, prometheus, x/*, gateway-api), ensure the module graph is clean.
- Run:
go mod tidy -vand commit changes if any.- Verify license compliance for newly pulled transitive modules, especially gateway-api-inference-extension.
- Confirm that Prometheus common/procfs versions work with your metrics registration (no duplicated collectors).
Also applies to: 106-108, 131-140, 153-164
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
.golangci.yml(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 on Ubuntu
hdefazio
force-pushed
the
fix/use_odh_kserve
branch
3 times, most recently
from
e285cec to
4e187f5
Compare
hdefazio
changed the title
hdefazio
force-pushed
the
fix/use_odh_kserve
branch
from
4e187f5 to
b8a3ee1
Compare
|
pr to change the unit tests to run with go 1.24: openshift/release#68246 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
dev_tools/devspace.yaml (2)
58-58: Dev image upgraded to Go 1.24: good; consider aligning patch level with go.mod toolchainNice update to a 1.24 dev image. go.mod pins toolchain go1.24.5 while this image is 1.24.2. If the container lacks 1.24.5, the Go toolchain directive will download it at runtime, which can slow local dev and require outbound network. Either update the image to a 1.24.5-based tag or drop/adjust the toolchain directive to match the image to avoid the download.
105-111: Fix YAML trailing whitespace and keep profile cleanYAMLlint flags trailing spaces on Line 106. Also, minor nit: trim the trailing space after the profile name.
Apply this diff:
- - name: go1.22 + - name: go1.22go.mod (3)
3-6: Go toolchain vs environment: align versions or avoid forced downloadsYou’ve set:
- go 1.24.1
- toolchain go1.24.5
If build/dev images provide only 1.24.2, the toolchain directive will fetch 1.24.5 on the fly. If that’s undesirable in CI or dev (air-gapped, or to reduce variance), consider one of:
- Align images/CI to 1.24.5.
- Or drop/relax the toolchain directive to match what’s installed.
Two alternative diffs (pick one strategy):
Option A: remove toolchain pin
-go 1.24.1 - -toolchain go1.24.5 +go 1.24.1Option B: adjust toolchain to match image (if image is 1.24.2)
-toolchain go1.24.5 +toolchain go1.24.2
14-14: Replace to ODH kserve: solid; consider pinning to a tag if availableThe replace to the ODH kserve fork/commit achieves the goal and unblocks types like authenticationRef. If the odh-v2.33 tag exists on the fork, pinning to it improves reproducibility and readability.
Proposed (only if the tag exists):
-replace github.com/kserve/kserve => github.com/opendatahub-io/kserve v0.0.0-20250812054942-f704774f7e6d +replace github.com/kserve/kserve => github.com/opendatahub-io/kserve odh-v2.33Also applies to: 170-173
122-127: OTel component versions are mixed (1.33.0 exporters with 1.35.0 core)This typically works, but cross-minor mismatches have occasionally caused init/runtime surprises. Keep an eye on traces export and shutdown behavior. If anything looks off, bump exporters to match 1.35.0 to reduce variance.
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.devcontainer/devcontainer.json(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test-e2e.yml(1 hunks).github/workflows/test.yml(1 hunks)Containerfile(1 hunks)README.md(1 hunks)dev_tools/devspace.yaml(2 hunks)go.mod(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/devcontainer.json
🚧 Files skipped from review as they are similar to previous changes (5)
- Containerfile
- README.md
- .github/workflows/lint.yml
- .github/workflows/test-e2e.yml
- .github/workflows/test.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
dev_tools/devspace.yaml
[error] 106-106: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
dev_tools/devspace.yaml (1)
105-111: Profile patch path looks correct; approach LGTMUsing a profile to override dev.app.devImage for a Go 1.22 opt-in is clean and minimally invasive. No functional issues spotted.
go.mod (2)
29-33: Version skew: k8s libs v0.33.1 with controller-runtime v0.19.1You’re forced to hold controller-runtime at v0.19.1 due to KEDA (as noted), while bumping k8s.io/* to v0.33.1. That’s a large skew and can surface subtle issues (API and scheme changes, client behavior). If CI is green, fine—just make sure e2e covers reconciliation, watches, leader election, and webhook paths.
If you see oddities, options include:
- Introducing a replace to constrain k8s.io/* closer to what controller-runtime v0.19 supports, or
- Investigating the KEDA incompatibility window again to see if 0.20.x is feasible now.
Also applies to: 166-169
159-160: Gateway API inference extension addition helps address authenticationRef parityGood to see gateway-api-inference-extension included; this complements the ODH kserve fork for preserving fields like authenticationRef.
|
Re: Code rabbit comment
I did originally set it up this way, but vscode flagged it as invalid syntax. Plus running |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
go.mod (3)
35-35: Align controller-runtime require with replace, or document the divergence.You require
sigs.k8s.io/controller-runtime v0.20.4but forcibly replace it withv0.19.1. This works but is confusing and risks re-introducingv0.20.xon future tidy/updates. Two options:
- Prefer aligning the require to the actual version used (recommended for clarity).
- If you need the higher require for transitive reasons, add a clarifying comment where it is required.
Proposed alignment:
- sigs.k8s.io/controller-runtime v0.20.4 + sigs.k8s.io/controller-runtime v0.19.1Also confirm runtime compatibility with your Kubernetes stack (you’re on k8s 0.33.1) while pinned to controller-runtime 0.19.1.
Also applies to: 164-167
119-125: Consider aligning OpenTelemetry exporter versions to the SDK/core (optional).You’re on otel core/sdk v1.35.0 but exporters pinned at v1.33.0. While 1.x is generally compatible, keeping the same minor across otel modules reduces churn and surprises.
Suggestion:
- go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.33.0 // indirect - go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.33.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.35.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.35.0 // indirectIf the graph pulls these down again, add explicit requires or a replace block to keep them aligned.
27-30: Kubernetes 0.33.1 bump: sanity-check transitive compatibility (optional).Given the mix of k8s 0.33.1, controller-runtime (replaced to 0.19.1), and KEDA constraints, please confirm builds, CRD generation, and e2e tests pass with this graph. Your deployment test is encouraging; just ensure CI covers these edges.
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
go.mod(6 hunks)
🔇 Additional comments (2)
go.mod (2)
168-171: ODH kserve replacement looks correct and matches PR intent.Keeping the upstream import path and using a replace to the ODH fork (odh-v2.33 commit) is a clean way to adopt the fork without touching call sites. Comment annotations are helpful.
3-4: Incorrectgodirective` recommendationThe
godirective in go.mod accepts full semver (e.g.go 1.24.1), not justmajor.minor. No change is required—go 1.24.1is valid.If you do wish to pin the toolchain to that exact patch release, you can add:
go 1.24.1 +toolchain go1.24.1Likely an incorrect or invalid review comment.
go.mod
Outdated
|
|
||
| // Use ODH release branch instead | ||
| // odh-v2.33 | ||
| replace github.com/kserve/kserve => github.com/opendatahub-io/kserve v0.0.0-20250812054942-f704774f7e6d |
There was a problem hiding this comment.
I am not sure if it is a good idea here, as it would add more work later, as it points to a specific commit; we would need to update it every time.
Instead, maybe point our kserve here:
https://github.com/opendatahub-io/odh-model-controller/pull/498/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R12
github.com/opendatahub-io/kserve release-v0.15There was a problem hiding this comment.
please see this comment: #498 (comment)
tldr: I didn't think this was actually valid formatting and might lead to annoying file changes since go wants to replace the release tag with the format I committed
There was a problem hiding this comment.
I was thinking this would just be a new step in the odh release process to update which tag this points to
There was a problem hiding this comment.
If we want to switch to odh, imho, we should do it properly, which means use the right dependency and update the imports.
Also, if we want to move it now, we should be using our 0.15 branch instead of the fixed commit for incubating.
side note, this change might be moved to another PR and discussed.
There was a problem hiding this comment.
can we land this for now and then followup with the proper implementation? This is the actual bug fix for the JIRA linked in the description so we need it for 2.24
There was a problem hiding this comment.
The actual bug in question is caused by the ExtMetricAuthentication type being missing in the available upstream releases. Plus, by not using the ODH kserve releases, I think we are opening ourselves up to more of these bugs in the future as we don't always pull in minor upstream versions (eg: upstream is on release 0.15.2 and we were still using 0.15.0 in go.mod).
There was a problem hiding this comment.
I've pulled the change out to #499 and will rebase it once this pr lands
There was a problem hiding this comment.
The actual bug in question is caused by the ExtMetricAuthentication type being missing in the available upstream releases.
So, why, when you reapply things work?
There was a problem hiding this comment.
The problem is that the odh-model-controller was acting on the object but didn't understand all of its fields, so it was accidentally damaging it before the kserve-controller could do its job correctly. I believe the second apply works because the UPDATE call is less destructive than CREATE, so the odh-model-controller was doing less damage to the object.
There was a problem hiding this comment.
Resolving this thread - let's pick the discussion back up in #499
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
166-167: Global downgrade of controller-runtime to v0.19.1: verify compatibility and align the require for clarity.The replace is justified by the referenced KEDA issue. Two follow-ups:
- Compatibility check: controller-runtime v0.19.x may not officially track k8s v0.33.x; confirm build/tests pass across your supported clusters and no subtle runtime issues arise with client/cache/informer behavior.
- Clarity: you currently require v0.20.4 but replace to v0.19.1. Consider aligning the require to reflect the effective version to reduce confusion for future maintainers.
Suggested tweak to align the require (no functional change due to replace, but clearer intent):
- sigs.k8s.io/controller-runtime v0.20.4 + sigs.k8s.io/controller-runtime v0.19.1
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
go.mod(6 hunks)
🔇 Additional comments (3)
go.mod (3)
3-3: Go toolchain bumped to 1.24.6 — good.Matches the reviewer’s ask to use the latest patch, and aligns with the PR objective to upgrade to Go 1.24.
27-31: Kubernetes ecosystem aligned at v0.33.1.api, apimachinery, apiserver, client-go, apiextensions-apiserver, and component-base are consistently set to v0.33.1. This is the right move to avoid go.mod graph conflicts.
Also applies to: 151-152
168-171: Replace kserve with ODH fork pinned to a pseudo-version — good, reproducible.Using the pseudo-version that resolves odh-v2.33 is expected and preferred for reproducibility since non-SemVer tags are normalized by Go tooling. The inline comment mapping to odh-v2.33 is helpful for maintainers. No changes requested.
hdefazio
changed the title
hdefazio
changed the title
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
hdefazio
force-pushed
the
fix/use_odh_kserve
branch
from
9bb3cc2 to
51c6b0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dev_tools/devspace.yaml (1)
58-58: Dev image tag: verify existence and access; consider aligning with go-toolset base.Good to bump to Go 1.24.2. Please verify that this image/tag exists and is accessible to all contributors. Also consider aligning the dev image with the builder base (ubi9/go-toolset:1.24) for parity and fewer "works-in-dev vs. works-in-CI" discrepancies.
To check the tag on Quay:
#!/bin/bash set -euo pipefail ORG="rh-ee-allausas" REPO="golang" TAG="1.24.2-odh-devspace" curl -fsSL "https://quay.io/api/v1/repository/${ORG}/${REPO}/tag/?limit=200" \ | rg -n "\"name\":\s*\"${TAG}\"" -n -C0 && echo "Tag exists." || { echo "Tag ${ORG}/${REPO}:${TAG} not found or API inaccessible."; exit 1; }If you opt to align with RH go-toolset (example):
- Pros: Matches CI builder toolchain, RH-supported image.
- Cons: You’ll need dev-only tooling (e.g., sshd, debug utils) layered in.
Example alternative value (only if you provide a dev-flavored image for it):
- registry.access.redhat.com/ubi9/go-toolset:1.24
🧹 Nitpick comments (2)
.github/workflows/lint.yml (1)
18-18: Go 1.24 pin looks good; consider sourcing from go.mod for single source of truth.Using '~1.24' is fine. Optionally, rely on go.mod to avoid drift.
Apply this diff if you prefer the go.mod-driven configuration:
- go-version: '~1.24' + go-version-file: 'go.mod' + check-latest: true + cache: truedev_tools/devspace.yaml (1)
105-111: Trailing space and profile clarity.
- Line 106 has trailing whitespace flagged by yamllint.
- Running a Go 1.22 profile while go.mod targets 1.24 may cause local-build mismatches. Keep it if needed, but document the constraint for contributors.
Apply this diff to fix the whitespace:
- - name: go1.22 + - name: go1.22Optionally add a short comment to warn about toolchain mismatch:
- name: go1.22 patches: # Use patches to modify the base configuration + # NOTE: Use only if the codebase does not rely on Go 1.24-specific features. + # go.mod currently targets 1.24; builds may fail or behave differently under 1.22. - op: replace path: dev.app.devImage value: quay.io/rh-ee-allausas/golang:1.22-odh-devspace-debug
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.devcontainer/devcontainer.json(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test-e2e.yml(1 hunks).github/workflows/test.yml(1 hunks)Containerfile(1 hunks)README.md(1 hunks)dev_tools/devspace.yaml(2 hunks)go.mod(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/devcontainer.json
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- .github/workflows/test.yml
- Containerfile
- .github/workflows/test-e2e.yml
- go.mod
🧰 Additional context used
🪛 YAMLlint (1.37.1)
dev_tools/devspace.yaml
[error] 106-106: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/lint.yml (1)
23-24: Confirm golangci-lint v1.64.8 release and Go 1.24 compatibilityWe’ve verified that the v1.64.8 tag exists on GitHub, but:
- Release notes for v1.64.8 don’t explicitly mention Go 1.24 support.
- CI workflows in that tag don’t include a Go 1.24 test matrix.
Please manually confirm that v1.64.8 is compatible with Go 1.24—e.g. review the changelog at https://golangci-lint.run/product/changelog/#1648 or test it against a Go 1.24 project—to avoid potential CI breakages.
Signed-off-by: Hannah DeFazio <[email protected]>
|
lgtm once the open comments are resolved |
Signed-off-by: Hannah DeFazio <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
dev_tools/devspace.yaml (1)
105-110: Optional: add a brief description to the go1.22 profileTo aid future cleanup, consider annotating that this profile is temporary/back-compat only.
Apply this diff to add a comment:
profiles: - - name: go1.22 + - name: go1.22 + # Backward compatibility profile to use Go 1.22 dev image if needed. Consider removing after N releases. patches: # Use patches to modify the base configuration - op: replace path: dev.app.devImage value: quay.io/rh-ee-allausas/golang:1.22-odh-devspace-debuggo.mod (3)
3-3: Usego 1.24instead of patch-specificgo 1.24.4The
godirective usually specifies the language version (major.minor). Pinning a patch here adds churn without practical effect. If you need an exact toolchain, prefer thetoolchaindirective.Apply this diff:
-go 1.24.4 +go 1.24If you intentionally rely on
go 1.24.4behavior, please confirm the Go tool’s acceptance of a patch-levelgodirective across developers’ environments and CI.
27-31: Verify controller-runtime v0.19.1 compatibility with k8s v0.33.1You’re requiring k8s modules at v0.33.1 while replacing controller-runtime down to v0.19.1 due to KEDA issue 6660. That combination may be outside the supported compatibility matrix for controller-runtime and could surface subtle compile/runtime issues when MVS hoists k8s deps to 0.33.x for controller-runtime as well.
- Please confirm a full build and unit/integration tests pass locally and in CI with this combination.
- If possible, consider one of:
- Temporarily aligning k8s.* modules to the version line supported by controller-runtime v0.19.x; or
- Moving to a KEDA version that supports controller-runtime >= v0.20.x, then dropping the replace.
As a minor cleanup to reduce confusion, you can align the require to match the replaced version:
Apply this diff:
- sigs.k8s.io/controller-runtime v0.20.4 + sigs.k8s.io/controller-runtime v0.19.1The replace comment and rationale are helpful—thanks for including them.
Also applies to: 151-152, 162-164
104-105: Optional: align OpenTelemetry module minorsYou’re mixing otel core at 1.35.0 with exporters pinned to 1.33.0. This typically works, but aligning to a single minor across otel packages tends to avoid edge cases.
If feasible, consider bumping exporters to 1.35.x (or holding core at 1.33.x) in a follow-up to keep versions consistent.
Also applies to: 120-121, 119-125
📜 Review details
Configuration used: CodeRabbit UI
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 (2)
dev_tools/devspace.yaml(2 hunks)go.mod(6 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
dev_tools/devspace.yaml
[error] 106-106: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
dev_tools/devspace.yaml (1)
58-58: Default dev image bump to Go 1.24 looks goodThe new devImage points to the 1.24 toolchain variant tailored for devspace. Matches the PR objective and prior discussion.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hdefazio, VedantMahabaleshwarkar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm as the open discussion is moved to #499 |
* Replace the upstream kserve dependency with the latest odh release Signed-off-by: Hannah DeFazio <[email protected]> * Update to go 1.24 Signed-off-by: Hannah DeFazio <[email protected]> * Update golangci-lint to be compatible with go 1.24 Signed-off-by: Hannah DeFazio <[email protected]> * Update devspace to go 1.24 and save go 1.22 as a profile Signed-off-by: Hannah DeFazio <[email protected]> * Remove go toolchain pinned version Signed-off-by: Hannah DeFazio <[email protected]> * Upgrade to go 1.24.6 specifically Signed-off-by: Hannah DeFazio <[email protected]> * Remove odh kserve - will split into new pr Signed-off-by: Hannah DeFazio <[email protected]> * Use go 1.24.4 for compatibility with CI Signed-off-by: Hannah DeFazio <[email protected]> * Update devspace image to match minimum go version Signed-off-by: Hannah DeFazio <[email protected]> --------- Signed-off-by: Hannah DeFazio <[email protected]>
* Replace the upstream kserve dependency with the latest odh release Signed-off-by: Hannah DeFazio <[email protected]> * Update to go 1.24 Signed-off-by: Hannah DeFazio <[email protected]> * Update golangci-lint to be compatible with go 1.24 Signed-off-by: Hannah DeFazio <[email protected]> * Update devspace to go 1.24 and save go 1.22 as a profile Signed-off-by: Hannah DeFazio <[email protected]> * Remove go toolchain pinned version Signed-off-by: Hannah DeFazio <[email protected]> * Upgrade to go 1.24.6 specifically Signed-off-by: Hannah DeFazio <[email protected]> * Remove odh kserve - will split into new pr Signed-off-by: Hannah DeFazio <[email protected]> * Use go 1.24.4 for compatibility with CI Signed-off-by: Hannah DeFazio <[email protected]> * Update devspace image to match minimum go version Signed-off-by: Hannah DeFazio <[email protected]> --------- Signed-off-by: Hannah DeFazio <[email protected]> Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
4 participants
Description
RHOAIENG-32595
Merge criteria:
Summary by CodeRabbit