refactor: env overwriter #1297
refactor: env overwriter #1297
Conversation
| // action: we want to keep the user defined values and upsert the odigos value. | ||
| for _, sdkEnvValue := range envMetadata.values { | ||
| if strings.Contains(observedValue, sdkEnvValue) { | ||
| if sdkEnvValue == desiredOdigosPart { |
There was a problem hiding this comment.
In the start we do
desiredOdigosPart, ok := envMetadata.values[currentSdk]
and here sdkEnvValue is taken from the same map.
So this internal if looks like it will always be true if we reach it?
We are also checking for equality in scenario 2, so I'm not sure about this part
There was a problem hiding this comment.
This part is in if(strings.Contains(observedValue, sdkEnvValue)) so I think it's ok.
This if will be untrue when:
desiredOdigosPartcontains the new value for new SDKobservedValuecontains an old value, which is hit in the iteration on all possible values for any SDKsdkEnvValuewhich is still inobservedValueis the old value- then
sdkEnvValue != desiredOdigosPart
| // The key is the environment variable name. | ||
| // The value is either the original value of the environment variable or nil if the environment variable | ||
| // was not set by the user. | ||
| type OriginalEnv map[string]*string |
There was a problem hiding this comment.
If the env var was not set by the user I think we should not create an entry for it.
There was a problem hiding this comment.
We create an entry with nil so that we know to delete it when reverting. Otherwise it's not straight forward to know if an arbitrary environment variable should be deleted from manifest (injected by odigos) or if it was there to begin with and should be untouched.
There was a problem hiding this comment.
We used the annotation for that, right? If an env var is in the map it means we appended to its value in the manifest, if it's not it is not taken from the manifest.
This nil approach will add a lot of odigos data on deployments and I don't think it is required.
| } | ||
| for j := range oldPodSpec.Spec.Containers[i].Env { | ||
| if oldPodSpec.Spec.Containers[i].Env[j].Value != newPodSpec.Spec.Containers[i].Env[j].Value { | ||
| return true |
There was a problem hiding this comment.
Do we need to check the env name before checking the value?
If we can check only the env vars we care about using some util function from envOverwriter we can decrease the amount of reconciles
There was a problem hiding this comment.
good point, thanks! updated
There was a problem hiding this comment.
I still think this can be improve to only check if the env vars we care about have changed/added/deleted
No description provided.