-
Notifications
You must be signed in to change notification settings - Fork 880
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
Allow modification of the hostNetwork parameter to avoid the need of … #1046
Conversation
…a dedicated pod ip
Is there any chance this PR can be reviewed by owner? |
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.
Thanks for bringing this up!
In general when adding a new parameter it's best to:
- Set the default in values.yaml, and update values.schema.json
- Add some
helm template
tests with the parameter set and unset (in test/unit/csi-daemonset.bats in this case)
@tvoran Thanks for the review all comment fixed. Please help take a look again. Can you also review similar PR for CSI-Driver : kubernetes-sigs/secrets-store-csi-driver#1581 Thank you |
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 is looking good, just a minor fix in the tests.
test/unit/csi-daemonset.bats
Outdated
local actual=$(helm template \ | ||
--show-only templates/csi-daemonset.yaml \ | ||
. | tee /dev/stderr | |
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 think these will need csi.enabled=true set since by default the csi daemonset is not deployed in the chart.
local actual=$(helm template \ | |
--show-only templates/csi-daemonset.yaml \ | |
. | tee /dev/stderr | | |
local actual=$(helm template \ | |
--show-only templates/csi-daemonset.yaml \ | |
--set 'csi.enabled=true' \ | |
. | tee /dev/stderr | |
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.
done
test/unit/csi-daemonset.bats
Outdated
local actual=$(helm template \ | ||
--show-only templates/csi-daemonset.yaml \ | ||
--set 'csi.hostNetwork=true' \ | ||
. | tee /dev/stderr | |
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.
local actual=$(helm template \ | |
--show-only templates/csi-daemonset.yaml \ | |
--set 'csi.hostNetwork=true' \ | |
. | tee /dev/stderr | | |
local actual=$(helm template \ | |
--show-only templates/csi-daemonset.yaml \ | |
--set 'csi.enabled=true' \ | |
--set 'csi.hostNetwork=true' \ | |
. | tee /dev/stderr | |
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.
done
values.yaml
Outdated
@@ -1119,6 +1119,9 @@ csi: | |||
# generating secret versions. | |||
hmacSecretName: "" | |||
|
|||
#Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip |
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.
nit: formatting
#Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip | |
# Allow modification of the hostNetwork parameter to avoid the need of a | |
# dedicated pod ip |
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.
done
values.schema.json
Outdated
"hostNetwork": { | ||
"type": "boolean" | ||
}, |
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.
nit: These are in alphabetical order, so it would be good to move this to be after hmacSecretName.
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.
done
Sorry, that chart is beyond my purview. |
all comments fixed @tvoran Please help take a look. Thank you |
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.
Thanks!
Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip.
We would like our daemonsets to run in the hostNetwork so to avoid needing allocate IP for them.
Adding hostNetwork to this template allow us to do this.