-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add support for Multiple VolumeSnapshotClasses #178
Conversation
Signed-off-by: Anshul Ahuja <[email protected]>
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 comments in internal/util/util.go
Signed-off-by: Anshul Ahuja <[email protected]>
CSIDeleteSnapshotSecretNamespace = "velero.io/csi-deletesnapshotsecret-namespace" | ||
CSIVSCDeletionPolicy = "velero.io/csi-vsc-deletion-policy" | ||
VolumeSnapshotClassSelectorLabel = "velero.io/csi-volumesnapshot-class" | ||
VolumeSnapshotClassDriverBackupAnnotationPrefix = "velero.io/csi-volumesnapshot-class" |
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.
Looks like VolumeSnapshotClassDriverBackupAnnotationPrefix
and is duplicated with VolumeSnapshotClassDriverPVCAnnotation
.
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.
Actually all 3 variables including VolumeSnapshotClassSelectorLabel have the same string
I kept that intentionally same to make user experience easier
While on the other hand - all are separate 3 variables such that it is easy to change in code than reusing the variables.
Do you have any suggestion for keeping the values different?
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 agree that these are logically different parameters. I think it's fine that they have the same value, since they're used in different contexts -- i.e. a PVC annotation shouldn't need to be called "csi-volumesnapshot-class-pvc" since it's already on the PVC.
CC @shubham-pampattiwar for review |
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 some comments. We can't merge this until we get the design PR approved and merged, though.
CSIDeleteSnapshotSecretNamespace = "velero.io/csi-deletesnapshotsecret-namespace" | ||
CSIVSCDeletionPolicy = "velero.io/csi-vsc-deletion-policy" | ||
VolumeSnapshotClassSelectorLabel = "velero.io/csi-volumesnapshot-class" | ||
VolumeSnapshotClassDriverBackupAnnotationPrefix = "velero.io/csi-volumesnapshot-class" |
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 agree that these are logically different parameters. I think it's fine that they have the same value, since they're used in different contexts -- i.e. a PVC annotation shouldn't need to be called "csi-volumesnapshot-class-pvc" since it's already on the PVC.
Signed-off-by: Anshul Ahuja <[email protected]>
Resolved all comments and merge conflicts |
Signed-off-by: Anshul Ahuja <[email protected]>
Lets get the design merged in first. |
The design PR has been approved by 2 maintainers, would be useful if someone can merge it and also meanwhile review/ approve this current PR. |
I am not sure how this got closed, must have done by mistake |
Design PR is merged |
@blackpiglet / @shubham-pampattiwar can you please do final signoff and get this merged. |
@reasonerjt can we get this merged as well? |
Fix: #5750
Design: vmware-tanzu/velero#5774