add artifactory store #921
Conversation
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
mergify
bot
added
conflict
mcalhoun
added
no-release
mcalhoun
force-pushed
the
feature/add-artifactory-store
branch
from
40a0c36 to
a08e985
Compare
📝 WalkthroughWalkthroughThis pull request introduces changes to Atmos' Terraform command handling and storage capabilities. The modifications simplify hook processing in the Terraform command execution, implement a new Artifactory store, and update project dependencies. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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 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: 10
🧹 Nitpick comments (8)
pkg/store/artifactory_store.go (2)
50-56: Consider checking for additional common environment variables.To enhance flexibility, you might want to check for
ARTIFACTORY_API_KEYorJFROG_API_KEYas alternative environment variables for authentication.
196-196: Check the error returned bytempFile.Close().Ensure that any errors from closing the file are handled appropriately.
Add error handling:
defer tempFile.Close() + if err := tempFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %v", err) + }pkg/store/registry.go (1)
11-22: Consistent error handling for store initialization failures.Consider wrapping the error with context to aid in debugging if the store creation fails.
Apply this diff:
store, err := NewArtifactoryStore(opts) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create Artifactory store: %w", err) }pkg/hooks/cmd.go (2)
26-31: Avoid magic prefix check and clarify the condition.Using
strings.Index(value, ".") == 0to check for a leading dot can be simplified and made more readable.Apply this diff:
if strings.Index(value, ".") == 0 { outputValue = e.GetTerraformOutput(&atmosConfig, info.Stack, info.ComponentFromArg, outputKey, true) } else { outputValue = value }Simplify the condition:
- if strings.Index(value, ".") == 0 { + if strings.HasPrefix(value, ".") {
82-87: Handle errors within the loop consistently.Currently, the loop returns on the first error. Consider collecting all errors to provide comprehensive feedback.
pkg/store/artifactory_store_test.go (3)
20-56: Thorough mock implementation with proper error handling.The
DownloadFilesmock implementation:
- Handles error cases first
- Properly manages file creation
- Includes realistic test data
Consider adding logging to help with debugging test failures.
func (m *MockArtifactoryClient) DownloadFiles(params ...services.DownloadParams) (int, int, error) { + // Log mock call parameters for debugging + fmt.Printf("Mock DownloadFiles called with pattern: %s, target: %s\n", params[0].Pattern, params[0].Target) args := m.Called(params[0]) totalDownloaded := args.Int(0) totalFailed := args.Int(1) err := args.Error(2)
63-111: Comprehensive test cases for store initialization.The test suite covers:
- Valid configuration with access token
- Missing access token scenario
- Environment variable handling
Consider adding test cases for:
- Invalid URL format
- Empty repository name
159-234: Thorough error handling test coverage.The test suite effectively covers:
- Download failures
- Empty download scenarios
- Successful downloads
Consider adding test cases for:
- Network timeouts
- Partial downloads
📜 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 (7)
cmd/terraform.go(2 hunks)go.mod(10 hunks)internal/exec/terraform.go(1 hunks)pkg/hooks/cmd.go(1 hunks)pkg/store/artifactory_store.go(1 hunks)pkg/store/artifactory_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/terraform.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/store/artifactory_store.go
203-203: Error return value of tempFile.Write is not checked
(errcheck)
🪛 GitHub Actions: Dependency Review
go.mod
[error] Incompatible license detected: Package 'github.com/nwaples/[email protected]' uses BSD-2-Clause license which is not allowed
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/store/artifactory_store.go (1)
161-165: Verify the file path for the downloaded file.Ensure that the
keyused infilepath.Join(tempDir, key)correctly corresponds to the downloaded file's name.Run the following script to check the files in the temp directory:
✅ Verification successful
The file path construction is correct and secure.
The implementation ensures the downloaded file matches the input key:
- Downloads are performed with
Flat=true, preserving the original filename- The path is safely constructed using
filepath.Join- The same key is used consistently throughout the download and read operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List files in the temp directory to verify the downloaded file name ls "$(find /tmp -maxdepth 1 -type d -name 'artifactorystore*' | tail -n1)"Length of output: 126
Script:
#!/bin/bash # Find the context of the file read operation and key usage ast-grep --pattern 'func $$$($$$) { $$$ downloadedFile := filepath.Join(tempDir, key) $$$ }' # Also search for key assignments rg "key :=" pkg/store/artifactory_store.go -B 2 -A 2Length of output: 155
Script:
#!/bin/bash # Get the content of the file to understand the context cat pkg/store/artifactory_store.goLength of output: 5654
pkg/store/registry.go (1)
11-22: Validate therepo_nameoption before initializing the store.Ensure that
opts.RepoNameis not empty to prevent misconfiguration.Add a check:
if err := parseOptions(storeConfig.Options, &opts); err != nil { return nil, fmt.Errorf("failed to parse Artifactory store options: %w", err) } + + if opts.RepoName == "" { + return nil, fmt.Errorf("repo_name must be specified for Artifactory store") + }✅ Verification successful
The suggested validation is necessary and correctly placed 💪
The
RepoNamefield is required for Artifactory operations, and there's currently no validation to ensure it's provided. Adding this check will prevent configuration issues early with a clear error message.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find ArtifactoryStoreOptions struct definition and NewArtifactoryStore implementation ast-grep --pattern 'type ArtifactoryStoreOptions struct { $$$ }' ast-grep --pattern 'func NewArtifactoryStore($$$) { $$$ }' # Search for any existing repo_name validation rg "RepoName.*=.*\"\"" pkg/Length of output: 692
Script:
#!/bin/bash # Find NewArtifactoryStore implementation with a more flexible pattern rg -A 10 "^func NewArtifactoryStore" pkg/Length of output: 642
cmd/terraform.go (2)
12-12: LGTM! Import statement simplified.The direct import of the hooks package without an alias improves code readability.
59-59: Nice refactoring! Hook processing simplified.The complex hook processing logic has been elegantly abstracted into a single call to
hooks.RunE, which improves maintainability and reduces the chance of errors.pkg/store/artifactory_store_test.go (2)
16-18: Well-structured mock implementation.The
MockArtifactoryClientstruct properly embedsmock.Mockfor mocking functionality.
113-157: Well-structured key generation tests.The test cases effectively cover:
- Simple paths
- Nested components
- Multi-level stacks
go.mod (1)
34-34: New dependency added for Artifactory integration.The addition of
github.com/jfrog/jfrog-client-go v1.49.0aligns with the new Artifactory store implementation.🧰 Tools
🪛 GitHub Actions: Dependency Review
[error] Incompatible license detected: Package 'github.com/nwaples/[email protected]' uses BSD-2-Clause license which is not allowed
mcalhoun
force-pushed
the
feature/add-artifactory-store
branch
from
81f85de to
e4d7bb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/store/artifactory_store.go (2)
57-57:⚠️ Potential issueFix environment variable name in error message.
The error message mentions
ARTIFACTORY_ACCESS_KEY, but the code checks forARTIFACTORY_ACCESS_TOKEN.Apply this diff:
- return "", fmt.Errorf("either access_token must be set in options or one of JFROG_ACCESS_TOKEN or ARTIFACTORY_ACCESS_KEY environment variables must be set") + return "", fmt.Errorf("either access_token must be set in options or one of JFROG_ACCESS_TOKEN or ARTIFACTORY_ACCESS_TOKEN environment variables must be set")
165-167:⚠️ Potential issueImprove error message for download failures.
When
totalFailed > 0, provide more specific information about the failure.Apply this diff:
if totalFailed > 0 { - return nil, fmt.Errorf("failed to download file: %v", err) + return nil, fmt.Errorf("failed to download file: %d files failed to download", totalFailed) }go.mod (1)
223-223:⚠️ Potential issueCritical: Address license compatibility issue.
The
github.com/nwaples/rardecode v1.1.3package uses the BSD-2-Clause license which is not allowed per organization policy.Consider:
- Finding an alternative package with a compatible license
- Obtaining necessary approvals for using BSD-2-Clause licensed code
- Implementing the required functionality without this dependency
🧰 Tools
🪛 GitHub Actions: Dependency Review
[error] Incompatible license detected: Package 'github.com/nwaples/[email protected]' uses BSD-2-Clause license which is not allowed
🧹 Nitpick comments (2)
cmd/terraform.go (1)
Line range hint
94-94: A small battle scar needs attention, brave coder! 🔍There's a typo in the error message: "Unknkown" should be "Unknown".
- fmt.Printf(`Error: Unknkown command %q for %q`+"\n", args[0], cmd.CommandPath()) + fmt.Printf(`Error: Unknown command %q for %q`+"\n", args[0], cmd.CommandPath())pkg/store/artifactory_store.go (1)
84-91: Consider making timeouts and retries configurable.The dial timeout (180s), request timeout (60s), and HTTP retries (0) are hardcoded. Consider making these configurable through
ArtifactoryStoreOptionsfor better flexibility.Apply this diff to add configuration options:
type ArtifactoryStoreOptions struct { AccessToken *string `mapstructure:"access_token"` Prefix *string `mapstructure:"prefix"` RepoName string `mapstructure:"repo_name"` StackDelimiter *string `mapstructure:"stack_delimiter"` URL string `mapstructure:"url"` + DialTimeout *time.Duration `mapstructure:"dial_timeout"` + RequestTimeout *time.Duration `mapstructure:"request_timeout"` + HttpRetries *int `mapstructure:"http_retries"` }
📜 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 (7)
cmd/terraform.go(2 hunks)go.mod(10 hunks)internal/exec/terraform.go(1 hunks)pkg/hooks/cmd.go(1 hunks)pkg/store/artifactory_store.go(1 hunks)pkg/store/artifactory_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/exec/terraform.go
- pkg/store/registry.go
- pkg/hooks/cmd.go
- pkg/store/artifactory_store_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/store/artifactory_store.go
1-1: : # github.com/cloudposse/atmos/pkg/store [github.com/cloudposse/atmos/pkg/store.test]
pkg/store/artifactory_store_test.go:153:14: assignment mismatch: 1 variable but store.getKey returns 2 values
(typecheck)
🪛 GitHub Actions: Dependency Review
go.mod
[error] Incompatible license detected: Package 'github.com/nwaples/[email protected]' uses BSD-2-Clause license which is not allowed
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/terraform.go (2)
12-12: Clean import statement change, warrior! 🛡️The removal of the alias makes the code more direct and battle-ready. Let's verify all references have been updated accordingly.
✅ Verification successful
Import changes are battle-tested and clear, warrior! 🛡️
The removal of the
halias for the hooks package is complete with no remaining references in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of the old 'h.' prefix that might have been missed rg "h\." --type goLength of output: 21906
59-59: By Odin's beard, this is a mighty refactor! 🗡️The simplification of hook processing into a single
hooks.RunEcall is a strategic victory. However, let's verify our battle plan:Let's ensure this warrior handles all the scenarios from the previous implementation!
✅ Verification successful
By the might of Zeus, this refactor stands strong! 💪
The
hooks.RunEimplementation inpkg/hooks/cmd.godemonstrates robust error handling, proper hook processing, and maintains all the essential functionality while providing a cleaner interface. The code properly validates hooks, processes store commands, and propagates errors appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and analyze the hooks.RunE implementation to ensure it covers all cases ast-grep --pattern 'func RunE(cmd *cobra.Command, args []string, info *schema.ConfigAndStacksInfo) error { $$$ }' # Search for any error handling patterns in the old implementation that we should verify rg "if.*err.*!= nil" --type goLength of output: 55667
pkg/store/artifactory_store.go (3)
110-124: LGTM! Path construction is robust.The path construction logic is well-implemented:
- Handles component parts correctly
- Safely joins path segments
- Removes double slashes
188-241: LGTM! Robust implementation with proper cleanup.The implementation is solid:
- Validates input parameters
- Properly handles temporary file cleanup
- Comprehensive error handling
19-24: 🛠️ Refactor suggestionConsider using a non-pointer type for
stackDelimiter.Since
stackDelimiteris a critical field used in path construction, using a non-pointer string with a default value would be safer and prevent potential nil pointer dereferences.Apply this diff:
type ArtifactoryStore struct { prefix string repoName string rtManager ArtifactoryClient - stackDelimiter *string + stackDelimiter string }Likely invalid or redundant comment.
mcalhoun
force-pushed
the
feature/add-artifactory-store
branch
from
e4d7bb1 to
d3af0c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/store/artifactory_store.go (1)
111-113: Remove redundant nil check fors.stackDelimiter.The
s.stackDelimiteris always initialized inNewArtifactoryStore, so this nil check is unnecessary.Apply this diff:
func (s *ArtifactoryStore) getKey(stack string, component string, key string) (string, error) { - if s.stackDelimiter == nil { - return "", fmt.Errorf("stack delimiter is not set") - } stackParts := strings.Split(stack, *s.stackDelimiter)
📜 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 (7)
cmd/terraform.go(2 hunks)go.mod(10 hunks)internal/exec/terraform.go(1 hunks)pkg/hooks/cmd.go(1 hunks)pkg/store/artifactory_store.go(1 hunks)pkg/store/artifactory_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/store/registry.go
- internal/exec/terraform.go
- pkg/hooks/cmd.go
- pkg/store/artifactory_store_test.go
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/store/artifactory_store.go (1)
160-167: Handle potentialnilerror when download fails.When
totalFailed > 0, theerrmight benil. Adjust the error message to reflect the actual issue.Apply this diff:
if totalFailed > 0 { - return nil, fmt.Errorf("failed to download file: %v", err) + return nil, fmt.Errorf("failed to download file: %d files failed to download", totalFailed) }cmd/terraform.go (1)
59-59: Streamline hook execution inPostRunE.The simplification using
hooks.RunEenhances code readability and maintainability.
|
These changes were released in v1.148.1. |
what
why
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Dependencies
Tests