Replace custom EnvVar with corev1.EnvVar#4570
Replace custom EnvVar with corev1.EnvVar#4570anveshthakur wants to merge 4 commits intostacklok:mainfrom
Conversation
anveshthakur
requested review from
ChrisJBurns,
JAORMX,
jerm-dro,
jhrozek and
yrobla
as code owners
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for the PR! The type migration is clean and the test updates are thorough.
question: EmbeddingServerSpec.Env (at embeddingserver_types.go:76) still uses the custom EnvVar type, which prevents deleting it as #4547 suggests. Should it be included in this PR?
|
|
||
| // convertEnvVarsFromMCPServer converts MCPServer environment variables to builder format | ||
| func convertEnvVarsFromMCPServer(envs []mcpv1alpha1.EnvVar) map[string]string { | ||
| func convertEnvVarsFromMCPServer(envs []corev1.EnvVar) map[string]string { |
There was a problem hiding this comment.
blocker: The issue (#4547) notes that controller conversion logic should be simplified now that the types match directly. This function converts to map[string]string which is fine for the RunConfig path (JSON config file). But the same Name/Value-only copy pattern exists at mcpserver_controller.go:1142 and :1734, where it builds a new corev1.EnvVar{Name: envVar.Name, Value: envVar.Value} from what is already a corev1.EnvVar — silently dropping ValueFrom when setting env vars on the proxy Deployment pod spec.
Those two locations should be simplified to:
env = append(env, m.Spec.ResourceOverrides.ProxyDeployment.Env...)Without this, valueFrom references on proxy env vars won't propagate to the pod spec.
There was a problem hiding this comment.
Good Catch ! Totally missed these
Made changes for this.
#4570 didn't mention this location |
2 participants
Summary
Fixes #
#4547
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers