Remove Restic code path from PodVolumeRestore#9732
Remove Restic code path from PodVolumeRestore#9732blackpiglet wants to merge 1 commit intovelero-io:mainfrom
Conversation
Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9732 +/- ##
==========================================
+ Coverage 60.98% 61.30% +0.32%
==========================================
Files 384 383 -1
Lines 36612 36386 -226
==========================================
- Hits 22327 22306 -21
+ Misses 12677 12474 -203
+ Partials 1608 1606 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This might cause upgrade failure on our side.
Have we considered adding another version of the CRD ie. v2?
We can then implement conversion webhook to immediately deprecate/remove older v1 CRD.
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version
There was a problem hiding this comment.
For V1, we can keep same enums but edit description like https://github.com/vmware-tanzu/velero/pull/9676/changes#diff-e710ccbdbd4149cab26deb6b55961c09fdf6ef984d8ebfe252a6e30c40e8a31f
shubham-pampattiwar
left a comment
There was a problem hiding this comment.
Thanks for putting this together, the restic code path cleanup is great to see.
One thing I wanted to flag: removing existing enum values (restic, "") from the CRD schemas is a breaking change per the https://github.com/operator-framework/operator-controller/blob/main/docs/concepts/crd-upgrade-safety.md policy. This would block upgrades for OLM-managed deployments, and existing CRs in etcd with those values would fail validation against the new schema.
Could we keep the enum as-is and instead add controller-level validation that rejects restic / "" at reconcile time (e.g., fail with "restic uploader is no longer supported, use kopia")? Same end result without the schema-level break.
+1 to @kaovilai's concern as well.
|
+1 to avoiding the breaking change to CRDs. Without version bumps, it's ok to add to enum values but not to remove them. Failing with a validation error when Restic is specified should be sufficient. |
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #9468
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.