-
Notifications
You must be signed in to change notification settings - Fork 205
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
handle stringification the same way for all vpc-cni configuration pro… #1048
handle stringification the same way for all vpc-cni configuration pro… #1048
Conversation
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.
let me validate the e2e test and will merge after the minor code quality issues are addressed
lib/addons/vpc-cni/index.ts
Outdated
ConvertPropertiesToString(helmValues[key]); | ||
} | ||
else if (typeof helmValues[key] !== 'string'){ | ||
helmValues[key] = JSON.stringify(helmValues[key]) |
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.
github action issue missing semicolon here and line 410
/do-e2e-tests |
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.
end to end tests failed. A maintainer can provide more details.
e2e test failure unrelated to the changes. Will be addressed in a separate PR. |
/do-e2e-tests |
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.
end to end tests passed
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.
LGTM, thank you!
A finding of #1044 was that the way the configuration properties for the vpc-cni plugin are converted to strings is not handled in the same way for all properties.
Configuration for the vpc-cni core addon is defined as following structure:
Before that configuration is passed to the core addon undefined properties are removed and non-string properties are converted into strings by calling JSON.stringify().
For the "env" and "init.env" subobjects this is done in line 456 and 461 in a loop that iterates over all properties of this child objects, for the properties of the main object however JSON.stringify() is called when assigning the values in line 469 and 470.
This pull request changes that in a way that there is a recursive function "ConvertPropertiesToString" that handles the functionality described above by iterating over the whole configuration object including the embeded subobjects.
I verified that this code change does not introduce any functional changes by comparing the generated cloud formation output for different sets of parameters generated with and without this change and assure they are equal.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.