also search configuration from ~/.config/resticprofile on macOS #370
Conversation
WalkthroughThe recent modifications to Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 71.48% 71.52% +0.04%
==========================================
Files 121 121
Lines 12723 12746 +23
==========================================
+ Hits 9095 9116 +21
- Misses 3218 3220 +2
Partials 410 410
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
Outside diff range and nitpick comments (2)
filesearch/filesearch.go (2)
Line range hint
210-210: Potential security issue: Subprocessexec.Commandused inShellExpandmight be susceptible to command injection if not properly sanitized.Consider sanitizing the
filenameparameter to prevent any potential command injection vulnerabilities.
Line range hint
83-83: The variabledisplayFileis reassigned in theFindConfigurationFilefunction without using its initial value.Consider initializing
displayFiledirectly in the conditional branches to avoid unnecessary assignment.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .goreleaser.yml (1 hunks)
- codecov.yml (1 hunks)
- filesearch/filesearch.go (7 hunks)
- filesearch/filesearch_test.go (8 hunks)
- schedule/scheduler_config.go (3 hunks)
Files skipped from review due to trivial changes (3)
- .goreleaser.yml
- codecov.yml
- schedule/scheduler_config.go
Additional context used
golangci-lint
filesearch/filesearch.go
[medium] 210-210: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
83-83: assigned to displayFile, but reassigned without using the value (wastedassign)
filesearch/filesearch_test.go
58-58: Function 'testLocations' is too long (135 > 60) (funlen)
1-1: package should be
filesearch_testinstead offilesearch(testpackage)
279-279: TestShellExpand's subtests should call t.Parallel (tparallel)
312-312: TestFindConfigurationIncludes's subtests should call t.Parallel (tparallel)
GitHub Check: codecov/patch
filesearch/filesearch.go
[warning] 235-235: filesearch/filesearch.go#L235
Added line #L235 was not covered by tests
[warning] 279-279: filesearch/filesearch.go#L279
Added line #L279 was not covered by tests
Additional comments not posted (6)
filesearch/filesearch.go (4)
20-20: The variableXDGAppNamehas been renamed toAppNameto reflect a more generic usage, aligning with the PR's objective to support macOS. This change also impacts the rest of the code whereXDGAppNamewas used.
43-45: Added macOS-specific configuration paths using theAppNamevariable. This aligns with the PR's objective to support configuration loading from~/.config/resticprofileon macOS. Ensure that this path is widely recognized on macOS systems as a standard configuration directory.
259-259: ThegetSearchBinaryLocationsfunction has been updated to include user-specific binary locations on non-Windows platforms. This is a logical extension to handle binaries in user-specific directories.
220-220: The functiongetSearchConfigurationLocationshas been updated to include configuration paths for macOS using a newly introduced utility functionaddRootToRelativePaths. This change is crucial for supporting macOS as per the PR's objective.However, the added lines are not covered by tests.
Also applies to: 222-222, 224-224, 233-235, 237-237
filesearch/filesearch_test.go (2)
43-44: The use oft.Parallel()inTestDefaultConfigDirsis appropriate for allowing parallel execution of tests, improving test execution speed.
368-399: The new test functionTestAddRootToRelativePathseffectively checks the functionality of theaddRootToRelativePathsutility function. It covers various scenarios including empty root and absolute paths.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
filesearch/filesearch.go (2)
Line range hint
220-236: The logic to dynamically construct configuration paths based on the platform is robust and maintainable. However, consider handling potential errors fromos.UserHomeDir()to ensure reliability even in edge cases where the home directory might not be accessible or determinable.- home, _ := os.UserHomeDir() + home, err := os.UserHomeDir() + if err != nil { + return []string{} // or handle error appropriately + }
Line range hint
210-210: Subprocess launched with potentially tainted input inShellExpand. Ensure that inputs are sanitised or validated to prevent injection vulnerabilities.- cmd := exec.Command("sh", "-c", fmt.Sprintf("echo %s", strings.ReplaceAll(filename, " ", `\ `))) + safeFilename := strings.ReplaceAll(filename, " ", `\ `) + safeFilename = strings.ReplaceAll(safeFilename, ";", `\:`) + cmd := exec.Command("sh", "-c", fmt.Sprintf("echo %s", safeFilename))filesearch/filesearch_test.go (2)
Line range hint
58-58: ThetestLocationsfunction is quite long, which might hinder maintainability and readability. Consider breaking it down into smaller, more focused helper functions.
Line range hint
1-1: The package declaration should befilesearch_testinstead offilesearchto ensure that the tests are properly isolated and are black-box tests. This helps in validating the public interface of the package without relying on its internal state.- package filesearch + package filesearch_test
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- filesearch/filesearch.go (7 hunks)
- filesearch/filesearch_test.go (11 hunks)
Additional context used
golangci-lint
filesearch/filesearch.go
[medium] 210-210: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
83-83: assigned to displayFile, but reassigned without using the value (wastedassign)
filesearch/filesearch_test.go
58-58: Function 'testLocations' is too long (135 > 60) (funlen)
1-1: package should be
filesearch_testinstead offilesearch(testpackage)
Additional comments not posted (4)
filesearch/filesearch.go (2)
20-20: RenamingXDGAppNametoAppNamesimplifies and generalises the variable name, aligning with the broader use case beyond XDG contexts. Good improvement for clarity and maintainability.
43-45: Adding macOS specific configuration paths under.config/resticprofile/aligns with the XDG standards and improves user experience on macOS by standardising configuration locations across different systems.filesearch/filesearch_test.go (2)
43-44: Usingt.Parallel()to run tests in parallel is a great practice to improve test execution efficiency. Consistent application across all tests ensures better performance.
197-198: Good use oft.Parallel()in test functions to enhance test execution efficiency. Ensure consistency by applying it to all subtests as well.Also applies to: 237-238, 245-246, 257-258, 280-281, 305-306, 315-316, 356-357, 375-376
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- filesearch/filesearch.go (8 hunks)
Files not reviewed due to errors (1)
- filesearch/filesearch.go (no review received)
Additional context used
golangci-lint
filesearch/filesearch.go
74-74: don't use
initfunction (gochecknoinits)
[medium] 210-210: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
83-83: assigned to displayFile, but reassigned without using the value (wastedassign)
Fixes #369