feat(controller): make Go (ADK) runtime agent image independently configurable#2021
feat(controller): make Go (ADK) runtime agent image independently configurable#2021QuentinBisson wants to merge 9 commits into
Conversation
3e0ab42 to
10a9c6f
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Go (ADK) runtime image configurability so operators can override registry/repository/pull policy independently from the default (Python) agent image, while preserving the existing derived-repo behavior.
Changes:
- Introduces
controller.goAgentImageHelm values and wires them into the controller ConfigMap asGO_IMAGE_*settings. - Adds controller flags for Go runtime image overrides and a
DefaultGoImageConfigto drive translation. - Updates translator logic + adds unit tests covering explicit Go repo/registry overrides and fallback derivation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| helm/kagent/values.yaml | Adds controller.goAgentImage values with fallback/derivation semantics. |
| helm/kagent/templates/controller-configmap.yaml | Exposes Go image override values via GO_IMAGE_* entries when set. |
| go/core/pkg/app/app.go | Adds --go-image-* flags to set Go-specific image defaults. |
| go/core/internal/controller/translator/agent/adk_api_translator.go | Introduces DefaultGoImageConfig global for Go runtime image overrides. |
| go/core/internal/controller/translator/agent/deployments.go | Applies Go-specific registry/pullPolicy overrides and optional repository override logic. |
| go/core/internal/controller/translator/agent/runtime_test.go | Adds tests for explicit Go repo/registry overrides and fallback derivation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| commandLine.StringVar(&agent_translator.DefaultGoImageConfig.Registry, "go-image-registry", "", "The registry to use for the Go (ADK) runtime agent image. When empty, falls back to --image-registry.") | ||
| commandLine.StringVar(&agent_translator.DefaultGoImageConfig.Repository, "go-image-repository", "", "The repository to use for the Go (ADK) runtime agent image. When empty, derived from --image-repository by replacing the last path segment with \"golang-adk\".") | ||
| commandLine.StringVar(&agent_translator.DefaultGoImageConfig.PullPolicy, "go-image-pull-policy", "", "The pull policy to use for the Go (ADK) runtime agent image. When empty, falls back to --image-pull-policy.") |
There was a problem hiding this comment.
Fixed: the flags now use the DefaultGoImageConfig fields as StringVar defaults, same as the skills-init-image-* flags. Defaults are encoded in the config since the derivation fallback was dropped.
| img := deployment.Spec.Template.Spec.Containers[0].Image | ||
| assert.Contains(t, img, "/kagent-golang-adk@", "Image should use the explicit flat repository") | ||
| assert.NotContains(t, img, "/golang-adk@", "Derived path must not appear when repository is explicitly set") | ||
| assert.Contains(t, img, "@sha256:test-go-base") |
There was a problem hiding this comment.
Fixed: the tests now assert equality on the full image reference (e.g. my-registry.io/kagent-golang-adk:v0.0.0-test) instead of substring matching.
| // Resolve base registry and pull policy; Go runtime uses DefaultGoImageConfig fields | ||
| // when set, falling back to DefaultImageConfig for any unset field. | ||
| baseRegistry := DefaultImageConfig.Registry | ||
| basePullPolicy := DefaultImageConfig.PullPolicy | ||
| if runtime == v1alpha2.DeclarativeRuntime_Go { | ||
| if DefaultGoImageConfig.Registry != "" { | ||
| baseRegistry = DefaultGoImageConfig.Registry | ||
| } | ||
| if DefaultGoImageConfig.PullPolicy != "" { | ||
| basePullPolicy = DefaultGoImageConfig.PullPolicy | ||
| } | ||
| } |
There was a problem hiding this comment.
Added TestRuntime_GoImageConfig_ExplicitPullPolicy: asserts the Go deployment uses the explicit pull policy and the Python deployment does not inherit it.
…figurable
Adds DefaultGoImageConfig (registry, repository, pullPolicy) with per-field
fallback to DefaultImageConfig and the existing golang-adk derivation logic.
Exposed as --go-image-{registry,repository,pull-policy} flags (auto-mapped to
GO_IMAGE_* env vars via LoadFromEnv) and as controller.goAgentImage.* Helm values.
Fixes registry layouts where the Python repository is a flat name that cannot
produce a valid path via last-segment replacement (e.g. kagent-golang-adk).
Tag is not configurable — Go runtime images are digest-pinned at link time.
Closes kagent-dev#2018
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
ac8e81a to
b54ae33
Compare
| // the derived repository (last segment of DefaultImageConfig.Repository replaced with | ||
| // "golang-adk") when Repository is empty. Tag is unused — Go runtime images are | ||
| // digest-pinned at controller link time via GoADKImageDigest / GoADKFullImageDigest. | ||
| var DefaultGoImageConfig = ImageConfig{} |
There was a problem hiding this comment.
Why don't we just encode the defaults into this variable like we do for the other image configs?
There was a problem hiding this comment.
Hey @iplay88keys I wanted to preserve the issue's framing.
The empty default was there to avoid a breaking change: the controller currently derives the Go image from agentImage.repository, so anyone mirroring images only sets that one value and the Go image follows. If we hardcode the default and always emit GO_IMAGE_REPOSITORY, their next helm upgrade silently repoints the Go image back to cr.kagent.dev and pods fail to pull.
I do agree that this is not the better change though and I will replace this PR with the way it is done for the other images.
There was a problem hiding this comment.
I see, is there another workaround we can build in? Maybe something like: if they are overriding the image for the python image, but not for the go image, throw an error/warning?
There was a problem hiding this comment.
I think we should be able to add a log warning if the 2 registries are different yes
There was a problem hiding this comment.
@EItanya thoughts on the breaking change aspect? I'm thinking it's fine if we document it and we'd likely want that to go in on a minor release to draw attention to the breaking nature of that change.
There was a problem hiding this comment.
I agree with @iplay88keys suggestion, this would likely go into the next minor release where we will make go runtime by default and it would be helpful to be able to configure go runtime images. So I think the breaking change would be fine as it's more clear in the long run
There was a problem hiding this comment.
Agreed, we can put it in 0.10.x
|
This pull request has been marked as stale because of no activity in the last 15 days. It will be closed in the next 5 days unless it is tagged "no stalebot" or other activity occurs. |
Encode concrete defaults into DefaultGoImageConfig (registry, repository, pullPolicy) like the other image configs, and drop the repository derivation from the Python image. The Helm chart always emits GO_IMAGE_* and defaults the registry to the global registry, so mirroring through .Values.registry covers the Go runtime image too. The controller warns at startup when the Go and agent image registries differ. Signed-off-by: QuentinBisson <quentin@giantswarm.io>
iplay88keys
left a comment
There was a problem hiding this comment.
There are a few changes that will need to be made to work with main. The biggest is that regular declarative agents are now referenced by tag and digests are only used for sandbox agents.
| // Tag is unused: Go runtime images are digest-pinned at controller link time | ||
| // via GoADKImageDigest / GoADKFullImageDigest. | ||
| var DefaultGoImageConfig = ImageConfig{ | ||
| Registry: "cr.kagent.dev", |
There was a problem hiding this comment.
This should be changed to ghcr.io
There was a problem hiding this comment.
Done. Rebased on main and moved the defaults to ghcr.io/kagent-dev/kagent/golang-adk. Also adapted to #2242: regular Go agents are now tag-referenced (new tag field on goAgentImage, --go-image-tag, GO_IMAGE_TAG), sandbox agents still pin by digest.
Adapts to kagent-dev#2242 (declarative agents referenced by tag, digests only for sandbox agents): - DefaultGoImageConfig defaults to ghcr.io/kagent-dev/kagent/golang-adk and now carries a Tag (regular Go agents are tag-referenced) - resolveRuntimeImage takes the tag as a parameter so the Go runtime resolves via DefaultGoImageConfig.Tag - new --go-image-tag flag, controller.goAgentImage.tag Helm value and GO_IMAGE_TAG ConfigMap key (coalesced with global tag / Chart version) - tests updated from digest to tag expectations; drops the upstream derivation test superseded by explicit Go image config
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
Fixes #2018.
What
Adds
DefaultGoImageConfig(registry, repository, tag, pullPolicy) so the Go (ADK) runtime agent image is configured independently instead of being derived from the Python repository by string surgery. Exposed end-to-end as:--go-image-{registry,repository,tag,pull-policy}controller flags (auto-mapped toGO_IMAGE_*env vars viaLoadFromEnv)controller.goAgentImage.{registry,repository,tag,pullPolicy}Helm valuesGO_IMAGE_*ConfigMap keys, following the same defaulting asagentImage/skillsInitImage(registry/pullPolicy default to the globals, tag coalesces with the global tag then the Chart version)Defaults are encoded in the variable (
ghcr.io/kagent-dev/kagent/golang-adk), matchingDefaultImageConfigandDefaultSkillsInitImageConfig. Since #2242, regular declarative agents reference the image by tag; sandbox agents still pin by digest viaGoADKImageDigest/GoADKFullImageDigest. The-fullvariant selection is unchanged for both paths.Why
When
spec.declarative.runtime: gois used, the controller derived the Go runtime repository from the Python repository by replacing the last path segment withgolang-adk. This breaks for flat-name registry layouts (e.g. a mirrored registry where the image ismy-registry.io/kagent-golang-adk, which the derivation cannot produce).Breaking change
Operators who mirror images and only set
agentImageno longer get a derived Go image location; they must setcontroller.goAgentImageas well. The controller logs a startup warning when the two registries differ so a misconfigured mirror is visible before a Go agent fails to pull. As discussed in the review thread, this targets a minor release (0.10.x).Files changed
go/core/internal/controller/translator/agent/adk_api_translator.goDefaultGoImageConfigwith encoded defaultsgo/core/internal/controller/translator/agent/deployments.goDefaultGoImageConfig; tag threaded throughresolveRuntimeImagego/core/pkg/app/app.go--go-image-*flags + registry-mismatch startup warninghelm/kagent/values.yamlcontroller.goAgentImageblockhelm/kagent/templates/controller-configmap.yamlGO_IMAGE_*keysgo/core/internal/controller/translator/agent/runtime_test.go,imageconfig_test.go