Fix runtime_config API semantics and validation#4435
Fix runtime_config API semantics and validation#4435slyt3 wants to merge 8 commits intostacklok:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b498de6ad
ℹ️ 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".
|
The merge conflicts must be solved. |
8b498de to
fa5aa22
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4435 +/- ##
==========================================
+ Coverage 69.57% 69.68% +0.11%
==========================================
Files 489 494 +5
Lines 50193 50523 +330
==========================================
+ Hits 34920 35209 +289
- Misses 12590 12618 +28
- Partials 2683 2696 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@JAORMX Yeah I know I was rebasing and resolving conflicts; branch is now cleanly mergeable. I’m fixing the remaining docgen check now. |
cdf4b43 to
8327428
Compare
|
@JAORMX Hi! This PR should be ready, one E2E test is failing but seems unrelated; I’d really appreciate your review when you have time, thank you :) |
|
@slyt3 I'll be unavailable this week as I'm flying to MCP Dev Summit. Perhaps you can reach out in Discord and another maintainer can check this out? |
|
@JAORMX Got it, thanks for letting me know! Safe travels to the MCP Dev Summit. I will reach out on Discord and see if another maintainer can take a look. |
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for the PR and for your patience through the review cycles — the scoping improvements over #3772 are clear (API-only merge semantics, input validation, protocol-scheme enforcement). Ozz's original feedback has been well addressed.
Requesting changes for the mixed-in behavioral changes that make the runtime_config additions harder to review in isolation. See inline comments.
|
@jerm-dro i removed the unrelated changes from that PR and clarified the runtime_config API contract in the docs/comments |
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for sticking with this through the review cycles and for cleanly scoping the changes to the API layer.
Summary
Reopening closed PR #3772 and carries over the original
runtime_configworkload API work with the review feedback addressed.The previous version changed runtime config merge behavior for all callers, including CLI, and allowed some invalid
runtime_configcases to be ignored or fail later during image build. This update keeps the behavior scoped to the workload API, adds validation, and tightens the response/update round-trip behavior.Fixes #3676
Type of change
Test plan
task test)task test-e2e)task lint-fix)Manual testing:
go test ./pkg/runner -run 'TestLoadRuntimeConfig_' -count=1go test ./pkg/api/v1 -run 'Test(RuntimeConfigFromRequest|RuntimeConfigForImageBuild|CreateWorkload|UpdateWorkload)$' -count=1go test ./pkg/api/v1 -run 'Test(RuntimeConfigForImageBuild|RunConfigToCreateRequest)$' -count=1Changes
pkg/runner/protocol.gopkg/runner/protocol_test.gopkg/api/v1/workload_service.goruntime_configpkg/api/v1/workload_service_test.gopkg/api/v1/workload_types.goruntime_configfrom non-protocol response payloadspkg/api/v1/workloads_test.goruntime_configbehaviorpkg/api/v1/workloads_types_test.godocs/server/docs.godocs/server/swagger.jsondocs/server/swagger.yamlDoes this introduce a user-facing change?
Yes. Invalid runtime_config input now fails early with a clear API validation error, and runtime_config is only accepted on create/update when image is a protocol URI.
Special notes for reviewers
This is intended as the replacement for #3772, not a separate feature. The main goal is to keep the runtime-config behavior scoped to the workload API without changing shared runner/CLI semantics.