backube / volsync

Asynchronous data replication for Kubernetes volumes
https://volsync.readthedocs.io
GNU Affero General Public License v3.0
601 stars 69 forks source link

restic mover should return error if unable to 'EnsurePVCFromSrc' #1455

Closed benfiola closed 1 week ago

benfiola commented 1 week ago

Describe what this PR does It appears that the restic mover will suppress errors when EnsurePVCFromSource is called. If the ReplicationSource has incorrect values (- in my case, I had forgotten the spec.restic.copyMethod field) - the resource will silently fail to re-reconcile.

Is there anything that requires special attention? None that I can immediately see.

Related issues: This PR addresses the following issue

(Aside from this hiccup caused by user error - thank you for the excellent controller! It's exactly what I've been looking for and works great.)

openshift-ci[bot] commented 1 week ago

Hi @benfiola. Thanks for your PR.

I'm waiting for a backube member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
tesshuflower commented 1 week ago

@benfiola thanks for this!

I'm the one who introduced this bug :grimacing: I've just pushed another commit that makes the same fix for other movers that have the same issue.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.2%. Comparing base (b36e2df) to head (840adbd). Report is 13 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1455 +/- ## ===================================== Coverage 66.2% 66.2% ===================================== Files 57 57 Lines 7474 7474 ===================================== Hits 4950 4950 Misses 2236 2236 Partials 288 288 ``` | [Files with missing lines](https://app.codecov.io/gh/backube/volsync/pull/1455?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=backube) | Coverage Δ | | |---|---|---| | [controllers/mover/rclone/mover.go](https://app.codecov.io/gh/backube/volsync/pull/1455?src=pr&el=tree&filepath=controllers%2Fmover%2Frclone%2Fmover.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=backube#diff-Y29udHJvbGxlcnMvbW92ZXIvcmNsb25lL21vdmVyLmdv) | `73.7% <100.0%> (ø)` | | | [controllers/mover/restic/mover.go](https://app.codecov.io/gh/backube/volsync/pull/1455?src=pr&el=tree&filepath=controllers%2Fmover%2Frestic%2Fmover.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=backube#diff-Y29udHJvbGxlcnMvbW92ZXIvcmVzdGljL21vdmVyLmdv) | `82.2% <100.0%> (ø)` | | | [controllers/mover/rsync/mover.go](https://app.codecov.io/gh/backube/volsync/pull/1455?src=pr&el=tree&filepath=controllers%2Fmover%2Frsync%2Fmover.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=backube#diff-Y29udHJvbGxlcnMvbW92ZXIvcnN5bmMvbW92ZXIuZ28=) | `75.2% <100.0%> (ø)` | | | [controllers/mover/rsynctls/mover.go](https://app.codecov.io/gh/backube/volsync/pull/1455?src=pr&el=tree&filepath=controllers%2Fmover%2Frsynctls%2Fmover.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=backube#diff-Y29udHJvbGxlcnMvbW92ZXIvcnN5bmN0bHMvbW92ZXIuZ28=) | `72.6% <100.0%> (ø)` | |

🚨 Try these New Features:

benfiola commented 1 week ago

Sounds good! Closing this one in favor of your changes!

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
100.0% Duplication on New Code

See analysis details on SonarQube Cloud

tesshuflower commented 1 week ago

@benfiola Oops, I should've been clearer - the commit I pushed was to your branch - so still need to get this PR merged (contains your change, plus my additional change to do the same for other movers).

benfiola commented 1 week ago

OH, my bad! 😅 I’ll stop touching the buttons.

tesshuflower commented 1 week ago

/ok-to-test

tesshuflower commented 1 week ago

/retest /approve /lgtm

openshift-ci[bot] commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benfiola, tesshuflower

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/backube/volsync/blob/main/OWNERS)~~ [tesshuflower] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment