Skip to content

Conversation

Len4i
Copy link

@Len4i Len4i commented May 2, 2024

No description provided.

Leonid Podolinskiy added 3 commits May 2, 2024 10:47
AlexanderMarkov
AlexanderMarkov previously approved these changes May 2, 2024
#minor
want: 0,
},
{
name: "correctly finds the index of the env var",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this test represent a scenario where the arg envVarName is not in envVarList and therefore function return -1 , right ? in that case it's still true to name it "correctly finds the index of the env var" ? maybe test envVarName does not exist or something like that

if envVarIndex != -1 {
value := strings.ReplaceAll(container.Env[envVarIndex].Value, patchedEnvValue, "")
if value == "" {
container.Env = append(container.Env[:envVarIndex], container.Env[envVarIndex+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this confused me a bit but with some help of non-human :) I managed to get it , the logic is basically to remove environment variable from container environment variable list by applying technics of slicing all elements before but not include the index of the removed env var and all elements from the element immediately after the one at index removed environment variable to the end.

maybe we can add comment or extract this logic to function name removeEnvVar

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, there is slices.Delete was added in go 1.21
we can use it 👍

@Len4i Len4i merged commit 04d4b21 into main May 2, 2024
@Len4i Len4i deleted the DEVOPS-831-fix-env-var-cleanup-upon-change branch May 2, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants