Add copy from repository in configuration #486
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)config/profile.go (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hgraeber
force-pushed
the
fix-copy
branch
2 times, most recently
from
c9e8b9a to
81b2208
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #486 +/- ##
==========================================
+ Coverage 79.29% 79.39% +0.10%
==========================================
Files 136 137 +1
Lines 13264 13390 +126
==========================================
+ Hits 10517 10630 +113
- Misses 2326 2340 +14
+ Partials 421 420 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/profile.go (1)
354-355: Misleading description references “backup” instead of “copy”.
check-before/check-aftersit in the copy section, yet the doc-strings still say “backup command”.
Rename to “copy command” to avoid confusion when reading the generated documentation or help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/profile.go(3 hunks)config/profile_test.go(3 hunks)wrapper.go(1 hunks)wrapper_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
wrapper_test.go (1)
config/confidential.go (1)
NewConfidentialValue(81-83)
config/profile.go (4)
config/confidential.go (1)
ConfidentialValue(14-16)shell/args.go (1)
Args(11-14)constants/parameter.go (5)
ParameterRepository(23-23)ParameterRepositoryFile(24-24)ParameterPasswordFile(25-25)ParameterPasswordCommand(26-26)ParameterKeyHint(27-27)shell/arg.go (2)
NewArg(40-51)ArgConfigEscape(21-21)
🪛 GitHub Check: codecov/patch
wrapper.go
[warning] 152-155: wrapper.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 160-163: wrapper.go#L160-L163
Added lines #L160 - L163 were not covered by tests
[warning] 169-170: wrapper.go#L169-L170
Added lines #L169 - L170 were not covered by tests
[warning] 174-177: wrapper.go#L174-L177
Added lines #L174 - L177 were not covered by tests
[warning] 182-185: wrapper.go#L182-L185
Added lines #L182 - L185 were not covered by tests
config/profile.go
[warning] 402-403: config/profile.go#L402-L403
Added lines #L402 - L403 were not covered by tests
[warning] 427-428: config/profile.go#L427-L428
Added lines #L427 - L428 were not covered by tests
🔇 Additional comments (4)
wrapper.go (1)
144-189: Consider extracting the pre-/post orchestration into a shared helperThe new Copy flow (check → retention → copy → retention → check) mirrors
getBackupAction, but is hand-coded a second time.
Duplicated control-flow increases the risk of the two drifting and is already showing up as uncovered lines (Codecov hints).A small, composable helper such as
func (r *resticWrapper) runWithGuards( beforeChecks, beforeRetention, main, afterRetention, afterChecks func() error) error { … }would remove the duplication and make future maintenance & test coverage simpler.
[ suggest_optional_refactor ]
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 152-155: wrapper.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 160-163: wrapper.go#L160-L163
Added lines #L160 - L163 were not covered by tests
[warning] 169-170: wrapper.go#L169-L170
Added lines #L169 - L170 were not covered by tests
[warning] 174-177: wrapper.go#L174-L177
Added lines #L174 - L177 were not covered by tests
[warning] 182-185: wrapper.go#L182-L185
Added lines #L182 - L185 were not covered by testswrapper_test.go (1)
1632-1635: LGTM – test data now reflects the newTo*fieldsThe switch to
ToRepository/ToPasswordFilealigns the test with the refactored struct – no issues spotted.config/profile_test.go (2)
1505-1607: Great coverage for source/destination flag mappingThese tests thoroughly exercise the new
From*↔To*permutations across restic versions – nice work.One small ask: add a
require.NoError(t, copySection.Validate())(or similar) before expectations to guard future validation logic.[ suggest_nitpick ]
1610-1712: Symmetric “To” tests complete the matrix*The complementary tests for destination-centric copies are clear and valuable.
No functional concerns.
resticprofile implements a "special case" for the copy command, which does not use the "from-"-prefix and reverses the copy direction. With this change the standard direction is used when the "from" prefix is explicitly used.
|
Hey! how is it going with this PR? Tell me if you need any help 👍🏻 |
|
It's the part of
|
|
If the first part of the PR is ready, you can definitely split it in two. |
creativeprojects
left a comment
creativeprojects
left a comment
There was a problem hiding this comment.
I just have that one comment about the flag name, otherwise it looks good to me 👍🏻
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/profile.go (2)
391-438: Complex but logically sound implementation of init flag generation.The method correctly handles both copy directions with appropriate chunker parameter and initialisation logic. However, the implementation is quite complex with profile copying and intricate conditional logic.
Consider if this complexity can be reduced through helper methods or clearer separation of concerns, though the current logic appears functionally correct.
440-503: Comprehensive handling of version-specific flag generation for both copy directions.The method correctly implements flag generation for both copy scenarios while handling restic version differences appropriately. The repositoryArgs map approach provides clean parameter management.
The implementation is functionally sound but quite complex. Consider extracting helper methods for flag generation logic to improve maintainability and reduce code duplication between the copy-to and copy-from branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/profile.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
config/profile.go (5)
util/maybe/bool.go (1)
Bool(11-13)config/confidential.go (1)
ConfidentialValue(14-16)shell/args.go (1)
Args(11-14)constants/parameter.go (5)
ParameterRepository(23-23)ParameterRepositoryFile(24-24)ParameterPasswordFile(25-25)ParameterPasswordCommand(26-26)ParameterKeyHint(27-27)shell/arg.go (2)
NewArg(40-51)ArgConfigEscape(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: Build and test (1.24, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
config/profile.go (4)
347-363: Excellent refactoring of CopySection fields for source/destination clarity.The separation into explicit
From*andTo*fields provides clear semantics for copy operations. The field descriptions correctly identify source vs destination repositories, addressing the confusion highlighted in past reviews.
367-367: LGTM! Clean implementation of copy direction detection.The
IsCopyTo()method provides a clear way to determine copy operation direction based on destination repository configuration.
369-377: Excellent fix for repository path normalisation.The conditional logic properly normalises both source and destination repository paths based on copy direction, ensuring consistent behaviour with other repository fields throughout the codebase.
379-389: Consistent implementation of conditional path resolution.The method correctly applies root path resolution to the appropriate source or destination fields based on copy direction, maintaining consistency with the overall path handling approach.
This is consistent with the `init` section.
|
Thanks for the change 👍🏻 Actually I found something during testing: when the While leaving the So we have two choices:
Any preference? |
|
... which I realise is also opening yet another a can of worms 😆 In the order of initialisation, the source should be done first, then the target with the |
|
It is important to initialize the target repository to be consistent with the backup section. This is useful for making copies to external drives and initialize them on first use. Nor forgetting to use I wouldn't bother with initializing a source drive. This is really an edge case. Possibly forgetting |
|
Makes sense 😉 |
2 participants
Fix the copy command so that
from-repositoryandfrom-repository-fileworks like they work with restic alone. That meanscopies from repository
r2tor1and not in the opposite direction, like it does now. That means the repository mentioned directly belowprofile(r1) is the destination similar tobackup. The special case without the "from"-prefix mention in the docs works like before.As a bonus the retention section can allow
before-copyandafter-copyanalog tobefore-backupandafter-backup. This will be reserved for a future PR.