Vmcp: Add but do not use app config to the CRD #3203
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
e46aa5c to
7d66da8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
- Coverage 57.24% 57.04% -0.20%
==========================================
Files 347 348 +1
Lines 34485 34608 +123
==========================================
+ Hits 19740 19743 +3
- Misses 13104 13223 +119
- Partials 1641 1642 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
There was a problem hiding this comment.
overall great, some nits inline around the required values and omitempty and questions around error handling.
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
1caf867 to
f26f2c7
Compare
jhrozek
left a comment
There was a problem hiding this comment.
apologies for the delay, I saw the updated PR during the day and thought I'd acked it but forgot
Summary Stacked on #3203 This migrates the group spec field to use the config data. Supports #3125 Changes Delete the legacy groupRef field on the spec. Add kubebuilder annotations Update documentation, examples, and tests to match new schema --------- Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Summary
This PR adds the
vmcpconfig.Configto theVirtualMCPServerSpec, but does not use it. This sets up subsequent work to migrate the existing functionality (e.g. defaults, validation, discovery) built up around the otherVirtualMCPServerSpecfields onto their equivalent fields invmcpconfig.Config.Supports #3125
Large PR Justification
This PR is large in lines-of-code, but is conceptually small. This PR contains the minimal changes to add
config.Configto theVirtualMCPServerSpec.All changes are motivated by errors from
task operator-generate && task operator-manifests && task lint.Changes
Migrate from
anyto a CRD-compatiblejson.Maporjson.Any.The CRD code generation does not support
anytypes in the app config object. The simplest thing to do would be replace all uses withruntime.RawExtension. However, that makes the code that previously depended onmap[string]anyless typesafe and more verbose, since it must unmarshall the raw contents manually. Consequently, I introduced apkg/json/any.gothat is simpler to work with. Specifically, thejson.Mapis a typesafe and CRD-compatiblemap[string]any.Many test files were updated to use this new type rather than
map[string]any.Use
stringrather thanfloatforSamplingRateThis one is straightforward. Sampling rate was previously a float which is unsupported by kubebuilder. This also required regenerating the swagger docs.
Generate DeepCopy methods
Kubebuilder requires the config types have DeepCopy methods. Changes to the taskfile and existing config objects ensures this code is generated.
Next Steps
After this change, I will incrementally migrate the existing
VirtualMCPServerSpecfields to use theirConfigequivalent. These migrations are not necessarily straightforward, so I will be attacking them one-by-one to ensure functionality is not lost or broken during the migration.