add support for yml extension when searching for configuration file#506
Conversation
## Walkthrough
The codebase refactors file searching logic to use a new `Finder` struct, enabling dependency injection of the filesystem and improving testability. All previous package-level file search functions are now instance methods on `Finder`. The configuration file search now supports both `.yaml` and `.yml` extensions.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| filesearch/filesearch.go | Refactored to introduce a `Finder` struct encapsulating an `afero.Fs` filesystem. All file search functions are now methods of `Finder`. Removed global `fs` and `AppName`, now using `constants.ApplicationName`. Added `.yml` to configuration extensions. Added `NewFinder` constructor. |
| filesearch/filesearch_test.go | Refactored tests to use isolated `Finder` instances with in-memory filesystems. Removed global filesystem setup and redundant cleanup. Simplified test data structures and added a test case for `.yml` support. |
| commands_display.go, complete.go, config/config.go, main.go | Updated to use `filesearch.NewFinder().<method>()` instead of package-level file search functions. No logic changes, only invocation method altered. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Main
participant Finder
participant FS as FileSystem
Main->>Finder: NewFinder()
Main->>Finder: FindConfigurationFile("profiles.yml")
Finder->>FS: Check existence for profiles.yml/yaml
FS-->>Finder: File exists?
Finder-->>Main: Return file path or errorAssessment against linked issues
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #506 +/- ##
==========================================
- Coverage 79.34% 79.33% -0.00%
==========================================
Files 134 134
Lines 13231 13233 +2
==========================================
+ Hits 10497 10498 +1
- Misses 2316 2317 +1
Partials 418 418
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: 1
🧹 Nitpick comments (2)
filesearch/filesearch_test.go (2)
235-239: Flaky test: relies on the real filesystem
TestCannotFindConfigurationFilecallsNewFinder(), which wires the OS filesystem.
If a developer (or CI runner) happens to have a file calledsome_config_file.{yaml|yml|…}in any of the search paths, this test will produce a false negative.Use an in-memory filesystem (as you did in the other tests) to guarantee determinism:
- finder := NewFinder() - found, err := finder.FindConfigurationFile("some_config_file") + fs := afero.NewMemMapFs() + finder := Finder{fs: fs} + found, err := finder.FindConfigurationFile("some_config_file")
242-252:TestFindResticBinaryalso depends on the host environmentThe current implementation succeeds if the host has a
resticbinary somewhere inPATH, but otherwise asserts an error.
This makes the outcome environment-dependent and weakens the value of the test.Consider switching to an in-memory filesystem and explicitly creating a dummy
resticfile in one of the expected locations. This makes the expected result unambiguous and removes the fallback branch from the test path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
commands_display.go(1 hunks)complete.go(1 hunks)config/config.go(1 hunks)filesearch/filesearch.go(11 hunks)filesearch/filesearch_test.go(8 hunks)main.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
commands_display.go (1)
filesearch/filesearch.go (1)
NewFinder(76-80)
main.go (1)
filesearch/filesearch.go (1)
NewFinder(76-80)
complete.go (1)
filesearch/filesearch.go (1)
NewFinder(76-80)
config/config.go (1)
filesearch/filesearch.go (1)
NewFinder(76-80)
filesearch/filesearch_test.go (1)
filesearch/filesearch.go (2)
Finder(72-74)NewFinder(76-80)
filesearch/filesearch.go (1)
constants/app.go (1)
ApplicationName(4-4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and test (1.24, windows-latest)
🔇 Additional comments (6)
commands_display.go (1)
168-168: Implementation successfully refactored to use the Finder struct.The code now creates a new Finder instance via
NewFinder()to call theFindResticBinarymethod instead of using the package-level function. This change supports the broader refactoring effort and maintains the same functionality.main.go (2)
278-278: Implementation successfully refactored to use the Finder struct.The configuration file search logic is now appropriately using the Finder struct instance. This change aligns with the broader refactoring effort while preserving the original functionality.
349-349: Implementation successfully refactored for restic binary location.The restic binary search logic now properly uses the Finder struct instance. This consistent pattern improves code maintainability.
complete.go (1)
137-137: Implementation successfully refactored for profile name listing.The configuration file search in the
listProfileNamesfunction now correctly uses the Finder struct instance, consistent with the application-wide refactoring approach.config/config.go (2)
104-104: Implementation successfully refactored for configuration includes.The code now properly uses the Finder struct to search for configuration includes, maintaining consistency with the refactoring approach across the codebase.
72-73:Details
✅ Verification successful
Verify implementation of .yml extension support in FindConfigurationFile.
While the refactoring to use Finder is visible in this PR, we should verify that the actual implementation in the filesearch package properly supports .yml extension as stated in the PR objective.
🏁 Script executed:
#!/bin/bash # Check if the FindConfigurationFile method in the Finder struct supports .yml extension # Look for .yml extension support in filesearch package rg -A 5 -B 5 "yml" --glob "filesearch/*.go" # Check for any test cases that verify .yml extension support rg -A 5 -B 5 "yml" --glob "filesearch/*_test.go"Length of output: 3852
.yml extension support verified
- The
filesearch/filesearch.goimplementation includes"yml"in its list of recognised extensions.- Corresponding test cases in
filesearch/filesearch_test.gocover searching for files with the.ymlsuffix.All good—no further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
filesearch/filesearch.go(11 hunks)filesearch/filesearch_test.go(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
filesearch/filesearch_test.go (1)
filesearch/filesearch.go (2)
Finder(70-72)NewFinder(74-78)
filesearch/filesearch.go (1)
constants/app.go (1)
ApplicationName(4-4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
filesearch/filesearch_test.go (9)
98-102: Good addition of .yml test caseThe addition of the
.ymlextension test case aligns with the main change infilesearch.go. This ensures proper testing coverage for the newly supported file extension.
140-144: LGTMAdded test case for
profiles.ymlin theunittest-configdirectory. This maintains consistency with other extensions test cases.
181-186: LGTMAppropriately added test case for
profiles.ymlin the XDG ConfigHome directory, which ensures complete coverage of all configuration extension possibilities.
207-229: Good refactoring to isolated test casesExcellent refactoring of the test logic to use subtests with parallel execution. The use of in-memory filesystem instances (
afero.NewMemMapFs()) for each test provides better isolation and prevents interference between tests. The test now properly exercises the newFinderstruct functionality.
235-236: LGTMProperly updated to use the new
Finderstruct.
244-261: Good test enhancementThe test has been improved by explicitly creating the test binary in the filesystem. This makes the test more reliable by ensuring the binary exists in a known location rather than depending on actual system paths.
266-269: LGTMUpdated to use the new
Finderstruct with in-memory filesystem.
281-281: LGTMTest properly updated to use
platform.IsWindows()and the newFinderstruct.Also applies to: 288-290, 296-296
339-375: LGTMTest properly updated to use the
Finderstruct with in-memory filesystem. This change maintains consistency with the broader refactoring.Also applies to: 380-380
filesearch/filesearch.go (4)
26-26: Great! Added .yml extension supportAdding the
.ymlextension to the list of supported configuration file extensions is a good change that addresses issue #404. This extension is commonly used as an alternative to.yaml.
13-13: Good use of constantsReplacing hardcoded string "resticprofile" with
constants.ApplicationNamefrom the constants package improves maintainability. If the application name needs to change in the future, it will only need to be updated in one place.Also applies to: 42-42, 224-224, 226-226
70-78: Well-designed refactoring to use dependency injectionThe introduction of the
Finderstruct and theNewFinderconstructor function is a good design choice. This change enables proper dependency injection of the filesystem interface, making the code more testable and modular.
82-82: LGTM on method conversionConverting standalone functions to methods on the
Finderstruct is a logical part of the refactoring. This ensures a consistent approach to dependency injection throughout the package.Also applies to: 110-110, 130-130, 175-175
|
1 participant
Add support for
ymlextension when searching for configuration fileFixes #404