Add new interface based downloader package #1077
Conversation
samtholiya
force-pushed
the
feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
branch
from
6cc237b to
f37cee1
Compare
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
|
Caution: we have #1076 we need to merge which modifies the same files |
samtholiya
force-pushed
the
feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
branch
from
4639839 to
eafed34
Compare
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
📝 WalkthroughWalkthroughThe pull request removes legacy GitHub URL processing and context management from the internal execution logic and transitions the download functionality to a new, modular downloader package. The changes update various functions to replace calls to previously used custom go-getter methods with new implementations from the downloader package. Additionally, a comprehensive downloader package is introduced with interfaces, concrete implementations, custom GitHub detectors, and accompanying tests and mocks to improve file fetching, error handling, and format autodetection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Downloader as FileDownloader
participant Factory as ClientFactory
participant DClient as DownloadClient
Client->>Downloader: Invoke Fetch(src, dest, mode, timeout)
Downloader->>Factory: NewClient(ctx, src, dest, mode)
Factory->>DClient: Create download client
Downloader->>DClient: Call Get()
DClient-->>Downloader: Return result/error
Downloader-->>Client: Return download result or parsed output
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/downloader/file_downloader_interface.go (2)
20-25: Consider adding context timeout validation.The
NewClientmethod accepts a context but doesn't specify timeout requirements.Add documentation to specify timeout expectations:
// ClientFactory abstracts the creation of a downloader client for better testability +// +// The provided context should include appropriate timeout settings to prevent +// long-running downloads from blocking indefinitely. // //go:generate mockgen -source=$GOFILE -destination=mock_$GOFILE -package=$GOPACKAGE type ClientFactory interface {
32-53: Consider adding String() method for ClientMode.Adding a String() method would improve logging and error messages.
type ClientMode int + +func (m ClientMode) String() string { + switch m { + case ClientModeAny: + return "any" + case ClientModeFile: + return "file" + case ClientModeDir: + return "directory" + default: + return "invalid" + } +}pkg/downloader/file_downloader.go (1)
57-87: Consider adding format-specific error messages.The error messages in
detectFormatAndParsecould be more specific about which format failed to parse.case utils.IsHCL(data): err = hcl.Unmarshal(d, &v) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse HCL: %w", err) }pkg/downloader/file_downloader_test.go (2)
12-25: Consider enhancing test coverage with additional assertions.The test verifies basic success scenario but could be more thorough by:
- Asserting that the mock was actually called with expected parameters
- Testing with different timeout values
- Testing with different ClientMode values
41-87: Consider adding negative test cases.While the test covers successful parsing of different formats, it would be beneficial to add test cases for:
- Invalid JSON/YAML/HCL content
- Empty files
- Files with unsupported formats
pkg/downloader/custom_github_detector.go (1)
69-78: Add logging for token injection failures.When token injection is skipped due to existing credentials, consider logging more details to help with debugging.
- u.LogDebug("Credentials found, skipping token injection\n") + u.LogDebug(fmt.Sprintf("Credentials found in URL for %s, skipping token injection\n", parsedURL.Host))internal/exec/vendor_model_component.go (2)
130-130: Consider splitting dependency migration into a separate PR.Based on previous feedback (from PR #768), significant dependency changes like replacing go-getter should be handled in separate PRs to minimize risk and improve reviewability.
181-181: Add error context for download failures.Consider wrapping the error with additional context about the download mode and timeout:
- if err = downloader.NewGoGetterDownloader(atmosConfig).Fetch(p.uri, filepath.Join(tempDir, p.mixinFilename), downloader.ClientModeFile, 10*time.Minute); err != nil { + if err = downloader.NewGoGetterDownloader(atmosConfig).Fetch(p.uri, filepath.Join(tempDir, p.mixinFilename), downloader.ClientModeFile, 10*time.Minute); err != nil { + return fmt.Errorf("failed to download package %s (mode: %v, timeout: %v): %w", p.name, downloader.ClientModeFile, 10*time.Minute, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(1 hunks)internal/exec/go_getter_utils.go(0 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_model.go(2 hunks)internal/exec/vendor_model_component.go(3 hunks)internal/exec/yaml_func_include.go(2 hunks)pkg/downloader/custom_github_detector.go(1 hunks)pkg/downloader/file_downloader.go(1 hunks)pkg/downloader/file_downloader_interface.go(1 hunks)pkg/downloader/file_downloader_test.go(1 hunks)pkg/downloader/gogetter_downloader.go(1 hunks)pkg/downloader/mock_file_downloader_interface.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/exec/go_getter_utils.go
🧰 Additional context used
🧠 Learnings (2)
internal/exec/vendor_model_component.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
internal/exec/validate_stacks.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#731
File: internal/exec/validate_stacks.go:0-0
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (13)
pkg/downloader/gogetter_downloader.go (3)
10-17: Well-structured client implementation!The
goGetterClientstruct follows good encapsulation principles by wrapping the go-getter client. TheGetmethod provides a clean interface for download operations.
53-56: LGTM - Clean factory method implementation!The
NewGoGetterDownloaderfunction follows the factory pattern well, properly initializing dependencies.
42-51:❓ Verification inconclusive
Consider thread safety for global state modification.
The function modifies the global
getter.Detectorsslice. This could lead to race conditions in concurrent scenarios.
🏁 Script executed:
#!/bin/bash # Search for concurrent usage of getter.Detectors rg -A 2 "getter\.Detectors"Length of output: 389
Global state thread safety concern: Verify concurrent use of
getter.DetectorsThe update to
getter.DetectorsinregisterCustomDetectorscould be problematic if multiple goroutines invoke it concurrently. Although our search only shows this update in one location, please ensure:
- The function is called only once during initialization or is otherwise protected against concurrent access.
- If concurrent access is possible, consider wrapping the update in a synchronization mechanism (e.g., using a mutex) to avoid potential race conditions.
internal/exec/yaml_func_include.go (1)
51-51: Clean integration of the new downloader!The change effectively integrates the new
FileDownloaderinterface while maintaining the existing error handling pattern.pkg/downloader/file_downloader_interface.go (1)
8-18: Excellent interface design with clear documentation!The
FileDownloaderinterface is well-documented and follows the interface segregation principle.pkg/downloader/file_downloader.go (1)
17-32: Excellent use of dependency injection!The struct design with injected dependencies makes the code highly testable. The use of functional options for tempPathGenerator and fileReader is a great pattern.
pkg/downloader/file_downloader_test.go (1)
27-39: LGTM! Well-structured error handling test.The test properly verifies error propagation and includes appropriate assertions.
pkg/downloader/mock_file_downloader_interface.go (1)
1-141: LGTM! Auto-generated mocks look correct.The mock implementations are properly generated and align with the interfaces.
internal/exec/vendor_model.go (2)
18-18: LGTM! Import of the new downloader package.The addition of the downloader package aligns with the PR's objective to improve testability and abstraction.
272-272: LGTM! Refactored download implementation.The change to use
downloader.NewGoGetterDownloaderimproves testability by abstracting the download functionality into a dedicated package. The timeout of 10 minutes and mode settings are preserved, maintaining the existing behavior.internal/exec/validate_stacks.go (2)
18-18: LGTM! Import of the new downloader package.The addition of the downloader package aligns with the PR's objective to improve testability and abstraction.
404-404: LGTM! Refactored schema download implementation.The change to use
downloader.NewGoGetterDownloaderwith a 30-second timeout and file mode is appropriate for schema downloads. Based on past learnings, the temporary file handling is correct as it gets overwritten on each download.go.mod (1)
27-27: LGTM! Addition of mock package dependency.The addition of
github.com/golang/mockis appropriate for generating mocks, supporting the PR's goal of improving testability.
…or-better-testability-and
…or-better-testability-and
…or-better-testability-and
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/downloader/file_downloader_test.go (3)
12-25: Consider parameterizing the timeout value.The test is well-structured, but consider making the timeout value configurable or using a shorter duration for tests. This would help speed up test execution while still validating the functionality.
+const testTimeout = 1 * time.Second func TestFileDownloader_Fetch_Success(t *testing.T) { // ... - err := fd.Fetch("src", "dest", ClientModeFile, 10*time.Second) + err := fd.Fetch("src", "dest", ClientModeFile, testTimeout)
27-39: Use predefined errors instead of dynamic error creation.Following Go best practices and the static analysis suggestion, consider using a predefined error variable instead of creating a new error instance.
+var errClientCreationFailed = errors.New("client creation failed") func TestFileDownloader_Fetch_Failure(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockFactory := NewMockClientFactory(ctrl) - expectedErr := errors.New("client creation failed") - mockFactory.EXPECT().NewClient(gomock.Any(), "src", "dest", ClientModeFile).Return(nil, expectedErr) + mockFactory.EXPECT().NewClient(gomock.Any(), "src", "dest", ClientModeFile).Return(nil, errClientCreationFailed) fd := NewFileDownloader(mockFactory) err := fd.Fetch("src", "dest", ClientModeFile, 10*time.Second) assert.Error(t, err) - assert.Equal(t, expectedErr, errors.Unwrap(err)) + assert.Equal(t, errClientCreationFailed, errors.Unwrap(err))🧰 Tools
🪛 GitHub Check: golangci
[failure] 32-32: [golangci] pkg/downloader/file_downloader_test.go#L32
do not define dynamic errors, use wrapped static errors instead: "errors.New("client creation failed")" (err113)
41-87: Consider improving the test robustness.While the table-driven tests are well-structured, there are a few potential improvements:
- The hardcoded temp file path
/tmp/testfile.jsonmight not work on all platforms (e.g., Windows).- The test data could include more edge cases.
func TestFileDownloader_FetchAndAutoParse(t *testing.T) { // ... - tempFile := "/tmp/testfile.json" + tempFile := t.TempDir() + "/testfile.json" // ... testCases := []struct { name string fileData string + wantErr bool }{ // ... existing cases ... + { + name: "should handle empty json", + fileData: `{}`, + wantErr: false, + }, + { + name: "should handle invalid json", + fileData: `{invalid}`, + wantErr: true, + }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/downloader/file_downloader_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/downloader/file_downloader_test.go
[failure] 32-32: [golangci] pkg/downloader/file_downloader_test.go#L32
do not define dynamic errors, use wrapped static errors instead: "errors.New("client creation failed")" (err113)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/downloader/file_downloader_test.go (1)
1-11: Clean and well-organized imports!The package declaration and imports are well-structured, using appropriate testing utilities.
…or-better-testability-and
…or-better-testability-and
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/downloader/gogetter_downloader_test.go (1)
14-21: Consider using t.TempDir() for cleaner test codeThe helper function works well, but the standard library provides a built-in alternative that handles cleanup automatically.
-// Test helper to create a temporary directory. -func createTempDir(t *testing.T) string { - dir, err := os.MkdirTemp("", "test") - if err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - return dir -} // Use t.TempDir() directly in your tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/downloader/gogetter_downloader_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/downloader/gogetter_downloader_test.go (5)
1-12: Clean and straightforward importsImport organization looks good, providing all necessary dependencies for the tests.
23-52: Solid test for GoGetterClient Get methodThe test effectively verifies the core functionality of copying files using go-getter. Good job properly cleaning up resources with defer statements.
One minor suggestion: consider using a constant for file permissions to improve readability:
- err := os.WriteFile(testFile, []byte("test content"), 0o600) + const readWritePerms = 0o600 + err := os.WriteFile(testFile, []byte("test content"), readWritePerms)
54-103: Well-structured table-driven test for client factoryGood use of table-driven tests to verify multiple modes with a single test function.
Consider adding at least one negative test case to verify error handling, such as:
tests := []struct { name string src string dest string mode ClientMode expectedMode getter.ClientMode + expectError bool }{ // existing test cases... + { + name: "Invalid mode", + src: "source.txt", + dest: "dest.txt", + mode: ClientMode(999), // Invalid mode + expectError: true, + },Then update the test logic to handle error expectations.
105-119: Effective detector registration testGood practice saving and restoring the original detectors to prevent test interference.
121-135: Good verification of GoGetterDownloader initializationThe test properly verifies both object creation and detector registration.
Given the similarity with TestRegisterCustomDetectors, consider testing additional GoGetterDownloader functionality beyond initialization if applicable.
…or-better-testability-and
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/go_getter_utils.go (2)
40-51: Consider enhancing scheme validation for broader compatibility.The
IsValidSchemefunction currently supports a limited set of schemes. Since this PR aims to improve the downloader abstraction, consider expanding this list to include all schemes mentioned in the documentation comments below (like "oci", "s3", "gcs", etc.).func IsValidScheme(scheme string) bool { validSchemes := map[string]bool{ "http": true, "https": true, "git": true, "ssh": true, "git::https": true, "git::ssh": true, + "s3": true, + "gcs": true, + "oci": true, + "scp": true, + "sftp": true, + "hg": true, + "file": true, + "dir": true, + "tar": true, + "zip": true, } return validSchemes[scheme] }
53-102: Consider converting comment block to GoDoc format.This detailed comment about supported schemes is valuable information but would be more accessible as proper GoDoc documentation. Consider restructuring it as package or function documentation.
-/* -Supported schemes: - -file, dir, tar, zip -http, https -git, hg -s3, gcs -oci -scp, sftp -Shortcuts like github.com, bitbucket.org - -...rest of the comment... -*/ +// SupportedSchemes lists all schemes that can be used with this package: +// +// - File-related Schemes: +// - file: Local filesystem paths +// - dir: Local directories +// - tar: Tar files, potentially compressed (tar.gz, tar.bz2, etc.) +// - zip: Zip files +// +// - HTTP/HTTPS: +// - http: HTTP URLs +// - https: HTTPS URLs +// +// ...rest of the comment converted to GoDoc format...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/go_getter_utils.go(1 hunks)internal/exec/vendor_model_component.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model_component.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/go_getter_utils.go (4)
5-8: Consider updating imports after refactoring.The imports section now has only two packages (
fmtandstrings), indicating a significant reduction after the downloader logic was moved. This aligns with the refactoring goal mentioned in the PR.
10-10: Good comment addition to clarify function purpose.Adding this descriptive comment improves code readability by clearly stating what the function does.
1-102:✅ Verification successful
Verify impact of removing downloader functionality from this file.
According to the PR summary, the
GoGetterGetand custom GitHub detector functionality have been removed from this file and moved to the newpkg/downloaderpackage. This is a good architectural change that improves testability as mentioned in the objectives.
🏁 Script executed:
#!/bin/bash # Check if the new downloader package has been properly implemented # and is being used where the old functions were used # First, check if the new package exists if [ -d "pkg/downloader" ]; then echo "✅ New downloader package exists" else echo "❌ pkg/downloader package does not exist" exit 1 fi # Look for usages of the new downloader echo "Checking for usages of the new downloader:" rg "downloader\.New" --type go # Check if there are any remaining references to the old functions echo "Checking for any remaining references to GoGetterGet:" rg "GoGetterGet" --type goLength of output: 1240
Downloader Functionality Removal Verified
- The new
pkg/downloaderpackage exists and is actively used (as seen in files likeinternal/exec/yaml_func_include.go,internal/exec/vendor_model.go, etc.).- There are no remaining references to the old
GoGetterGetfunction.- The separation of concerns enhances testability as intended.
No further changes are required.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 13-13: [golangci] internal/exec/go_getter_utils.go#L13
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("URI cannot be empty")" (err113)
11-38:✅ Verification successful
Verification needed: Is ValidateURI still in the right place?
The
ValidateURIfunction remains in this package while download functionality has been moved topkg/downloader. Verify if this function should also be moved to maintain cohesion.
🏁 Script executed:
#!/bin/bash # Check where ValidateURI is being called from rg "ValidateURI\(" --type goLength of output: 156
Final Verification: ValidateURI's Location is Appropriate
Based on our grep results, the
ValidateURIfunction is solely referenced in files within theinternal/execpackage (specifically ingo_getter_utils.goandvendor_utils.go). There’s no indication that it’s being invoked from thepkg/downloaderpackage. Consequently, its placement here maintains logical cohesion.No changes are necessary; please ensure that you continue to monitor that all internal usages remain aligned with this structure.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 13-13: [golangci] internal/exec/go_getter_utils.go#L13
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("URI cannot be empty")" (err113)
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
|
We need to ignore mock_ pattern go files from codecov |
…or-better-testability-and
Add something like this to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
24-24: Good refactoring to use standardized error variables.Using the defined error constants instead of inline error strings improves consistency and allows for better error handling by consumers. This change makes the code more maintainable and enables better error comparison.
Also applies to: 27-28, 32-32, 35-35, 40-40, 45-45
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/go_getter_utils.go (5)
8-8: Imports cleanup looks good.The removal of unnecessary imports matches the refactoring that's moving functionality to the downloader package.
10-17: Good standardization of error variables.Defining these error constants at the package level improves error handling consistency and makes error checking more reliable. This approach allows for better error handling by consumers of this package.
19-20: Well-defined constant for maximum URI size.Defining MaxURISize as a constant follows good practice for magic numbers. The 2048 character limit is a reasonable standard for URIs.
21-21: Clear documentation for ValidateURI function.The function comment clearly explains the purpose, which helps with code maintenance and readability.
1-114: Significant functionality has been moved to a dedicated package.The removal of downloader functionality (CustomGitHubDetector, GoGetterGet) in favor of the new pkg/downloader package is a strong architectural improvement that aligns with the PR objectives. This separation of concerns will:
- Improve testability through proper abstraction
- Make the code more maintainable
- Create a cleaner package structure
The remaining validation logic is focused and appropriate for this package.
…or-better-testability-and
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…or-better-testability-and
|
#1146 |
what
This PR refactors
GoGetterGetandDownloadDetectFormatAndParseFileto improve testability and abstraction. Previously, these were standalone functions ininternal/exec, making them difficult to mock and limiting flexibility in handling different file download mechanisms.Changes Introduced
FileDownloader)gogetterusage, allowing for easy mocking in unit tests.GoGetterGetandDownloadDetectFormatAndParseFileinto a struct implementingFileDownloader.pkg/downloader.why
internal/exec.references
Summary by CodeRabbit
New Features
Refactor
Tests