-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is in if(strings.Contains(observedValue, sdkEnvValue))
so I think it's ok.
This if will be untrue when:
desiredOdigosPart
contains the new value for new SDKobservedValue
contains an old value, which is hit in the iteration on all possible values for any SDKsdkEnvValue
which is still inobservedValue
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, thanks! updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this can be improve to only check if the env vars we care about have changed/added/deleted
No description provided.