vmware-tanzu / velero

Backup and migrate Kubernetes applications and their persistent volumes
https://velero.io
Apache License 2.0
8.45k stars 1.37k forks source link

Extend Volume Policies feature to support more actions #7664

Closed shubham-pampattiwar closed 3 months ago

shubham-pampattiwar commented 3 months ago

Thank you for contributing to Velero!

Please add a summary of your change

Implementation PR for Extend Volume Policies feature to support more actions

Related design: https://github.com/vmware-tanzu/velero/pull/6956

Does your change fix a particular issue?

Fixes https://github.com/vmware-tanzu/velero/issues/6640

Please indicate you've done the following:

Lyndon-Li commented 3 months ago

@shubham-pampattiwar Could help to address the comments so that we can further review and merge it? To meet FC, we hope this could be merged by the end of this week or early next week.

shubham-pampattiwar commented 3 months ago

@Lyndon-Li addressed PR comments.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 73.85621% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 58.66%. Comparing base (22b9465) to head (8d2bef2). Report is 24 commits behind head on main.

Files Patch % Lines
pkg/backup/item_backupper.go 35.48% 16 Missing and 4 partials :warning:
internal/volumehelper/volume_policy_helper.go 78.31% 11 Missing and 7 partials :warning:
internal/resourcepolicies/resource_policies.go 75.00% 0 Missing and 1 partial :warning:
pkg/cmd/server/server.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #7664 +/- ## ========================================== + Coverage 58.51% 58.66% +0.14% ========================================== Files 343 344 +1 Lines 28486 28723 +237 ========================================== + Hits 16670 16849 +179 - Misses 10401 10445 +44 - Partials 1415 1429 +14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Lyndon-Li commented 3 months ago

@shubham-pampattiwar Two more following up comments from me and @blackpiglet, see if you could address it. Basically, we suggest to avoid generating a dynamic client inside the helper, but get it from the backup as an input parameter and then call the existing function in util/pvc_pv.go.

Lyndon-Li commented 3 months ago

@shubham-pampattiwar We also see the code coverage has dropped by 0.24% due to this PR, could you help to add some more test case?

shubham-pampattiwar commented 3 months ago

@Lyndon-Li @blackpiglet Addressed PR feedback and added tests. PTAL, Thanks !