Provide option to enable volume mode conversion flag to HostPath driver deployment script #379
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/S
RaunakShah
force-pushed
the
add-flag-2
branch
from
166ae93 to
143e46a
Compare
deploy/util/deploy-hostpath.sh
Outdated
| @@ -246,6 +250,10 @@ for i in $(ls ${BASE_DIR}/hostpath/*.yaml | sort); do | |||
| fi | |||
| echo "$line" | |||
| done)" | |||
| if volume_mode_conversion; then | |||
| # Enable volume mode conversion feature flag | |||
| modified="${modified//--prevent-volume-mode-conversion=false/--prevent-volume-mode-conversion=true}" | |||
There was a problem hiding this comment.
Is this line the reason why you need the --prevent-volume-mode-conversion=false in the manifest files?
Then I suggest to either find a way to inject this parameter without relying on special markers in the YAML file (but I understand that this might be hard in pure shell), or use a generic marker like # end of snapshot-controller arguments.
There was a problem hiding this comment.
Yes, I've tried to alter the flow of the script as little as possible. I've added start and end markers here, so that any future feature flags can be added in the same block.
There was a problem hiding this comment.
This is not what I meant 😅
I meant that # end of snapshot-controller arguments should be in the YAML file, and then this script injects the new parameters at that line. That makes the changes to the YAML file more generic and it is more obvious that the line is special and better shouldn't get removed. That was not clear with the additional --prevent-volume-mode-conversion=false.
There was a problem hiding this comment.
Aaaaaah, gotcha! I'll make those changes and test it out
There was a problem hiding this comment.
@pohly made some changes to the scripts and YAMLs. As you suggested, I've added a marker to the yaml and inject the arguments for the container if a variable is set. PTAL!
RaunakShah
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
…t-volume-mode-conversion flag if VOLUME_MODE_CONVERSION_TESTS is set to true in deploy-hostpath.sh
RaunakShah
force-pushed
the
add-flag-2
branch
from
85393ef to
d6470ca
Compare
RaunakShah
changed the title
deploy/util/deploy-hostpath.sh
Outdated
| if volume_mode_conversion; then | ||
| sed -i -e 's/# end csi-provisioner args/- \"--prevent-volume-mode-conversion=true\"\n # end csi-provisioner args/' $i | ||
| fi | ||
| # end of container feature flag arguments |
RaunakShah
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
RaunakShah
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@pohly can we merge this to unblock kubernetes-csi/external-provisioner#832? |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, RaunakShah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR sets
--prevent-volume-mode-conversion=trueflag in external-provisioner ifVOLUME_MODE_CONVERSION_TESTSis set.VOLUME_MODE_CONVERSION_TESTSdefaults to false; a new variable indeploy.shcan enable that feature while deploying the driver.This will be used in sidecar e2e tests in the prow jobs.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: