Skip to content
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

Avoid allocating empty strings in the Env slice #3597

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

jiaqiluo
Copy link
Member

@jiaqiluo jiaqiluo commented Jun 4, 2024

#3587
rancher/rancher#45645

Problem:
Previously, we declared the Env slice with the size of the c.Services.KubeAPI.ExtraEnv field, which leads to empty strings at the beginning of the Env slice because we use Golang's append function to add new elements, and Docker is not happy with that.

Fix:
We fix this issue by declaring the Env variable as an empty slice.
We also enhance the getUniqStringList function to properly trim leading and trailing spaces in each element and to ignore empty strings.
We add unit tests for the getUniqStringList function and update the integration tests.

Dev validation:
Compile the code locally and run rke up succeeds with the following section in cluster.yaml file which failed before the fixing:

services:
  kube-api:
    extra_env:
    - TEST_VAR=test

@jiaqiluo jiaqiluo requested a review from a team as a code owner June 4, 2024 23:40
@jiaqiluo jiaqiluo force-pushed the fix-kubeapi-env branch 2 times, most recently from 1830b16 to 01151b7 Compare June 4, 2024 23:57
Previously, we declared the Env slice with the size of the `c.Services.KubeAPI.ExtraEnv` field, which leads to empty strings at the beginning of the Env slice because we use Golang's append function to add new elements, and Docker is not happy with that.
We fix this issue by declaring the Env variable as an empty slice. We also enhance the `getUniqStringList` function to properly trim leading and trailing spaces in each element and to ignore empty strings. We add unit tests for the `getUniqStringList` function and update the integration tests.
Copy link
Contributor

@jakefhyde jakefhyde left a comment

Choose a reason for hiding this comment

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

LGTM, nice job!

@jakefhyde jakefhyde requested a review from a team June 5, 2024 02:27
@jiaqiluo jiaqiluo merged commit cfc6fcc into rancher:release/v1.6 Jun 7, 2024
1 check passed
@jiaqiluo jiaqiluo deleted the fix-kubeapi-env branch June 7, 2024 16:34
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