-
Notifications
You must be signed in to change notification settings - Fork 154
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
Copy volumeSnapshotClass parameters when cloning #1128
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.
Overall looks good
}, | ||
} | ||
} | ||
|
||
func Mss2msi(in map[string]string) map[string]interface{} { |
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.
Can you please add comment to the function? It will help understand its purpose.
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.
Added a comment to clarify
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
@@ -63,7 +63,7 @@ func cloneSnapshotClass(ctx context.Context, dynCli dynamic.Interface, snapClass | |||
for _, key := range excludeAnnotations { | |||
delete(existingAnnotations, key) | |||
} | |||
usNew := UnstructuredVolumeSnapshotClass(snapClassGVR, targetClassName, sourceSnapClass.Driver, newDeletionPolicy) | |||
usNew := UnstructuredVolumeSnapshotClass(snapClassGVR, targetClassName, sourceSnapClass.Driver, newDeletionPolicy, sourceSnapClass.Parameters) |
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.
Hi @bathina2,
By introducing this argument it looks like we are assuming params
field will always be there in the VolumeSnapshotClass
. But I have seen some examples where params field is not present in the VoluemSnapshotClass
resoruce, and because of that when we try to create the copied VolumeSnapshotClass
with nil
as parameter value VolumeSnapshotClass Creation fails with below error
"k10-clone-csi-hostpath-snapclass" is invalid: parameters: Invalid value: "null": parameters in body must be of type object: "null"
I was thinking we shouldn't pass the parameters field if the values is nil.
While cloning `VolumeSnapshotClass`, as part of #1128 we started passing params. But we should not pass params if the values is `nil`, because in that case creation `VolumeSnapshotClass` fails with `nil` not allowed.
While cloning `VolumeSnapshotClass`, as part of #1128 we started passing params. But we should not pass params if the values is `nil`, because in that case creation `VolumeSnapshotClass` fails with `nil` not allowed.
…class (#1145) * Don't use params field while cloning volumesnapshotclass While cloning `VolumeSnapshotClass`, as part of #1128 we started passing params. But we should not pass params if the values is `nil`, because in that case creation `VolumeSnapshotClass` fails with `nil` not allowed. * Refactor, address review comments Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Change Overview
Copy parameters when cloning.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan