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
use GA topology labels for PVs #2219
Conversation
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
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 lgtm 👍
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.
sorry for the delay in reviewing here @cpanato. Added a couple specific comments.
I'd also like to add a unit test case or two for this. We already have a test case here around the existing AZ label:
https://github.com/vmware-tanzu/velero/blob/master/pkg/backup/backup_test.go#L1743-L1776
It'd be nice to:
- add a test case that confirms that if both labels exist on the PV, the GA one is the one that's used
- add a test case that confirms that if only the GA label exists on the PV, it's used
- perhaps just re-title the existing test case to something like "if only the beta label exists on the PV, it's used"
Let me know if you have any questions on the structure of the test; hopefully a copy-paste with some modifications works.
Thanks again for the PR!
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@skriss thanks so much for taking your time to 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.
LGTM -- those unit test cases are exactly what I was looking for, thanks!
@carlisia please take another look when you get a chance! |
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.
👍 thank you!
add stable topology labels for PV the old label was deprecated in K8s 1.17 - https://kubernetes.io/docs/reference/kubernetes-api/labels-annotations-taints/#topologykubernetesiozone