Conversation
Allow runtime_config in create and update workload API requests. Wire runtime overrides through image retrieval and persist them in RunConfig. Update API docs and tests for stacklok#3676. Signed-off-by: Mantas Suminas <mantas.sumin@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97e8aa6d9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
==========================================
- Coverage 66.57% 66.55% -0.02%
==========================================
Files 432 432
Lines 42215 42246 +31
==========================================
+ Hits 28103 28117 +14
- Misses 11970 11990 +20
+ Partials 2142 2139 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Avoid invalid protocol Dockerfiles when runtime override omits builder_image by merging missing fields with base runtime config. Add tests for merged behavior and override precedence. Signed-off-by: slyt3 <mantas.sumin@gmail.com>
Trim builder_image and ignore empty runtime_config payloads to avoid forwarding meaningless overrides.
github-actions
bot
added
size/S
Add explicit returns after fatal nil guards so staticcheck can prove dereferences are safe.
github-actions
bot
added
size/S
Signed-off-by: slyt3 <mantas.sumin@gmail.com>
github-actions
bot
added
size/S
|
I fixed the failing verify check separately: docs/server/swagger.json had an EOF newline mismatch; this is docs formatting only, not part of the #3676 runtime_config logic. |
github-actions
bot
added
size/S
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e6e895eee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Append additional_packages to base runtime defaults and filter blank package entries in API requests to avoid malformed build config.
github-actions
bot
added
size/S
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Thanks for working on this! The feature itself makes sense and the test coverage for runtimeConfigFromRequest is solid. A few things to address before merging:
-
loadRuntimeConfigmerge semantic is a breaking change: This changes behavior for all callers (CLI included), not just the API. The append-to-base semantic forAdditionalPackagesmeans CLI users of--runtime-add-packageget different behavior than before. Consider scoping this to the API path only, or splitting the merge-semantic change into its own PR. -
Input validation for
runtime_configfields:AdditionalPackagesvalues end up interpolated intoRUNshell commands in Dockerfile templates (apk add,apt-get install).BuilderImagegoes intoFROMdirectives. Neither is validated beyond whitespace trimming. We should add validation before merging (I'll handle this separately). -
Test style in
protocol_test.go: Usest.Fatal/reflect.DeepEqualinstead of testify. The rest of the PR's tests use testify correctly. -
Silent ignore for non-protocol images: When
runtime_configis passed with a regular Docker image, nothing happens and there's no feedback to the user.
See inline comments for details.
| // Resolve base config from user configuration or defaults. | ||
| baseConfig := getBaseRuntimeConfig(transportType) | ||
| if override == nil { | ||
| return baseConfig | ||
| } | ||
|
|
||
| // Merge overrides into base config so omitted fields inherit sane defaults. | ||
| merged := &templates.RuntimeConfig{ | ||
| BuilderImage: baseConfig.BuilderImage, | ||
| AdditionalPackages: append([]string{}, baseConfig.AdditionalPackages...), | ||
| } | ||
|
|
||
| if strings.TrimSpace(override.BuilderImage) != "" { | ||
| merged.BuilderImage = strings.TrimSpace(override.BuilderImage) | ||
| } | ||
| if len(override.AdditionalPackages) > 0 { | ||
| merged.AdditionalPackages = append(merged.AdditionalPackages, override.AdditionalPackages...) | ||
| } | ||
|
|
||
| return merged | ||
| } | ||
|
|
||
| func getBaseRuntimeConfig(transportType templates.TransportType) *templates.RuntimeConfig { | ||
| // Try loading from user config | ||
| provider := config.NewProvider() | ||
| if userConfig, err := provider.GetRuntimeConfig(string(transportType)); err == nil && userConfig != nil { | ||
| return userConfig | ||
| return &templates.RuntimeConfig{ | ||
| BuilderImage: userConfig.BuilderImage, | ||
| AdditionalPackages: append([]string{}, userConfig.AdditionalPackages...), | ||
| } | ||
| } | ||
|
|
||
| // Fall back to defaults | ||
| defaultConfig := templates.GetDefaultRuntimeConfig(transportType) | ||
| return &defaultConfig | ||
| return &templates.RuntimeConfig{ | ||
| BuilderImage: defaultConfig.BuilderImage, | ||
| AdditionalPackages: append([]string{}, defaultConfig.AdditionalPackages...), |
There was a problem hiding this comment.
This changes the merge semantic from full-replacement to append-with-base for all callers (CLI included), not just the API path. Before this PR, passing an override replaced everything; now AdditionalPackages are appended to base defaults and BuilderImage falls back to base if empty.
This is a breaking behavioral change for CLI users of --runtime-add-package -- they previously got exactly what they specified, now they get base packages plus their additions with no way to subtract.
A couple of options:
- Keep full-replacement semantics here and let the API layer handle merging if that's desired.
- If merge is the intended behavior for all paths, this should be a separate, clearly documented PR since it changes existing CLI behavior.
Also: there's double-trimming of BuilderImage -- runtimeConfigFromRequest already trims whitespace at the API boundary, and then loadRuntimeConfig trims again at line 173. Pick one place to trim (prefer the API boundary) and trust internal code receives clean input.
| // Runtime overrides only apply to protocol-scheme image builds. | ||
| if runtimeConfigOverride != nil && req.URL == "" { | ||
| options = append(options, runner.WithRuntimeConfig(runtimeConfigOverride)) | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment says "Runtime overrides only apply to protocol-scheme image builds" but if a user passes runtime_config with a regular Docker image (not go://, npx://, uvx://), no warning or error is returned -- it's silently ignored. This will confuse users who expect it to have an effect.
Consider returning a validation error (or at least a warning in the response) when runtime_config is provided alongside a non-protocol-scheme image.
|
|
||
| func TestLoadRuntimeConfig_MergesMissingOverrideFields(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| base := loadRuntimeConfig(templates.TransportTypeGO, nil) | ||
| if base == nil { | ||
| t.Fatal("loadRuntimeConfig returned nil base config") | ||
| return | ||
| } | ||
| if base.BuilderImage == "" { | ||
| t.Fatal("base runtime config has empty builder image") | ||
| } | ||
|
|
||
| override := &templates.RuntimeConfig{ | ||
| AdditionalPackages: []string{"curl"}, | ||
| } | ||
| got := loadRuntimeConfig(templates.TransportTypeGO, override) | ||
| if got == nil { | ||
| t.Fatal("loadRuntimeConfig returned nil merged config") | ||
| return | ||
| } | ||
|
|
||
| // Missing builder image in override should inherit the base builder image. | ||
| if got.BuilderImage != base.BuilderImage { | ||
| t.Fatalf("BuilderImage = %q, want base %q", got.BuilderImage, base.BuilderImage) | ||
| } | ||
|
|
||
| // Additional packages should be appended to base defaults. | ||
| expectedPackages := append([]string{}, base.AdditionalPackages...) | ||
| expectedPackages = append(expectedPackages, "curl") | ||
| if !reflect.DeepEqual(got.AdditionalPackages, expectedPackages) { | ||
| t.Fatalf("AdditionalPackages = %v, want %v", got.AdditionalPackages, expectedPackages) | ||
| } | ||
|
|
||
| // Ensure merged config is detached from input slices. | ||
| override.AdditionalPackages[0] = "git" | ||
| if got.AdditionalPackages[len(got.AdditionalPackages)-1] != "curl" { | ||
| t.Fatalf("AdditionalPackages mutated via override input: got %v", got.AdditionalPackages) | ||
| } | ||
| } | ||
|
|
||
| func TestLoadRuntimeConfig_UsesOverrideBuilderImage(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| customImage := "golang:1.24-alpine" | ||
| got := loadRuntimeConfig(templates.TransportTypeGO, &templates.RuntimeConfig{ | ||
| BuilderImage: customImage, | ||
| }) | ||
| if got == nil { | ||
| t.Fatal("loadRuntimeConfig returned nil config") | ||
| return | ||
| } | ||
| if got.BuilderImage != customImage { | ||
| t.Fatalf("BuilderImage = %q, want %q", got.BuilderImage, customImage) | ||
| } | ||
| } |
There was a problem hiding this comment.
These tests use t.Fatal/t.Fatalf and reflect.DeepEqual instead of testify assertions (assert.Equal, require.NotNil, etc.). The other test files in this same PR (workload_service_test.go, workloads_types_test.go) correctly use testify, so let's keep things consistent here.
For example:
// Instead of:
if got.BuilderImage != base.BuilderImage {
t.Fatalf("BuilderImage = %q, want base %q", got.BuilderImage, base.BuilderImage)
}
// Use:
assert.Equal(t, base.BuilderImage, got.BuilderImage)And reflect.DeepEqual can be replaced with assert.Equal.
| { | ||
| name: "with runtime config override", | ||
| requestBody: `{"name": "test-workload", "image": "go://github.com/example/server", "runtime_config": {"builder_image": "golang:1.24-alpine", "additional_packages": ["ca-certificates"]}}`, | ||
| setupMock: func(_ *testing.T, wm *workloadsmocks.MockManager, _ *runtimemocks.MockRuntime, gm *groupsmocks.MockManager) { | ||
| wm.EXPECT().DoesWorkloadExist(gomock.Any(), "test-workload").Return(false, nil) | ||
| gm.EXPECT().Exists(gomock.Any(), "default").Return(true, nil) | ||
| wm.EXPECT().RunWorkloadDetached(gomock.Any(), gomock.Any()). | ||
| DoAndReturn(func(_ context.Context, runConfig *runner.RunConfig) error { | ||
| assert.NotNil(t, runConfig.RuntimeConfig) | ||
| assert.Equal(t, "golang:1.24-alpine", runConfig.RuntimeConfig.BuilderImage) | ||
| assert.Equal(t, []string{"ca-certificates"}, runConfig.RuntimeConfig.AdditionalPackages) | ||
| return nil | ||
| }) | ||
| }, | ||
| expectedRuntimeConfig: &templates.RuntimeConfig{ | ||
| BuilderImage: "golang:1.24-alpine", | ||
| AdditionalPackages: []string{"ca-certificates"}, | ||
| }, | ||
| expectedServerOrImage: "go://github.com/example/server", | ||
| expectedStatus: http.StatusCreated, | ||
| expectedBody: "test-workload", | ||
| }, | ||
| { | ||
| name: "with tool filters", |
There was a problem hiding this comment.
Good test for the happy path! A few edge cases are missing that would strengthen coverage:
- Empty
runtime_config({}): Verify it doesn't override defaults (i.e.,runtimeConfigFromRequestreturns nil). runtime_configwith a non-protocol-scheme image: e.g.,"image": "nginx:latest"+runtime_configset. Verify the behavior is well-defined (currently silently ignored).- Update path round-trip: Update a workload without specifying
runtime_configand verify whether a previously stored config is preserved or cleared. This matters for UX since users may not expect an update to drop their runtime overrides.
|
@JAORMX thankyouuuu for taking time to look into this, gonna fix those issues! |
2 participants
I added
runtime_configto workload to create and update API requests for #3676; It now passes the runtime image override into the build/retriever flow and stores it inRunConfig, updated API docs and tests