Add snapshot parameter flag #236
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @wnxn. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
|
/assign @pohly PTAL |
|
/assign @pohly |
pkg/sanity/sanity.go
Outdated
| } | ||
| err = yaml.Unmarshal(yamlFile, &reqConfig.TestVolumeParameters) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("error unmarshaling yaml: %v", err)) | ||
| } | ||
| } | ||
|
|
||
| snapParamPath := reqConfig.TestSnapshotParametersFile | ||
| if len(snapParamPath) != 0 { |
There was a problem hiding this comment.
Instead copying the unmarshaling code from the reqConfig.TestVolumeParametersFile handling above, can you move it into a helper function?
There was a problem hiding this comment.
Thanks for your review. I will amend my commit.
pkg/sanity/sanity.go
Outdated
| TestVolumeExpandSize int64 | ||
| TestVolumeParametersFile string | ||
| TestVolumeParameters map[string]string | ||
| TestSnapshotParametersFile string |
There was a problem hiding this comment.
Can you add the new fields in a separate block (i.e. don't mix with TestVolumeExpand*) and add documentation? I know it's missing also for the other fields, but that's not a good example to follow 🤷♂️
wnxn
force-pushed
the
add-snapshot-parameter-flag
branch
from
6f14761
to
b1cb447
Compare
pkg/sanity/sanity.go
Outdated
| @@ -357,6 +354,22 @@ func loadSecrets(path string) (*CSISecrets, error) { | |||
| return &creds, nil | |||
| } | |||
|
|
|||
| // loadMap reads a map from given file path. | |||
| func loadMap(path string) map[string]string { | |||
There was a problem hiding this comment.
I think you can make this more generic:
func loadFromFile(from string, to interface{}) {
if from != "" {
...
err = yaml.Unmarshal(yamlFile, to)
...
}
}
Then call it with:
loadFromFile(reqConfig.TestSnapshotParametersFile, &reqConfig.TestSnapshotParameters)
Untested, but I think it'll work, and then we have something that could also become useful for populating other data structures.
There was a problem hiding this comment.
OK, I will update and test it.
There was a problem hiding this comment.
Hi @pohly , The method is worked and I updated this PR. Would you please review again?
Signed-off-by: Xin Wang <wileywang@yunify.com>
wnxn
force-pushed
the
add-snapshot-parameter-flag
branch
from
b1cb447
to
e0947e1
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, wnxn 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
…driver-host-path_version Bump csi-driver-host-path version up to v1.12.0
Signed-off-by: Xin Wang wileywang@yunify.com
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #235
Special notes for your reviewer:
Does this PR introduce a user-facing change?: