Enhanced OCI Registry Authentication and Layer Extraction Support #1413
Conversation
…e need to allow authentication to private registries. Signed-off-by: Chris Harden <[email protected]>
…ed to oci_utils.go Signed-off-by: Chris Harden <[email protected]>
📝 WalkthroughWalkthroughAdds multi-source OCI registry authentication (GHCR, Docker, env, ECR, ACR, GCR), threads OCI settings into the pull flow, implements per-layer extraction with ZIP/TAR dispatch and hardened ZIP protections, extends schema/config for OCI settings, bumps AWS SDK deps, and adds extensive auth/extraction tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant OCI as pullImage/processOciImage
participant Auth as getRegistryAuth
participant Env as Viper/Env
participant Docker as Docker config/creds
participant ECR as AWS ECR
participant ACR as Azure ACR
participant GCR as Google GCR
Caller->>OCI: pullImage(imageRef, atmosConfig)
OCI->>Auth: getRegistryAuth(registry, atmosConfig)
Auth->>Env: check GHCR env/atmos vars
Auth->>Docker: check docker config.json & cred helpers
Auth->>ECR: attempt ECR auth (AWS SDK)
Auth->>ACR: attempt ACR auth (service-principal/default)
Auth->>GCR: attempt GCR (ADC)
Auth-->>OCI: authenticator or error
alt authenticator returned
OCI->>Registry: pull with auth
else
OCI->>Registry: pull anonymously
end
sequenceDiagram
autonumber
participant OCI as processOciImage
participant Layer as image layer
participant ZIP as extractZipFile
participant TAR as tar extractor
participant RAW as extractRawData
OCI->>Layer: iterate layers
alt mediaType contains "zip"
OCI->>ZIP: extractZipFile(reader, dest)
alt success
ZIP-->>OCI: files extracted
else
ZIP-->>OCI: error
OCI->>RAW: extractRawData(reader, dest, idx)
end
else
OCI->>TAR: extract tar
alt success
TAR-->>OCI: files extracted
else
TAR-->>OCI: error
OCI->>RAW: extractRawData(reader, dest, idx)
end
end
OCI-->>OCI: log per-layer outcomes and summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
internal/exec/oci_utils.go (4)
329-345: Cloud provider auth functions need implementationThe ECR authentication placeholder returns an error indicating incomplete implementation. Consider implementing or clearly documenting this as a limitation.
Would you like me to help implement the AWS ECR authentication using the AWS SDK? I can open an issue to track this enhancement.
348-356: ACR auth needs implementationAzure Container Registry authentication is not fully implemented.
Would you like me to help implement the Azure ACR authentication? I can create an issue to track this.
359-367: GCR auth needs implementationGoogle Container Registry authentication is not fully implemented.
Would you like me to help implement the Google GCR authentication? I can create an issue to track this.
450-468: Consider adding file permissions to raw data extractionThe raw data extraction creates files with default permissions. Consider setting appropriate permissions.
func extractRawData(reader io.Reader, destDir string, layerIndex int) error { // Create a temporary file to store the raw data tempFile := filepath.Join(destDir, fmt.Sprintf("layer_%d_raw", layerIndex)) file, err := os.Create(tempFile) if err != nil { return fmt.Errorf("failed to create temp file: %w", err) } defer file.Close() // Copy the raw data _, err = io.Copy(file, reader) if err != nil { return fmt.Errorf("failed to copy raw data: %w", err) } + // Set appropriate permissions + if err := os.Chmod(tempFile, 0644); err != nil { + log.Warn("Failed to set permissions on raw data file", "file", tempFile, "error", err) + } + log.Debug("Extracted raw data to temp file", "file", tempFile) return nil }internal/exec/oci_utils_test.go (5)
146-148: Test modifies global HOME environment variableThe test modifies the HOME environment variable which could affect parallel test execution.
Consider using
t.Setenvfor automatic cleanup:- originalHome := os.Getenv("HOME") - os.Setenv("HOME", tempDir) - defer os.Setenv("HOME", originalHome) + t.Setenv("HOME", tempDir)
35-37: Use t.Setenv for environment variable managementMultiple test functions manually set and unset environment variables. Use
t.Setenvfor automatic cleanup.Replace all
os.Setenvandos.Unsetenvcalls witht.Setenv:setupEnv: func() { - os.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_TOKEN", "test-token") },Also applies to: 44-46, 54-56, 64-66, 101-102, 110-111, 119-120
238-288: Consider capturing log output for artifact type validationThe test acknowledges that
checkArtifactTypeonly logs. Consider capturing logs to verify behavior.You could use a log capture mechanism to verify the function's behavior instead of just calling it without assertions.
450-475: Integration test needs enhancementThe integration test only validates error cases. Consider adding mock registry interactions for positive test cases.
Consider using a mock OCI registry or the go-containerregistry test helpers to create more comprehensive integration tests.
511-520: Benchmark should reset environment after testThe benchmark sets an environment variable but doesn't clean it up.
func BenchmarkGetRegistryAuth(b *testing.B) { // Setup test environment - os.Setenv("GITHUB_TOKEN", "test-token") + b.Setenv("GITHUB_TOKEN", "test-token") b.ResetTimer() for i := 0; i < b.N; i++ { _, _ = getRegistryAuth("ghcr.io") } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exec/oci_utils.go(7 hunks)internal/exec/oci_utils_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
internal/exec/oci_utils_test.gointernal/exec/oci_utils.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
Files:
internal/exec/oci_utils_test.go
🧠 Learnings (1)
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Include integration tests for command flows
Applied to files:
internal/exec/oci_utils_test.go
🧬 Code graph analysis (1)
internal/exec/oci_utils_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(25-60)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (11)
internal/exec/oci_utils.go (8)
106-113: Authentication fallback logic looks goodThe fallback to anonymous authentication when no credentials are found is a sensible approach for public registries.
124-189: Comprehensive authentication orchestrationThe multi-source authentication flow is well-structured with clear precedence order and appropriate logging at each step.
76-93: Good error resilience in layer processingThe approach to continue processing other layers when one fails improves robustness. The summary logging provides useful debugging information.
403-412: Smart format detection for layer extractionThe media type-based extraction with ZIP detection is a good approach for handling different artifact formats.
414-447: Robust fallback extraction strategyThe multi-level fallback (ZIP → TAR → raw data) with special handling for metadata layers shows good defensive programming.
471-523: ZIP extraction implementation is solidThe ZIP extraction properly handles directory creation and file extraction. Good error handling throughout.
533-552: Artifact type checking is comprehensiveGood support for multiple artifact types with clear logging of supported vs unsupported types.
277-309: Potential command injection vulnerability in credential store executionThe
registryparameter is directly concatenated into the command execution without validation. A malicious registry name could potentially inject commands.Add input validation:
func getCredentialStoreAuth(registry, credsStore string) (authn.Authenticator, error) { + // Validate registry to prevent command injection + if strings.ContainsAny(registry, ";&|`$(){}[]<>'\"\n\r") { + return nil, fmt.Errorf("invalid registry name: %s", registry) + } + + // Validate credsStore to prevent command injection + if strings.ContainsAny(credsStore, ";&|`$(){}[]<>/\\") { + return nil, fmt.Errorf("invalid credential store name: %s", credsStore) + } + // For Docker Desktop on macOS, the credential store is typically "desktop" // We need to use the docker-credential-desktop helper to get credentials // Try to execute the credential helper cmd := exec.Command("docker-credential-"+credsStore, "get")Likely an incorrect or invalid review comment.
internal/exec/oci_utils_test.go (3)
23-170: Comprehensive authentication test coverageThe table-driven tests cover all authentication sources well, including edge cases and placeholder implementations.
172-216: ZIP extraction test is thoroughGood test coverage including subdirectories and content verification.
477-509: Benchmark tests are valuableGood addition of benchmarks for performance-critical operations.
…on, test environment cleanup, and enhanced test coverage Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/exec/oci_utils.go (2)
165-170: Incomplete ECR authentication implementationThe ECR authentication returns an error indicating it's not fully implemented. This should either be completed or clearly documented as a limitation.
Would you like me to help implement the complete ECR authentication using the AWS SDK, or should we open an issue to track this?
483-486: Memory concern with ReadAll for ZIP extractionReading entire ZIP files into memory could cause issues with large archives.
Consider streaming extraction for large files or add a size limit check:
+const maxZipSize = 500 * 1024 * 1024 // 500MB limit + +// Check layer size first if available +if size, err := layer.Size(); err == nil && size > maxZipSize { + return fmt.Errorf("ZIP file too large: %d bytes (max: %d)", size, maxZipSize) +} + zipData, err := io.ReadAll(reader)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exec/oci_utils.go(7 hunks)internal/exec/oci_utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/oci_utils_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
internal/exec/oci_utils.go
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/oci_utils.go (4)
76-83: Good defensive approach to layer processingTracking successful layers and continuing despite failures improves resilience when dealing with problematic OCI images.
106-113: Solid fallback to anonymous authThe authentication flow properly defaults to anonymous access when credentials aren't found, maintaining backward compatibility.
280-287: Good command injection preventionThe validation checks for potentially dangerous characters prevent command injection attacks when interfacing with external credential helpers.
152-155: Potential security concern with environment variable namingThe current implementation allows dots in registry names to be replaced with underscores, which could lead to naming collisions. For example, both
my.registry.comandmy-registry.comwould map to the same environment variablesMY_REGISTRY_COM_USERNAMEandMY_REGISTRY_COM_PASSWORD.Consider using a more unique separator or hash-based approach:
-registryEnvName := strings.ToUpper(strings.ReplaceAll(registry, ".", "_")) +// Replace dots and hyphens with double underscores to avoid collisions +registryEnvName := strings.ToUpper(strings.ReplaceAll(strings.ReplaceAll(registry, ".", "__"), "-", "___"))⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-07-01T02:22:25.901Z Learning: Applies to **/*.go : Use snake_case for environment variables
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/exec/oci_utils.go (1)
295-303: Add timeout to docker-credential helper execution to avoid hangsThis was raised previously and remains. Add a 5s timeout and return a clear error when exceeded.
- cmd := exec.Command("docker-credential-"+credsStore, "get") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "docker-credential-"+credsStore, "get") cmd.Stdin = strings.NewReader(registry) - output, err := cmd.Output() + output, err := cmd.Output() if err != nil { - return nil, fmt.Errorf("failed to get credentials from store %s: %w", credsStore, err) + if ctx.Err() == context.DeadlineExceeded { + return nil, fmt.Errorf("credential helper %q timed out", credsStore) + } + return nil, fmt.Errorf("failed to get credentials from store %s: %w", credsStore, err) }
🧹 Nitpick comments (3)
internal/exec/oci_utils.go (3)
3-21: Unify error handling (drop github.com/pkg/errors) and add time import for helper timeoutsYou’re already using fmt.Errorf with %w everywhere. Keeping github.com/pkg/errors only for errors.New is unnecessary and can confuse linters. Also, we’ll need time for credential-helper timeouts below.
Apply this diff to the imports:
import ( "archive/zip" "bytes" "context" "encoding/base64" "encoding/json" "fmt" "io" "os" "os/exec" "path/filepath" "strings" log "github.com/charmbracelet/log" // Charmbracelet structured logger - "github.com/pkg/errors" + "errors" + "time" "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/ecr" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/google/uuid" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/cloudposse/atmos/pkg/schema" )No other code changes required besides the import swap; var ErrNoLayers will now bind to the stdlib errors package, which is desired.
468-477: Detect ZIP by magic bytes as well, not just mediaType substringLayer media types are often tar+gzip. Some registries may store ZIP data under generic/custom types. Lightweight sniffing reduces false negatives.
- // Check if it's a ZIP file - mediaTypeStr := string(mediaType) - if strings.Contains(mediaTypeStr, "zip") { + mediaTypeStr := string(mediaType) + // Peek first 4 bytes to detect ZIP magic (PK\x03\x04) + header := make([]byte, 4) + n, _ := io.ReadFull(uncompressed, header) + if n > 0 { + uncompressed = io.NopCloser(io.MultiReader(bytes.NewReader(header[:n]), uncompressed)) + } + if (n == 4 && bytes.Equal(header[:4], []byte("PK\x03\x04"))) || strings.Contains(mediaTypeStr, "zip") { log.Debug("Detected ZIP layer, extracting as ZIP", "index", index, "digest", layerDesc, "mediaType", mediaTypeStr) extractionErr = extractZipFile(uncompressed, destDir) } else { // Default to tar extraction log.Debug("Extracting as TAR", "index", index, "digest", layerDesc, "mediaType", mediaTypeStr) extractionErr = extractTarball(uncompressed, destDir) }
109-116: Differentiate “no creds” from “error reading creds”Right now any error from getRegistryAuth is treated as “no auth found”. Consider a sentinel error (e.g., ErrNoRegistryAuth) to distinguish “not found” from real failures (e.g., unreadable $HOME or malformed Docker config) and log accordingly.
Optional small refactor:
- getRegistryAuth returns (authn.Authenticator, error) where error is ErrNoRegistryAuth for the “not found” case.
- Here, if errors.Is(err, ErrNoRegistryAuth) then fall back to anonymous; otherwise log a warning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)internal/exec/oci_utils.go(7 hunks)internal/exec/oci_utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/oci_utils_test.go
🧰 Additional context used
📓 Path-based instructions (2)
go.mod
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Manage dependencies with Go modules
Files:
go.mod
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
internal/exec/oci_utils.go
🧠 Learnings (1)
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to go.mod : Manage dependencies with Go modules
Applied to files:
go.mod
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
go.mod (2)
24-24: ECR SDK dependency addition looks goodAdding github.com/aws/aws-sdk-go-v2/service/ecr v1.49.2 aligns with the new getECRAuth implementation in internal/exec/oci_utils.go. No concerns from me here.
3-3: Pin minimum Go version and lock patch release
Thegodirective in go.mod may include a patch version to express the minimum required Go version (e.g.,go 1.24.5), but if you need to suggest or enforce a specific patch release for reproducible builds, you should use thetoolchaindirective.Locations to update:
- go.mod: line 3
Proposed diff:
-go 1.24.5 +go 1.24 +toolchain go1.24.5Please confirm that your CI/build environment is using Go 1.21 or newer—so it supports the
toolchaindirective. If it does not yet, you can either keepgo 1.24.5(allowed as a minimum version) or drop the patch in the Go directive (go 1.24) without adding atoolchainline.internal/exec/oci_utils.go (2)
485-495: Good fix: close old reader before resettingClosing uncompressed before reassigning avoids leaks. This addresses prior concerns.
606-625: Artifact-type check expansion LGTMSupportedTypes list and logging behavior look good and match the PR scope.
aknysh
added
no-release
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
golangci-lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Invalid Go version directive; use major.minor and optionally pin toolchainThe
godirective must be major.minor (e.g.,go 1.24), not a patch version. Consider adding a separatetoolchaindirective if you want to pin 1.24.5.Apply this diff:
-go 1.24.5 +go 1.24 +toolchain go1.24.5
♻️ Duplicate comments (1)
internal/exec/oci_utils.go (1)
296-301: Add timeout to credential helper exec to avoid hangs- cmd := exec.Command("docker-credential-"+credsStore, "get") - cmd.Stdin = strings.NewReader(registry) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "docker-credential-"+credsStore, "get") + cmd.Stdin = strings.NewReader(registry)Also add
timeto imports.import ( "archive/zip" "bytes" "context" "encoding/base64" "encoding/json" "fmt" "io" "os" "os/exec" "path/filepath" "strings" + "time" )
🧹 Nitpick comments (4)
internal/exec/oci_utils.go (4)
101-116: Add request timeout when pulling imagesGuard against network hangs by using a context with timeout for remote.Get.
-func pullImage(ref name.Reference) (*remote.Descriptor, error) { - var opts []remote.Option +func pullImage(ref name.Reference) (*remote.Descriptor, error) { + var opts []remote.Option + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() @@ - descriptor, err := remote.Get(ref, opts...) + opts = append(opts, remote.WithContext(ctx)) + descriptor, err := remote.Get(ref, opts...)
79-86: Post-extraction checks may miscount due to pre-existing filesIf destDir already has files, fileCount isn’t the number of extracted files. Consider ensuring destDir exists and capturing pre/post counts.
successfulLayers++ } // Check if any files were actually extracted -files, err := os.ReadDir(destDir) +_ = os.MkdirAll(destDir, 0o755) +files, err := os.ReadDir(destDir)Also applies to: 88-96
540-551: Avoid loading entire ZIP into memory; spool to temp fileReading all data with io.ReadAll can blow memory on large layers. Stream to a temp file and use zip.OpenReader.
- // Read the entire ZIP data into memory - zipData, err := io.ReadAll(reader) - if err != nil { - return fmt.Errorf("failed to read ZIP data: %w", err) - } - - // Create a ZIP reader - zipReader, err := zip.NewReader(bytes.NewReader(zipData), int64(len(zipData))) + tmpf, err := os.CreateTemp("", "atmos-zip-*") + if err != nil { + return fmt.Errorf("failed to create temp file for ZIP: %w", err) + } + // Ensure temp file is removed + tmpName := tmpf.Name() + defer func() { tmpf.Close(); os.Remove(tmpName) }() + if _, err := io.Copy(tmpf, reader); err != nil { + return fmt.Errorf("failed to buffer ZIP data: %w", err) + } + if _, err := tmpf.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("failed to rewind ZIP temp file: %w", err) + } + zr, err := zip.OpenReader(tmpName) if err != nil { return fmt.Errorf("failed to create ZIP reader: %w", err) } + defer zr.Close()And iterate
for _, file := range zr.File { ... }.
341-410: Cloud-provider auth stubs: document and feature-flag to avoid confusing logsACR/GCR helpers intentionally return errors. Consider adding TODOs with a short rationale and feature flags or log.Info to avoid implying partial success.
I can add TODOs and a simple config flag (e.g., atmos.vendor.allowCloudAuth) wired via Viper to guard these paths. Want a patch?
Also applies to: 412-432
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(2 hunks)internal/exec/oci_utils.go(7 hunks)internal/exec/oci_utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/oci_utils_test.go
🧰 Additional context used
📓 Path-based instructions (2)
go.mod
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Manage dependencies with Go modules
Files:
go.mod
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
internal/exec/oci_utils.go
🧠 Learnings (1)
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to go.mod : Manage dependencies with Go modules
Applied to files:
go.mod
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
go.mod (2)
133-134: Sync internal AWS modules with core SDKBumping internal/configsources and internal/endpoints/v2 is good to keep parity with the updated core SDK. No action needed beyond ensuring CI passes with these versions.
17-22: Verification successful: AWS ECR dependency cleanly integratedThe
go mod tidyandgo buildcommands exited without errors, confirming that:
- The direct dependency on
github.com/aws/aws-sdk-go-v2/service/ecr v1.49.2is correctly resolved.- All aws-sdk-go-v2 module versions remain consistent and no transitive conflicts were introduced.
Everything is good to merge.
internal/exec/oci_utils.go (1)
598-626: Artifact type checks look goodRecognition of multiple Terraform/OpenTofu artifact types with clear logging is on point.
Docker Config: ~/.docker/config.json + DOCKER_CONFIG env var Environment Variables: Registry-specific auth vars AWS ECR: Full implementation with region extraction Azure ACR: Service principal + Azure CLI support Terraform: TF_TOKEN_* env vars + credentials.tfrc.json GitHub Container Registry: GITHUB_TOKEN support Signed-off-by: Chris Harden <[email protected]>
Docker cred helper AWS ECR optimizations Signed-off-by: Chris Harden <[email protected]>
…mes for hyphenated registries Signed-off-by: Chris Harden <[email protected]>
…lpers; expand auth lookup Signed-off-by: Chris Harden <[email protected]>
…egion, and prefer matching ProxyEndpoint Signed-off-by: Chris Harden <[email protected]>
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exec/oci_utils.go (1)
335-341: Add a timeout for docker credential helper executionExternal helpers can hang and block pulls. Use CommandContext with a sane timeout.
Apply this diff:
- cmd := exec.Command("docker-credential-"+credsStore, "get") - cmd.Stdin = strings.NewReader(registry) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "docker-credential-"+credsStore, "get") + cmd.Stdin = strings.NewReader(registry)And ensure the import is present:
// at top import "time"
🧹 Nitpick comments (7)
internal/exec/oci_utils.go (7)
595-632: Avoid double-closing the layer reader and simplify fallback flowYou defer Close() on the initial reader, then close it manually and add a second defer after reassigning. While usually harmless, it's brittle and can mask errors.
Refactor to a small helper that obtains a fresh Uncompressed() per attempt and closes it exactly once per attempt:
extract := func(fn func(io.Reader) error) error { r, err := layer.Uncompressed() if err != nil { return fmt.Errorf("layer decompression error: %w", err) } defer r.Close() return fn(r) } if strings.Contains(mediaTypeStr, "zip") { extractionErr = extract(func(r io.Reader) error { return extractZipFile(r, destDir) }) } else { extractionErr = extract(func(r io.Reader) error { return extractTarball(r, destDir) }) } if extractionErr != nil { if err := extract(func(r io.Reader) error { return extractRawData(r, destDir, index) }); err != nil { if index == 0 { log.Warn("..."); return nil } return fmt.Errorf("all extraction methods failed: %w", err) } }
678-687: Don’t load entire ZIP into memory; spool to a temp file or streamReading all ZIP bytes can OOM on large layers. Prefer spooling to disk and using zip.OpenReader.
Apply this diff to spool to a temp file (and clean it up):
- // Read the entire ZIP data into memory - zipData, err := io.ReadAll(reader) - if err != nil { - return fmt.Errorf("failed to read ZIP data: %w", err) - } - - // Create a ZIP reader - zipReader, err := zip.NewReader(bytes.NewReader(zipData), int64(len(zipData))) - if err != nil { - return fmt.Errorf("failed to create ZIP reader: %w", err) - } + // Spool ZIP to a temp file to avoid unbounded memory usage + tmpf, err := os.CreateTemp("", "atmos_zip_*") + if err != nil { + return fmt.Errorf("failed to create temp ZIP file: %w", err) + } + tmpName := tmpf.Name() + defer func() { tmpf.Close(); os.Remove(tmpName) }() + if _, err := io.Copy(tmpf, reader); err != nil { + return fmt.Errorf("failed to buffer ZIP data: %w", err) + } + if _, err := tmpf.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("failed to rewind temp ZIP file: %w", err) + } + zipReader, err := zip.OpenReader(tmpName) + if err != nil { + return fmt.Errorf("failed to open ZIP: %w", err) + } + defer zipReader.Close()Follow-ups:
- Consider a size cap and reject overly large archives early.
101-116: Remote pull lacks a context; add a timeout to avoid indefinite hangsNetwork pulls can stall. Pass remote.WithContext with a timeout.
Apply this diff:
func pullImage(ref name.Reference) (*remote.Descriptor, error) { - var opts []remote.Option + var opts []remote.Option + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + opts = append(opts, remote.WithContext(ctx))Optionally make the timeout configurable (flags/env/config via Viper).
Also applies to: 118-125
136-142: Prefer Viper over direct os.Getenv for env-backed configurationAtmos prefers Viper-managed env access for consistency and testability. This applies to GITHUB_TOKEN, DOCKER_CONFIG, AZURE_, AZURE_CLI_AUTH, TF_TOKEN_, and GCP envs.
I’m leveraging our prior learning about avoiding direct os.Getenv. A minimal migration pattern:
import "github.com/spf13/viper" func init() { viper.AutomaticEnv() viper.BindEnv("github_token", githubTokenEnv) viper.BindEnv("docker_config", "DOCKER_CONFIG") // ... bind other keys as needed ... } // usage if token := viper.GetString("github_token"); token != "" { ... } configDir := viper.GetString("docker_config")This keeps precedence consistent (flags > env > config > defaults) and eases testing.
Also applies to: 145-151, 160-167, 207-217, 462-466, 470-473, 563-566, 801-804
43-48: Remove unused tempDirtempDir is created and removed but never used.
Apply this diff:
- tempDir, err := os.MkdirTemp("", uuid.New().String()) - if err != nil { - return fmt.Errorf("failed to create temp directory: %w", err) - } - defer removeTempDir(tempDir) + // no temp directory needed here
757-761: Doc comment tweak for parseOCIManifestThe function reads from an io.Reader, not necessarily a “JSON file”.
Apply this diff:
-// ParseOCIManifest reads and decodes an OCI manifest from a JSON file. +// parseOCIManifest reads and decodes an OCI manifest from a JSON stream.Also applies to: 787-795
319-361: Abstract external dependencies to interfaces for better testabilityDirect calls to exec.Command and AWS SDK make unit testing harder. Thin interfaces would let tests inject fakes.
Example:
type CredHelper interface { Get(registry, helper string, timeout time.Duration) ([]byte, error) } type ECRClient interface { GetAuthorizationToken(ctx context.Context, in *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) }Provide default implementations in production; use fakes in tests. This will also simplify injecting timeouts.
Also applies to: 397-407, 503-505, 521-526
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exec/oci_utils.go(8 hunks)internal/exec/oci_utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/oci_utils_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
internal/exec/oci_utils.go
🧠 Learnings (5)
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/oci_utils.go
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/oci_utils.go (3)
702-719: Zip Slip protection looks solidGood defense-in-depth: absolute path checks, Windows drive handling, clean/join, and prefix validation. Skipping symlinks is also prudent.
766-784: Supported artifact type check is clear and appropriately lenientGood: logs supported types and doesn’t hard-fail. This will help users diagnose unexpected media types without breaking workflows.
127-201: Confirm intended auth precedence (Terraform vs Docker vs env)Current order: GHCR token → Terraform (TF_TOKEN_* / tfrc) → Docker config/cred helpers → REGISTRY_NAME_{USERNAME,PASSWORD} → ECR/ACR/GCR. The PR summary previously listed Docker/env ahead of Terraform. Please confirm the intended precedence and update docs/tests accordingly.
If Terraform should come after Docker and env, reorder here and adjust tests. If this is the new desired order, please note it in the README/CHANGELOG.
Signed-off-by: Chris Harden <[email protected]>
Signed-off-by: Chris Harden <[email protected]>
Pushed |
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (18)
internal/exec/oci_azure_test.go (5)
21-24: DRY the E2E skip checks.Replace repeated env checks with a helper to keep tests tidy.
@@ -func TestACRAuthDirect(t *testing.T) { - if os.Getenv("ATMOS_AZURE_E2E") == "" { - t.Skip("Skipping Azure integration test (set ATMOS_AZURE_E2E=1 to run)") - } +func TestACRAuthDirect(t *testing.T) { + azureSkipIfNotE2E(t) @@ -func TestGetACRAuthViaServicePrincipalDirect(t *testing.T) { - if os.Getenv("ATMOS_AZURE_E2E") == "" { - t.Skip("Skipping Azure integration test (set ATMOS_AZURE_E2E=1 to run)") - } +func TestGetACRAuthViaServicePrincipalDirect(t *testing.T) { + azureSkipIfNotE2E(t) @@ -func TestGetACRAuthViaDefaultCredentialDirect(t *testing.T) { - if os.Getenv("ATMOS_AZURE_E2E") == "" { - t.Skip("Skipping Azure integration test (set ATMOS_AZURE_E2E=1 to run)") - } +func TestGetACRAuthViaDefaultCredentialDirect(t *testing.T) { + azureSkipIfNotE2E(t) @@ -func TestExchangeAADForACRRefreshTokenDirect(t *testing.T) { - if os.Getenv("ATMOS_AZURE_E2E") == "" { - t.Skip("Skipping Azure integration test (set ATMOS_AZURE_E2E=1 to run)") - } +func TestExchangeAADForACRRefreshTokenDirect(t *testing.T) { + azureSkipIfNotE2E(t) + +} + +func azureSkipIfNotE2E(t *testing.T) { + if os.Getenv("ATMOS_AZURE_E2E") == "" { + t.Skip("Skipping Azure integration test (set ATMOS_AZURE_E2E=1 to run)") + } }Also applies to: 87-90, 161-164, 213-216
299-333: Assert request details in the stubbed HTTP flow.Validate method/host/headers to catch regressions without hitting the network.
@@ - mockClient := &http.Client{ - Timeout: 30 * time.Second, - Transport: &mockTransport{ - response: &http.Response{ - StatusCode: http.StatusOK, - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(`{"refresh_token": "mock-refresh-token", "access_token": "mock-access-token"}`)), - }, - }, - } + mockClient := &http.Client{ + Timeout: 10 * time.Second, + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + assert.Equal(t, http.MethodPost, req.Method) + assert.Equal(t, "test.azurecr.io", req.URL.Host) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, "Bearer test-token", req.Header.Get("Authorization")) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(`{"refresh_token": "mock-refresh-token", "access_token": "mock-access-token"}`)), + }, nil + }), + } @@ -// mockTransport is a simple mock transport for testing -type mockTransport struct { - response *http.Response -} - -func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { - return m.response, nil -} +// roundTripFunc helps inline stub HTTP behavior in tests. +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +}Also applies to: 335-342
344-412: Parallelize pure JWT parsing tests.Safe speed-up; avoids coupling with E2E tests.
func TestExtractTenantIDFromTokenDirect(t *testing.T) { + t.Parallel() tests := []struct { @@ - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() tenantID, err := extractTenantIDFromToken(tt.token)
85-85: Finish comments with periods.Minor doc polish per repo guidelines.
-// TestGetACRAuthViaServicePrincipalDirect tests Azure Service Principal authentication directly +// TestGetACRAuthViaServicePrincipalDirect tests Azure Service Principal authentication directly. @@ -// mockTransport is a simple mock transport for testing +// mockTransport is a simple mock transport for testing.Also applies to: 335-335
132-140: Prefer explicit input validation for missing client ID.A deterministic validation error is better than bubbling up SDK errors.
- expectError: true, - errorMsg: "failed to get Azure token", + expectError: true, + errorMsg: "client ID is required",If you adopt this, ensure getACRAuthViaServicePrincipal validates inputs and returns wrapped errors like:
fmt.Errorf("azure service principal auth: client ID is required").internal/exec/oci_google.go (3)
42-44: Fix outdated comment to match implementation.The comment still mentions "_dcg" username; code uses "oauth2accesstoken".
Apply this diff:
-// For GCR, we use the token as the password with "_dcg" as username -// This is the standard pattern for GCR authentication +// For GCR/Artifact Registry, use an OAuth2 access token as the password with +// the username "oauth2accesstoken". This is the standard pattern for GCR/AR authentication.
20-48: Prefer google.Keychain for ADC/gcloud and per-registry auth.Keychain handles *.gcr.io and *.pkg.dev seamlessly (ADC, gcloud), and avoids manual token plumbing.
Apply this diff:
import ( - "context" - "errors" - "fmt" - - log "github.com/charmbracelet/log" - "github.com/google/go-containerregistry/pkg/authn" - "golang.org/x/oauth2/google" + "context" + "errors" + "fmt" + + log "github.com/charmbracelet/log" + "github.com/google/go-containerregistry/pkg/authn" + gck "github.com/google/go-containerregistry/pkg/authn/google" + "github.com/google/go-containerregistry/pkg/name" ) @@ // getGCRAuth attempts to get Google Container Registry authentication. func getGCRAuth(registry string) (authn.Authenticator, error) { - // Use Google Cloud Application Default Credentials - ctx := context.Background() - creds, err := google.FindDefaultCredentials(ctx, "https://www.googleapis.com/auth/cloud-platform") - if err != nil { - log.Debug("Failed to find Google Cloud credentials", logFieldRegistry, registry, "error", err) - return nil, fmt.Errorf("%w: %w", errFailedToFindGoogleCloudCredentials, err) - } - - if creds == nil || creds.TokenSource == nil { - log.Debug("No Google Cloud credentials found", logFieldRegistry, registry) - return nil, fmt.Errorf("%w %s", errNoGoogleCloudCredentialsFound, registry) - } - - // Get a token from the credentials - token, err := creds.TokenSource.Token() - if err != nil { - log.Debug("Failed to get Google Cloud token", logFieldRegistry, registry, "error", err) - return nil, fmt.Errorf("%w: %w", errFailedToGetGoogleCloudToken, err) - } - - // For GCR, we use the token as the password with "_dcg" as username - // This is the standard pattern for GCR authentication - log.Debug("Successfully obtained Google Cloud credentials", logFieldRegistry, registry) - return &authn.Basic{ - Username: "oauth2accesstoken", - Password: token.AccessToken, - }, nil + if registry == "" { + return nil, fmt.Errorf("empty registry") + } + r, err := name.NewRegistry(registry, name.WeakValidation) + if err != nil { + return nil, fmt.Errorf("invalid registry %q: %w", registry, err) + } + // Let google.Keychain resolve ADC/gcloud for this registry. + auth, err := gck.Keychain.Resolve(r) + if err != nil { + log.Debug("No Google credentials resolved", logFieldRegistry, registry, "error", err) + return nil, fmt.Errorf("%w: %w", errNoGoogleCloudCredentialsFound, err) + } + log.Debug("Successfully resolved Google credentials", logFieldRegistry, registry) + return auth, nil }
22-39: Add a short timeout to ADC token discovery.Prevents hangs in misconfigured environments.
Apply this diff (if keeping the current ADC flow):
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel()And add:
import ( "context" + "time"internal/exec/oci_aws_test.go (2)
10-46: LGTM on negative cases; add t.Parallel.Parallelize tests and subtests for speed and isolation.
Apply this diff:
func TestECRAuthDirect(t *testing.T) { + t.Parallel() @@ - for _, tt := range tests { + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() _, err := getECRAuth(tt.registry)
48-93: Exercise the actual parser instead of string heuristics.Use parseECRRegistry to validate account/region extraction across cases.
Apply this diff:
-// TestECRRegistryParsing tests the ECR registry parsing logic. +// TestECRRegistryParsing tests the ECR registry parsing logic. func TestECRRegistryParsing(t *testing.T) { - tests := []struct { - name string - registry string - isECR bool - }{ - { - name: "Standard ECR", - registry: "123456789012.dkr.ecr.us-west-2.amazonaws.com", - isECR: true, - }, - { - name: "ECR FIPS", - registry: "123456789012.dkr.ecr-fips.us-west-2.amazonaws.com", - isECR: true, - }, - { - name: "ECR China", - registry: "123456789012.dkr.ecr.cn-northwest-1.amazonaws.com.cn", - isECR: true, - }, - { - name: "ECR FIPS China", - registry: "123456789012.dkr.ecr-fips.cn-northwest-1.amazonaws.com.cn", - isECR: true, - }, - { - name: "Non-ECR registry", - registry: "docker.io", - isECR: false, - }, - { - name: "Invalid format", - registry: "invalid-registry.com", - isECR: false, - }, - } + t.Parallel() + tests := []struct { + name string + registry string + wantAcct string + wantRegion string + expectError bool + }{ + {"Standard ECR", "123456789012.dkr.ecr.us-west-2.amazonaws.com", "123456789012", "us-west-2", false}, + {"ECR FIPS", "123456789012.dkr.ecr-fips.us-west-2.amazonaws.com", "123456789012", "us-west-2", false}, + {"ECR China", "123456789012.dkr.ecr.cn-northwest-1.amazonaws.com.cn", "123456789012", "cn-northwest-1", false}, + {"ECR FIPS China", "123456789012.dkr.ecr-fips.cn-northwest-1.amazonaws.com.cn", "123456789012", "cn-northwest-1", false}, + {"Non-ECR registry", "docker.io", "", "", true}, + {"Invalid format", "invalid-registry.com", "", "", true}, + } @@ - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - isECR := strings.Contains(tt.registry, "dkr.ecr") && strings.Contains(tt.registry, "amazonaws.com") - assert.Equal(t, tt.isECR, isECR) - }) - } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + acct, region, err := parseECRRegistry(tt.registry) + if tt.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.wantAcct, acct) + assert.Equal(t, tt.wantRegion, region) + }) + }Remove the now-unused strings import.
internal/exec/oci_aws.go (3)
58-61: Include registry context when no auth data is returned.Improve diagnosability by wrapping with context.
Apply this diff:
- if len(authTokenOutput.AuthorizationData) == 0 { - return nil, errNoECRAuthorizationData - } + if len(authTokenOutput.AuthorizationData) == 0 { + return nil, fmt.Errorf("%w for %s", errNoECRAuthorizationData, ecrClient.EndpointResolver.(*ecr.EndpointResolverV2).PartitionID()) + }Alternatively, if the client/partition is not easily available here, use the input registry string instead:
- return nil, errNoECRAuthorizationData + return nil, fmt.Errorf("%w for %s", errNoECRAuthorizationData, "<registry>")
31-47: Compile the ECR regex once to avoid per-call recompile.Minor perf/readability tweak.
Apply this diff:
var ( @@ errFailedToLoadAWSConfig = errors.New("failed to load AWS config") ) +// Precompiled: supports ecr and ecr-fips across partitions (incl. .cn). +var ecrRegistryRe = regexp.MustCompile(`^(?P<acct>\d{12})\.dkr\.(?P<svc>ecr(?:-fips)?)\.(?P<region>[a-z0-9-]+)\.amazonaws\.com(?:\.cn)?$`) + // parseECRRegistry parses ECR registry string and extracts account ID and region. func parseECRRegistry(registry string) (accountID, region string, err error) { - re := regexp.MustCompile(`^(?P<acct>\d{12})\.dkr\.(?P<svc>ecr(?:-fips)?)\.(?P<region>[a-z0-9-]+)\.amazonaws\.com(?:\.cn)?$`) - m := re.FindStringSubmatch(registry) + m := ecrRegistryRe.FindStringSubmatch(registry) if m == nil { return "", "", fmt.Errorf("%w: %s", errInvalidECRRegistryFormat, registry) } - accountID = m[re.SubexpIndex("acct")] - region = m[re.SubexpIndex("region")] + accountID = m[ecrRegistryRe.SubexpIndex("acct")] + region = m[ecrRegistryRe.SubexpIndex("region")]
86-103: Optional: add public ECR (public.ecr.aws) path later.If needed, wire ecr-public API and a separate regex.
internal/exec/oci_docker.go (1)
223-226: Harden helper name validation with an allowlist.Denylisting special chars still permits odd cases. Prefer a tight allowlist like ^[A-Za-z0-9_-]+$ for credsStore.
- // Validate credsStore to prevent command injection - if strings.ContainsAny(credsStore, ";&|`$(){}[]<>/\\") { + // Validate credsStore using an allowlist (letters, digits, underscore, hyphen). + if !regexp.MustCompile(`^[A-Za-z0-9_-]+$`).MatchString(credsStore) { return nil, fmt.Errorf("%w: %s", errInvalidCredentialStoreName, credsStore) }internal/exec/oci_utils.go (4)
178-181: Support ATMOS_ variants for registry env creds.Per our env guidelines, bind ATMOS_USERNAME/PASSWORD in addition to *.
- bindEnv(v, usernameKey, fmt.Sprintf("%s_USERNAME", registryEnvName)) - bindEnv(v, passwordKey, fmt.Sprintf("%s_PASSWORD", registryEnvName)) + bindEnv(v, usernameKey, + fmt.Sprintf("%s_USERNAME", registryEnvName), + fmt.Sprintf("ATMOS_%s_USERNAME", registryEnvName), + ) + bindEnv(v, passwordKey, + fmt.Sprintf("%s_PASSWORD", registryEnvName), + fmt.Sprintf("ATMOS_%s_PASSWORD", registryEnvName), + )
235-241: Punctuate list comments per guideline.Ensure each bullet ends with a period.
// It checks multiple sources in order of preference: -// 1. GitHub Container Registry (ghcr.io) with GITHUB_TOKEN -// 2. Docker credential helpers (from ~/.docker/config.json) -// 3. Environment variables for specific registries -// 4. AWS ECR authentication (if AWS credentials are available) +// 1. GitHub Container Registry (ghcr.io) with GITHUB_TOKEN. +// 2. Docker credential helpers (from ~/.docker/config.json). +// 3. Environment variables for specific registries. +// 4. AWS ECR authentication (if AWS credentials are available).
357-358: Polish doc comments (periods and naming).
- End sentences with periods.
- Start unexported func comment with its name for godoc clarity.
-// extractRawData attempts to extract raw data from the layer as a fallback +// extractRawData attempts to extract raw data from the layer as a fallback. @@ -// extractZipFile extracts a ZIP file from an io.Reader into the destination directory +// extractZipFile extracts a ZIP file from an io.Reader into the destination directory. @@ -// ParseOCIManifest reads and decodes an OCI manifest from a JSON file. +// parseOCIManifest reads and decodes an OCI manifest from a JSON file.Also applies to: 490-491, 560-564
563-565: Wrap manifest parse error with context.Preserve error chain consistently.
- if err := json.NewDecoder(manifestBytes).Decode(&manifest); err != nil { - return nil, err - } + if err := json.NewDecoder(manifestBytes).Decode(&manifest); err != nil { + return nil, fmt.Errorf("parse OCI manifest: %w", err) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
internal/exec/oci_auth_test.go(1 hunks)internal/exec/oci_aws.go(1 hunks)internal/exec/oci_aws_test.go(1 hunks)internal/exec/oci_azure.go(1 hunks)internal/exec/oci_azure_test.go(1 hunks)internal/exec/oci_docker.go(1 hunks)internal/exec/oci_docker_test.go(1 hunks)internal/exec/oci_extraction_test.go(1 hunks)internal/exec/oci_github.go(1 hunks)internal/exec/oci_github_test.go(1 hunks)internal/exec/oci_google.go(1 hunks)internal/exec/oci_google_test.go(1 hunks)internal/exec/oci_processing_test.go(1 hunks)internal/exec/oci_utils.go(4 hunks)internal/exec/oci_utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/exec/oci_github.go
- internal/exec/oci_utils_test.go
- internal/exec/oci_processing_test.go
- internal/exec/oci_google_test.go
- internal/exec/oci_docker_test.go
- internal/exec/oci_azure.go
- internal/exec/oci_extraction_test.go
- internal/exec/oci_auth_test.go
- internal/exec/oci_github_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All comments must end with periods (complete sentences).
Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Use logging only for system/debug info; structured logging without string interpolation; follow logging hierarchy per docs/logging.md.
Most text UI must go to stderr; only data/results to stdout for piping compatibility.
Prefer SDKs over shelling out to binaries; use filepath.Join, os.PathSeparator/ListSeparator; gate OS-specific logic with runtime.GOOS when unavoidable.
For non-standard execution paths, capture telemetry via pkg/telemetry (CaptureCmd or CaptureCmdString) without collecting user data.
Files:
internal/exec/oci_aws_test.gointernal/exec/oci_azure_test.gointernal/exec/oci_docker.gointernal/exec/oci_aws.gointernal/exec/oci_google.gointernal/exec/oci_utils.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
**/*_test.go: Use table-driven unit tests for pure functions.
Co-locate tests alongside implementation files (use *_test.go).
Files:
internal/exec/oci_aws_test.gointernal/exec/oci_azure_test.go
🧠 Learnings (13)
📚 Learning: 2025-09-02T14:44:18.360Z
Learnt from: CR
PR: cloudposse/atmos#0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T14:44:18.360Z
Learning: Always run `go test ./...` after changes; if it fails due to network restrictions, note that in the PR description
Applied to files:
internal/exec/oci_azure_test.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to pkg/utils/file_utils.go : Reuse file operation helpers from pkg/utils/file_utils.go instead of duplicating.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-27T16:59:26.187Z
Learnt from: osterman
PR: cloudposse/atmos#729
File: internal/exec/help.go:48-51
Timestamp: 2024-10-27T16:59:26.187Z
Learning: In the Atmos CLI help messages, when providing examples that include the version number, use the actual version variable (e.g., `version.Version`) instead of placeholders like `<version>`.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
internal/exec/oci_utils.go
🧬 Code graph analysis (3)
internal/exec/oci_azure_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(25-60)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
internal/exec/oci_utils.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Context(382-397)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (9)
internal/exec/oci_azure_test.go (4)
21-24: LGTM: E2E guard in place.Skipping Azure networked tests by default is the right call.
47-52: LGTM: Sovereign cloud (.cn) covered.Good addition for Azure China domains.
71-80: All asserted error substrings are defined in internal/exec/oci_azure.go. Tests remain stable.
25-67: No Germany sovereign-cloud test needed.
Azure uses the global*.azurecr.ioin German regions; there is no separateazurecr.desuffix.internal/exec/oci_google.go (1)
20-33: No action needed for missing Google credentials. getGCRAuth errors (including errNoGoogleCloudCredentialsFound) propagate through getRegistryAuth and are handled by the caller’s anonymous fallback.internal/exec/oci_aws.go (1)
83-123: Solid ECR auth flow.Good parsing, timeout, error wrapping, and Basic auth construction.
internal/exec/oci_docker.go (1)
75-95: Robust Docker config loader.Good existence check, error wrapping, and JSON parsing. LGTM.
internal/exec/oci_utils.go (2)
147-158: GHCR path looks good.Delegating to getGitHubAuth keeps concerns separated. Nice.
53-59: bindEnv helper aligns with project standards.Good use of viper.BindEnv with fallback and structured debug.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/exec/oci_docker.go (1)
54-73: Honor precedence: env > config for Docker config path.Make env override config, then fall back to default.
- // Resolve Docker config path - configDir := atmosConfig.Settings.OCI.DockerConfig - if configDir == "" { - configDir = v.GetString("docker_config") - } + // Resolve Docker config path (env has precedence). + configDir := v.GetString("docker_config") + if configDir == "" { + configDir = atmosConfig.Settings.OCI.DockerConfig + }
🧹 Nitpick comments (5)
internal/exec/oci_azure.go (3)
51-60: Harden ACR name extraction (case, :port, more suffixes).Handle mixed case and optional port; consider azurecr.de if needed.
-func extractACRName(registry string) (string, error) { - // Expected formats: <acr-name>.azurecr.{io|cn|us} - for _, suf := range []string{".azurecr.io", ".azurecr.us", ".azurecr.cn"} { - if strings.HasSuffix(registry, suf) { - return strings.TrimSuffix(registry, suf), nil - } - } - return "", fmt.Errorf("%w: %s (expected <name>.azurecr.{io|us|cn})", errInvalidACRRegistryFormat, registry) -} +func extractACRName(registry string) (string, error) { + host := strings.ToLower(registry) + if i := strings.IndexByte(host, ':'); i > -1 { + host = host[:i] // strip :port if present. + } + for _, suf := range []string{".azurecr.io", ".azurecr.us", ".azurecr.cn"} { // add ".azurecr.de" if required. + if strings.HasSuffix(host, suf) { + name := strings.TrimSuffix(host, suf) + if name != "" { + return name, nil + } + } + } + return "", fmt.Errorf("%w: %s (expected <name>.azurecr.{io|us|cn})", errInvalidACRRegistryFormat, registry) +}
211-216: Add timeouts to Azure token requests.Avoid indefinite waits from Default/ClientSecret credential flows.
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()Also applies to: 241-245
24-49: Minor: end comments with periods.Several comments lack trailing periods (e.g., Lines 24, 62, 65, 70, 75, 97, 116, 121, 130). Please tidy per guidelines.
Also applies to: 51-261
internal/exec/oci_docker.go (2)
152-177: Optional: add Docker Hub aliases to direct auth search.Many configs store auth under https://index.docker.io/v1/ or registry-1.docker.io.
- registryVariants := []string{ + registryVariants := []string{ registry, "https://" + registry, "http://" + registry, + // Docker Hub common keys. + "https://index.docker.io/v1/", + "https://registry-1.docker.io", }
23-52: Minor: end comments with periods.A few comments miss trailing periods (e.g., Lines 54, 76, 97, 113, 137, 152, 179, 214, 267). Please tidy.
Also applies to: 54-73, 75-283
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
internal/exec/oci_azure.go(1 hunks)internal/exec/oci_docker.go(1 hunks)internal/exec/oci_docker_test.go(1 hunks)internal/exec/oci_github.go(1 hunks)internal/exec/oci_github_test.go(1 hunks)pkg/config/load.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/exec/oci_docker_test.go
- internal/exec/oci_github.go
- internal/exec/oci_github_test.go
- pkg/config/load.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All comments must end with periods (complete sentences).
Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Use logging only for system/debug info; structured logging without string interpolation; follow logging hierarchy per docs/logging.md.
Most text UI must go to stderr; only data/results to stdout for piping compatibility.
Prefer SDKs over shelling out to binaries; use filepath.Join, os.PathSeparator/ListSeparator; gate OS-specific logic with runtime.GOOS when unavoidable.
For non-standard execution paths, capture telemetry via pkg/telemetry (CaptureCmd or CaptureCmdString) without collecting user data.
Files:
internal/exec/oci_azure.gointernal/exec/oci_docker.go
🧠 Learnings (7)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Applied to files:
internal/exec/oci_docker.go
🧬 Code graph analysis (1)
internal/exec/oci_azure.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
⏰ 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). (1)
- GitHub Check: Summary
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/exec/oci_utils.go (2)
498-508: Fix ZIP size-limit check to avoid false positives.Read up to maxZipSize+1 and reject only when exceeded.
- limitedReader := io.LimitReader(reader, maxZipSize) + limitedReader := io.LimitReader(reader, int64(maxZipSize)+1) zipData, err := io.ReadAll(limitedReader) if err != nil { return fmt.Errorf("%w: %w", errFailedToReadZipData, err) } - // Check if we hit the size limit (indicates potential decompression bomb) - if len(zipData) == maxZipSize { - return fmt.Errorf("%w: %d bytes", errZipSizeExceeded, maxZipSize) - } + // Reject if ZIP exceeds configured max size. + if len(zipData) > maxZipSize { + return fmt.Errorf("%w: %d bytes", errZipSizeExceeded, maxZipSize) + }
129-136: Only fall back to anonymous on “no auth found”.Current logic hides real auth errors (e.g., unreadable docker config). Gate anonymous fallback on a sentinel error.
- auth, err := getRegistryAuth(registry, atmosConfig) - if err != nil { - log.Debug("No authentication found, using anonymous", logFieldRegistry, registry) - opts = append(opts, remote.WithAuth(authn.Anonymous)) - } else { + auth, err := getRegistryAuth(registry, atmosConfig) + if err != nil { + if errors.Is(err, errNoAuthenticationFound) { + log.Debug("No authentication found, using anonymous.", logFieldRegistry, registry) + opts = append(opts, remote.WithAuth(authn.Anonymous)) + } else { + log.Error("Registry auth error.", logFieldRegistry, registry, logFieldError, err) + return nil, fmt.Errorf("resolve registry auth: %w", err) + } + } else { opts = append(opts, remote.WithAuth(auth)) log.Debug("Using authentication for registry", logFieldRegistry, registry) }
🧹 Nitpick comments (1)
internal/exec/oci_utils.go (1)
396-399: Avoid over-restrictive “..” filename check.strings.Contains(fileName, "..") blocks legitimate names. You already enforce path containment in resolveZipFilePath; rely on that instead.
- // Check for path traversal patterns - if strings.Contains(fileName, "..") { - return fmt.Errorf("%w: path traversal not allowed: %s", errIllegalZipFilePath, fileName) - } + // Path traversal is prevented by resolveZipFilePath; no generic ".." substring check here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
internal/exec/oci_aws.go(1 hunks)internal/exec/oci_aws_test.go(1 hunks)internal/exec/oci_azure_test.go(1 hunks)internal/exec/oci_docker.go(1 hunks)internal/exec/oci_google.go(1 hunks)internal/exec/oci_utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/exec/oci_azure_test.go
- internal/exec/oci_aws_test.go
- internal/exec/oci_google.go
- internal/exec/oci_docker.go
- internal/exec/oci_aws.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All comments must end with periods (complete sentences).
Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Use logging only for system/debug info; structured logging without string interpolation; follow logging hierarchy per docs/logging.md.
Most text UI must go to stderr; only data/results to stdout for piping compatibility.
Prefer SDKs over shelling out to binaries; use filepath.Join, os.PathSeparator/ListSeparator; gate OS-specific logic with runtime.GOOS when unavoidable.
For non-standard execution paths, capture telemetry via pkg/telemetry (CaptureCmd or CaptureCmdString) without collecting user data.
Files:
internal/exec/oci_utils.go
🧠 Learnings (12)
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to pkg/utils/file_utils.go : Reuse file operation helpers from pkg/utils/file_utils.go instead of duplicating.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-27T16:59:26.187Z
Learnt from: osterman
PR: cloudposse/atmos#729
File: internal/exec/help.go:48-51
Timestamp: 2024-10-27T16:59:26.187Z
Learning: In the Atmos CLI help messages, when providing examples that include the version number, use the actual version variable (e.g., `version.Version`) instead of placeholders like `<version>`.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
internal/exec/oci_utils.go
🧬 Code graph analysis (1)
internal/exec/oci_utils.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(25-60)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/oci_utils.go (1)
53-58: Nice: Viper binding with ATMOS_ fallbacks.bindEnv wrapper and registry-specific env handling with ATMOS_ alternatives align with project guidelines.
Also applies to: 171-200
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/exec/oci_docker.go (2)
103-110: Verify logFieldRegistry constant is defined.Previous review flagged it; ensure a package-level const exists (e.g., in oci_utils.go).
Also applies to: 143-149, 189-189
68-69: Fix multiple %w usages (vet/golangci-lint).Use a single %w per fmt.Errorf.
- return "", fmt.Errorf("%w: %w", errFailedToGetUserHomeDir, err) + return "", fmt.Errorf("%w: %v", errFailedToGetUserHomeDir, err) - return DockerConfig{}, fmt.Errorf("%w: %w", errFailedToReadDockerConfigFile, err) + return DockerConfig{}, fmt.Errorf("%w: %v", errFailedToReadDockerConfigFile, err) - return DockerConfig{}, fmt.Errorf("%w: %w", errFailedToParseDockerConfigJSON, err) + return DockerConfig{}, fmt.Errorf("%w: %v", errFailedToParseDockerConfigJSON, err) - return nil, fmt.Errorf("%w %s: %w", errFailedToDecodeAuthForRegistry, reg, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) - return nil, fmt.Errorf("%w %s: %w", errCredentialHelperNotFound, helperCmd, err) + return nil, fmt.Errorf("%w %s: %v", errCredentialHelperNotFound, helperCmd, err) - return nil, fmt.Errorf("%w %s: %w", errFailedToGetCredentialsFromStore, credsStore, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToGetCredentialsFromStore, credsStore, err) - return nil, fmt.Errorf("%w: %w", errFailedToParseCredentialStoreOutput, err) + return nil, fmt.Errorf("%w: %v", errFailedToParseCredentialStoreOutput, err) - return "", "", fmt.Errorf("%w: %w", errFailedToDecodeBase64AuthString, err) + return "", "", fmt.Errorf("%w: %v", errFailedToDecodeBase64AuthString, err)Also applies to: 85-86, 91-92, 168-169, 235-236, 245-246, 255-256, 273-274
🧹 Nitpick comments (5)
internal/exec/oci_docker.go (5)
126-133: Also try http:// variant for credHelpers.Some configs store helper keys with http://. Low-risk to include.
// Try with https:// prefix httpsRegistry := "https://" + registry if helper, ok := credHelpers[httpsRegistry]; ok { if auth, err := tryCredentialHelper(registry, httpsRegistry, helper); err == nil { return auth, nil } } + + // Try with http:// prefix. + httpRegistry := "http://" + registry + if helper, ok := credHelpers[httpRegistry]; ok { + if auth, err := tryCredentialHelper(registry, httpRegistry, helper); err == nil { + return auth, nil + } + }
137-150: Increase hit rate: query credsStore with https/http variants.Global stores often index by full server URL. Try common variants before failing.
func tryGlobalCredentialStore(registry, credsStore string) (authn.Authenticator, error) { if credsStore == "" { return nil, errNoGlobalCredentialStore } - if auth, err := getCredentialStoreAuth(registry, credsStore); err == nil { - log.Debug("Using global credential store authentication", logFieldRegistry, registry, "store", credsStore) - return auth, nil - } else { - log.Debug("Global credential store authentication failed", logFieldRegistry, registry, "store", credsStore, "error", err) - return nil, err - } + candidates := []string{registry, "https://" + registry, "http://" + registry} + var lastErr error + for _, serverURL := range candidates { + if auth, err := getCredentialStoreAuth(serverURL, credsStore); err == nil { + log.Debug("Using global credential store authentication", logFieldRegistry, serverURL, "store", credsStore) + return auth, nil + } else { + lastErr = err + } + } + log.Debug("Global credential store authentication failed", logFieldRegistry, registry, "store", credsStore, "error", lastErr) + return nil, lastErr }
232-246: Configurable helper timeout via env.Let users tune slow helpers; default remains 5s. Honors precedence with Viper.
- ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + // Allow tuning via ATMOS_OCI_DOCKER_HELPER_TIMEOUT (e.g., "10s"), default 5s. + timeout := 5 * time.Second + v := viper.New() + bindEnv(v, "docker_helper_timeout", "ATMOS_OCI_DOCKER_HELPER_TIMEOUT") + if s := v.GetString("docker_helper_timeout"); s != "" { + if d, err := time.ParseDuration(s); err == nil && d > 0 && d <= 30*time.Second { + timeout = d + } + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - cmd := exec.CommandContext(ctx, helperCmd, "get") + cmd := exec.CommandContext(ctx, helperCmd, "get")
232-241: Improve testability by abstracting exec calls.Small indirection lets tests stub helpers without spawning processes.
+// For testability. +var ( + lookPathFn = exec.LookPath + commandContextFn = exec.CommandContext +) ... - if _, err := exec.LookPath(helperCmd); err != nil { + if _, err := lookPathFn(helperCmd); err != nil { return nil, fmt.Errorf("%w %s: %v", errCredentialHelperNotFound, helperCmd, err) } ... - cmd := exec.CommandContext(ctx, helperCmd, "get") + cmd := commandContextFn(ctx, helperCmd, "get")
33-33: Nit: end comments with periods.Keeps docs consistent with project rules.
- // Static errors for Docker authentication + // Static errors for Docker authentication. - // Try exact registry match first + // Try exact registry match first. - // Try with https:// prefix + // Try with https:// prefix. - // For Docker Desktop on macOS, the credential store is typically "desktop" - // We need to use the docker-credential-desktop helper to get credentials + // For Docker Desktop on macOS, the credential store is typically "desktop". + // We need to use the docker-credential-desktop helper to get credentials.Also applies to: 119-119, 126-127, 229-231
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/exec/oci_docker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All comments must end with periods (complete sentences).
Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Use logging only for system/debug info; structured logging without string interpolation; follow logging hierarchy per docs/logging.md.
Most text UI must go to stderr; only data/results to stdout for piping compatibility.
Prefer SDKs over shelling out to binaries; use filepath.Join, os.PathSeparator/ListSeparator; gate OS-specific logic with runtime.GOOS when unavoidable.
For non-standard execution paths, capture telemetry via pkg/telemetry (CaptureCmd or CaptureCmdString) without collecting user data.
Files:
internal/exec/oci_docker.go
🧠 Learnings (11)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
🧬 Code graph analysis (1)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/oci_docker.go (3)
54-63: Confirm bindEnv availability.This file relies on bindEnv. If not defined in package exec, replace with explicit Viper bindings.
- v := viper.New() - bindEnv(v, "docker_config", "ATMOS_OCI_DOCKER_CONFIG", "DOCKER_CONFIG") + v := viper.New() + // If bindEnv is unavailable here, explicitly bind and read the preferred one. + // v.BindEnv("docker_config", "ATMOS_OCI_DOCKER_CONFIG") // preferred + // if v.GetString("docker_config") == "" { v.BindEnv("docker_config", "DOCKER_CONFIG") }
23-31: Struct shape and tags look good.DockerConfig mirrors config.json (auths/credsStore/credHelpers) cleanly.
180-213: Auth source ordering is sensible with clear fallbacks.Per-registry helpers → global store → direct auth is a solid priority. Final error is meaningful.
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/oci_docker.go (1)
167-167: Fix multiple %w in fmt.Errorf (vet failure).Use a single %w and format the inner error with %v.
- return nil, fmt.Errorf("%w %s: %w", errFailedToDecodeAuthForRegistry, reg, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) - return nil, fmt.Errorf("%w %s: %w", errCredentialHelperNotFound, helperCmd, err) + return nil, fmt.Errorf("%w %s: %v", errCredentialHelperNotFound, helperCmd, err) - return nil, fmt.Errorf("%w %s: %w", errFailedToGetCredentialsFromStore, credsStore, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToGetCredentialsFromStore, credsStore, err)Also applies to: 234-235, 244-245
🧹 Nitpick comments (7)
internal/exec/oci_aws.go (3)
96-105: Add missing periods to comments to satisfy godot.A few comments in this block don’t end with periods.
- // Create context with timeout to prevent hanging AWS API calls + // Create context with timeout to prevent hanging AWS API calls. - // Load AWS config for the target region + // Load AWS config for the target region.
85-88: Doc comments should end with periods.Minor style nits per golangci-lint godot.
-// getECRAuth attempts to get AWS ECR authentication using AWS credentials -// Supports SSO/role providers by not gating on environment variables +// getECRAuth attempts to get AWS ECR authentication using AWS credentials. +// Supports SSO/role providers by not gating on environment variables.
51-64: Optionally prefer matching ProxyEndpoint entry.ECR can return multiple AuthorizationData entries; choosing the one whose ProxyEndpoint matches the target registry improves robustness. Low-risk refactor.
-func getECRAuthToken(ctx context.Context, ecrClient *ecr.Client, accountID string) (*types.AuthorizationData, error) { +func getECRAuthToken(ctx context.Context, ecrClient *ecr.Client, accountID string, registry string) (*types.AuthorizationData, error) { @@ - if len(authTokenOutput.AuthorizationData) == 0 { + if len(authTokenOutput.AuthorizationData) == 0 { return nil, fmt.Errorf("%w for account %s", errNoECRAuthorizationData, accountID) } - return &authTokenOutput.AuthorizationData[0], nil + // Prefer the entry whose ProxyEndpoint matches the target registry. + for i := range authTokenOutput.AuthorizationData { + ad := &authTokenOutput.AuthorizationData[i] + if ad.ProxyEndpoint != nil && strings.Contains(*ad.ProxyEndpoint, registry) { + return ad, nil + } + } + return &authTokenOutput.AuthorizationData[0], nilAnd update caller:
- authData, err := getECRAuthToken(ctx, ecrClient, accountID) + authData, err := getECRAuthToken(ctx, ecrClient, accountID, registry)internal/exec/oci_docker.go (1)
216-221: Precompile regex to avoid recompilation overhead.Minor perf/readability tweak.
- // Validate registry using an allowlist approach - // Registry may only include letters, digits, dots, colons, slashes, and hyphens - validRegistry := regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) - if !validRegistry.MatchString(registry) { + // Validate registry using an allowlist approach. + if !validRegistryRe.MatchString(registry) { return nil, fmt.Errorf("%w: %s", errInvalidRegistryName, registry) }Add once near other vars:
var ( + validRegistryRe = regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) // Static errors for Docker authenticationinternal/exec/oci_utils.go (3)
63-68: Remove unused tempDir to avoid unnecessary I/O and imports.tempDir is created and removed but never used; drop it and the uuid import.
-import ( +import ( "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" - "github.com/google/uuid" ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) @@ - tempDir, err := os.MkdirTemp("", uuid.New().String()) - if err != nil { - return fmt.Errorf("failed to create temp directory: %w", err) - } - defer removeTempDir(tempDir) + // No temp workspace required for extraction; write directly to destDir.Also applies to: 21-21, 67-67
124-151: Optional: add a request timeout to remote.Get.Prevents hangs on slow registries; matches the time-bounding used elsewhere.
-import ( +import ( + "context" "archive/zip" @@ func pullImage(ref name.Reference, atmosConfig *schema.AtmosConfiguration) (*remote.Descriptor, error) { var opts []remote.Option @@ - descriptor, err := remote.Get(ref, opts...) + // Add a conservative timeout for registry interactions. + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + opts = append(opts, remote.WithContext(ctx)) + descriptor, err := remote.Get(ref, opts...)Also applies to: 3-3
41-51: Minor: ensure comment sentences end with periods.One-line comments like “Additional supported artifact types” should end with periods per godot.
- // Additional supported artifact types + // Additional supported artifact types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/exec/oci_aws.go(1 hunks)internal/exec/oci_azure.go(1 hunks)internal/exec/oci_docker.go(1 hunks)internal/exec/oci_google.go(1 hunks)internal/exec/oci_utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/exec/oci_azure.go
- internal/exec/oci_google.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Distinguish UI output (stderr) from structured logging; never use logging for UI prompts/errors; data/results go to stdout for piping.
Use structured logging without message interpolation and respect logging level hierarchy; logging must not affect execution.
Most text UI must go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results go to stdout.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(cmd, err) or telemetry.CaptureCmdString("name", err); never capture user data.
Files:
internal/exec/oci_aws.gointernal/exec/oci_docker.gointernal/exec/oci_utils.go
🧠 Learnings (19)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.gointernal/exec/oci_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.gointernal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.gointernal/exec/oci_utils.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.gointernal/exec/oci_utils.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-27T16:59:26.187Z
Learnt from: osterman
PR: cloudposse/atmos#729
File: internal/exec/help.go:48-51
Timestamp: 2024-10-27T16:59:26.187Z
Learning: In the Atmos CLI help messages, when providing examples that include the version number, use the actual version variable (e.g., `version.Version`) instead of placeholders like `<version>`.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
internal/exec/oci_utils.go
🧬 Code graph analysis (2)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
internal/exec/oci_utils.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(25-60)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/oci_aws.go (1)
31-49: Regex-based ECR parsing is solid.Covers ecr/ecr-fips and .cn; clear error paths. Nice separation via parseECRRegistry.
internal/exec/oci_docker.go (1)
214-226: Allowlist validation and helper pre-checks look good.Good defense-in-depth on registry/store inputs and helper existence.
internal/exec/oci_utils.go (2)
129-141: Anonymous fallback gated correctly.Only falling back on errNoAuthenticationFound is the right behavior.
503-539: ZIP extraction hardening looks solid.Zip Slip guards, size cap, and symlink skipping are well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/exec/oci_docker.go (2)
105-109: Verify logFieldRegistry is defined.This identifier isn’t declared in this file. Ensure it’s defined once for the package (e.g., in oci_utils.go).
#!/bin/bash # Confirm logFieldRegistry exists in this package. rg -nP --type=go -C1 'const\s+logFieldRegistry\b|var\s+logFieldRegistry\b'Also applies to: 144-147
168-169: Fix multiple %w wrappers (vet/golangci-lint error).Only one %w is allowed per fmt.Errorf call.
- return nil, fmt.Errorf("%w %s: %w", errFailedToDecodeAuthForRegistry, reg, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) - return nil, fmt.Errorf("%w %s: %w", errCredentialHelperNotFound, helperCmd, err) + return nil, fmt.Errorf("%w %s: %v", errCredentialHelperNotFound, helperCmd, err) - return nil, fmt.Errorf("%w %s: %w", errFailedToGetCredentialsFromStore, credsStore, err) + return nil, fmt.Errorf("%w %s: %v", errFailedToGetCredentialsFromStore, credsStore, err)Also applies to: 235-236, 245-246
🧹 Nitpick comments (7)
internal/exec/oci_docker.go (7)
55-55: Fix godot: end comments with periods.Lint will fail (godot). Apply tidy punctuation.
-// Create a Viper instance for environment variable access +// Create a Viper instance for environment variable access. -// Resolve Docker config path (env has precedence). +// Resolve Docker config path (env has precedence). -// Check if Docker config file exists +// Check if Docker config file exists. -// Read Docker config file +// Read Docker config file. -// Parse Docker config JSON +// Parse Docker config JSON. -// For Docker Desktop on macOS, the credential store is typically "desktop" -// We need to use the docker-credential-desktop helper to get credentials +// For Docker Desktop on macOS, the credential store is typically "desktop". +// We need to use the docker-credential-desktop helper to get credentials.Also applies to: 59-59, 76-76, 81-81, 87-87, 229-230
113-135: Try http:// helper key variant as well.Some configs register helpers under http://server.
// Try with https:// prefix httpsRegistry := "https://" + registry if helper, ok := credHelpers[httpsRegistry]; ok { if auth, err := tryCredentialHelper(registry, httpsRegistry, helper); err == nil { return auth, nil } } + // Try with http:// prefix. + httpRegistry := "http://" + registry + if helper, ok := credHelpers[httpRegistry]; ok { + if auth, err := tryCredentialHelper(registry, httpRegistry, helper); err == nil { + return auth, nil + } + }
152-178: Support Docker Hub-style /v1/ auth keys.Docker often stores auths under https:///v1/. Add those variants to improve hit rate.
// Try different registry formats registryVariants := []string{ registry, "https://" + registry, "http://" + registry, + "https://" + registry + "/v1/", + "http://" + registry + "/v1/", }
238-244: Send newline to credential helper stdin.Helpers read until newline; sending one avoids edge cases with buffered reads.
- cmd := exec.CommandContext(ctx, helperCmd, "get") - cmd.Stdin = strings.NewReader(registry) + cmd := exec.CommandContext(ctx, helperCmd, "get") + cmd.Stdin = strings.NewReader(registry + "\n")
238-241: Make helper timeout configurable.Expose timeout via env (e.g., ATMOS_OCI_CRED_HELPER_TIMEOUT seconds) bound with viper; default to 5s.
Happy to draft the wiring if you want it in this PR.
232-246: Abstract exec for testability.Consider indirection for exec.LookPath/CommandContext so tests can stub the helper without forking processes.
+// Allow tests to override exec for credential helpers. +var lookPath = exec.LookPath +var commandContext = exec.CommandContext ... - if _, err := exec.LookPath(helperCmd); err != nil { + if _, err := lookPath(helperCmd); err != nil { return nil, fmt.Errorf("%w %s: %v", errCredentialHelperNotFound, helperCmd, err) } ... - cmd := exec.CommandContext(ctx, helperCmd, "get") + cmd := commandContext(ctx, helperCmd, "get")
157-176: Add a quick test for Docker Hub key variants.Please include tests that cover auth keys with /v1/ suffix to prevent regressions.
I can add table-driven cases to internal/exec/oci_docker_test.go for the added variants. Want me to push a patch?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/exec/oci_docker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Distinguish UI output (stderr) from structured logging; never use logging for UI prompts/errors; data/results go to stdout for piping.
Use structured logging without message interpolation and respect logging level hierarchy; logging must not affect execution.
Most text UI must go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results go to stdout.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(cmd, err) or telemetry.CaptureCmdString("name", err); never capture user data.
Files:
internal/exec/oci_docker.go
🧠 Learnings (10)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
🧬 Code graph analysis (1)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/oci_docker.go (3)
180-213: Nice fallback ordering and logging.Clear progression: per-registry helper → global store → direct auth, with structured debug logs. Looks good.
212-213: No action needed:errNoAuthenticationFoundis defined ininternal/exec/oci_utils.go.
Verified thaterrNoAuthenticationFoundis declared at line 38 inoci_utils.go, so its use inoci_docker.go(lines 212–213) is valid.
53-72: No action needed: no CLI flag for docker_config
Verification shows there’s no viper.BindPFlag or CLI flag defined for docker_config in the codebase, so using a fresh Viper here doesn’t drop any flag bindings. Pass in the shared Viper only if you later introduce a CLI flag for docker_config.
Signed-off-by: Chris Harden <[email protected]>
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/exec/oci_docker.go (6)
119-149: Nit: end comment sentences with periods (godot).Add periods to satisfy golangci-lint godot.
- // Try exact registry match first + // Try exact registry match first. @@ - // Try with https:// prefix + // Try with https:// prefix.
29-36: Optional: support identitytoken in Docker auths.Docker config may provide identitytoken instead of auth. Handle it to support token-based logins.
type DockerConfig struct { - Auths map[string]struct { - Auth string `json:"auth"` - } `json:"auths"` + Auths map[string]struct { + Auth string `json:"auth"` + IdentityToken string `json:"identitytoken"` + } `json:"auths"` CredsStore string `json:"credsStore"` CredHelpers map[string]string `json:"credHelpers"` }
166-194: Optional: return IdentityToken-based authenticator when present.Prefer identitytoken when available; fall back to basic auth decoding.
func tryDirectAuth(registry string, auths map[string]struct { Auth string `json:"auth"` }, ) (authn.Authenticator, error) { @@ for _, reg := range registryVariants { - if authData, exists := auths[reg]; exists && authData.Auth != "" { - username, password, err := decodeDockerAuth(authData.Auth) - if err != nil { - return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) - } - return &authn.Basic{ - Username: username, - Password: password, - }, nil - } + if authData, exists := auths[reg]; exists { + if authData.IdentityToken != "" { + return authn.FromConfig(authn.AuthConfig{IdentityToken: authData.IdentityToken}), nil + } + if authData.Auth != "" { + username, password, err := decodeDockerAuth(authData.Auth) + if err != nil { + return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) + } + return &authn.Basic{Username: username, Password: password}, nil + } + } }
231-244: Nit: precompile regexps at package level.Avoid recompiling on each call; minor hot-path polish.
+// Precompiled regex patterns for validation. +var ( + validRegistryRe = regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) + validStoreRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) +) @@ - validRegistry := regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) - if !validRegistry.MatchString(registry) { + if !validRegistryRe.MatchString(registry) { return nil, fmt.Errorf("%w: %s", errInvalidRegistryName, registry) } @@ - if !regexp.MustCompile(`^[A-Za-z0-9_-]+$`).MatchString(credsStore) { + if !validStoreRe.MatchString(credsStore) { return nil, fmt.Errorf("%w: %s", errInvalidCredentialStoreName, credsStore) }
248-252: Nit: end comment with a period (godot).- // Try to execute the credential helper + // Try to execute the credential helper.
286-299: Nit: end comment sentences with periods (godot).- // Decode base64 + // Decode base64. @@ - // Split username:password + // Split username:password.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/exec/oci_docker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Distinguish UI output (stderr) from structured logging; never use logging for UI prompts/errors; data/results go to stdout for piping.
Use structured logging without message interpolation and respect logging level hierarchy; logging must not affect execution.
Most text UI must go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results go to stdout.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(cmd, err) or telemetry.CaptureCmdString("name", err); never capture user data.
Files:
internal/exec/oci_docker.go
🧠 Learnings (10)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/oci_docker.go (4)
59-78: LGTM: Env-first Docker config resolution.Good use of bindEnv and precedence (env > config > default).
80-101: LGTM: Robust config load with clear sentinels.Clear error taxonomy and wrapping.
102-117: LGTM: Per-registry helper uses the exact server URL.Passing helperKey avoids mismatches.
111-112: logFieldRegistry is defined once at internal/exec/oci_utils.go:46 and used consistently across the package.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Chris Harden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/exec/oci_docker.go (3)
102-117: Per‑registry helper: correct server URL usage.Passing helperKey (exact config key) fixes the common mismatch bug. Nice.
151-177: Global credsStore variants: solid hardening.Trying https/http and /v1/ variants addresses Docker Hub quirks. Looks good.
221-225: Don’t fail fast when Docker config is missing; allow auth fallback.If config.json is absent, return a typed “no auth here” so upstream can try other sources.
// Load Docker config dockerConfig, err := loadDockerConfig(configPath) if err != nil { - return nil, err + if errors.Is(err, errDockerConfigFileNotFound) { + log.Debug("Docker config file not found; skipping Docker auth", "path", configPath) + return nil, fmt.Errorf("%w %s", errNoAuthenticationFound, registry) + } + return nil, err }
🧹 Nitpick comments (4)
internal/exec/oci_docker.go (4)
244-257: Avoid recompiling regex each call.Hoist regexes to package scope to reduce allocations on hot paths.
@@ var ( // Static errors for Docker authentication @@ ) +// Precompiled validators to avoid per-call allocations. +var ( + validRegistryRE = regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) + credStoreRE = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) +) @@ - // Validate registry using an allowlist approach - // Registry may only include letters, digits, dots, colons, slashes, and hyphens - validRegistry := regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) - if !validRegistry.MatchString(registry) { + // Validate registry using an allowlist approach. + if !validRegistryRE.MatchString(registry) { return nil, fmt.Errorf("%w: %s", errInvalidRegistryName, registry) } - // Validate credsStore using an allowlist (letters, digits, underscore, hyphen). - if !regexp.MustCompile(`^[A-Za-z0-9_-]+$`).MatchString(credsStore) { + // Validate credsStore using an allowlist (letters, digits, underscore, hyphen). + if !credStoreRE.MatchString(credsStore) { return nil, fmt.Errorf("%w: %s", errInvalidCredentialStoreName, credsStore) }
29-36: Support username/password and identitytoken entries in auths.Some Docker configs include username/password or identitytoken instead of “auth”. Add handling to broaden compatibility.
@@ type DockerConfig struct { - Auths map[string]struct { - Auth string `json:"auth"` - } `json:"auths"` + Auths map[string]struct { + Auth string `json:"auth"` + Username string `json:"username"` + Password string `json:"password"` + IdentityToken string `json:"identitytoken"` + } `json:"auths"` @@ func tryDirectAuth(registry string, auths map[string]struct { - Auth string `json:"auth"` + Auth string `json:"auth"` + Username string `json:"username"` + Password string `json:"password"` + IdentityToken string `json:"identitytoken"` }, ) (authn.Authenticator, error) { @@ for _, reg := range registryVariants { if authData, exists := auths[reg]; exists && authData.Auth != "" { username, password, err := decodeDockerAuth(authData.Auth) if err != nil { return nil, fmt.Errorf("%w %s: %v", errFailedToDecodeAuthForRegistry, reg, err) } - return &authn.Basic{ - Username: username, - Password: password, - }, nil + return &authn.Basic{Username: username, Password: password}, nil + } + if exists && auths[reg].Username != "" && auths[reg].Password != "" { + return &authn.Basic{Username: auths[reg].Username, Password: auths[reg].Password}, nil + } + if exists && auths[reg].IdentityToken != "" { + return authn.FromConfig(authn.AuthConfig{IdentityToken: auths[reg].IdentityToken}), nil } }Also applies to: 179-207
125-146: Fix godot: end comments with periods.A few comments are missing trailing periods; golangci-lint will flag them.
- // Try exact registry match first + // Try exact registry match first. @@ - // Try with https:// prefix + // Try with https:// prefix. @@ - // Try with http:// prefix. + // Try with http:// prefix. @@ - // Try different registry formats + // Try different registry formats. @@ - // Try to execute the credential helper + // Try to execute the credential helper. @@ - // Parse the JSON output from the credential helper + // Parse the JSON output from the credential helper. @@ - // Split username:password + // Split username:password.Also applies to: 185-191, 261-269, 277-283, 305-311
298-303: Trim whitespace before base64 decode.Defensive trim avoids failures from accidental spaces/newlines.
- decoded, err := base64.StdEncoding.DecodeString(authString) + decoded, err := base64.StdEncoding.DecodeString(strings.TrimSpace(authString))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/exec/oci_docker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Distinguish UI output (stderr) from structured logging; never use logging for UI prompts/errors; data/results go to stdout for piping.
Use structured logging without message interpolation and respect logging level hierarchy; logging must not affect execution.
Most text UI must go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results go to stdout.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(cmd, err) or telemetry.CaptureCmdString("name", err); never capture user data.
Files:
internal/exec/oci_docker.go
🧠 Learnings (10)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
🧬 Code graph analysis (1)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/oci_docker.go (2)
59-78: Env precedence honored.Good use of viper + bindEnv and fallback chain (env → config → default). No action needed.
109-116: The scripts will search for the missing constant and verify the helper’s error returns. Once we have the output, I’ll update the comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/oci_docker.go (1)
111-111: Verify logFieldRegistry is defined package-wide.This was flagged earlier; ensure it exists to avoid an undefined identifier.
🧹 Nitpick comments (6)
internal/exec/oci_docker.go (6)
39-39: Fix godot: end comment with a period.This avoids golangci-lint failures.
- // Static errors for Docker authentication + // Static errors for Docker authentication.
80-101: Include config path in read/parse error context.Improves debuggability without leaking secrets.
- configData, err := os.ReadFile(configPath) + configData, err := os.ReadFile(configPath) if err != nil { - return DockerConfig{}, fmt.Errorf("%w: %v", errFailedToReadDockerConfigFile, err) + return DockerConfig{}, fmt.Errorf("%w %s: %v", errFailedToReadDockerConfigFile, configPath, err) } @@ - if err := json.Unmarshal(configData, &dockerConfig); err != nil { - return DockerConfig{}, fmt.Errorf("%w: %v", errFailedToParseDockerConfigJSON, err) + if err := json.Unmarshal(configData, &dockerConfig); err != nil { + return DockerConfig{}, fmt.Errorf("%w %s: %v", errFailedToParseDockerConfigJSON, configPath, err) }
102-117: Remove unused parameter from tryCredentialHelper.registry isn’t used; unparam may flag this. Simplify signature and call sites.
-func tryCredentialHelper(registry, helperKey, helper string) (authn.Authenticator, error) { +func tryCredentialHelper(helperKey, helper string) (authn.Authenticator, error) { @@ - if helper, ok := credHelpers[registry]; ok { - if auth, err := tryCredentialHelper(registry, registry, helper); err == nil { + if helper, ok := credHelpers[registry]; ok { + if auth, err := tryCredentialHelper(registry, helper); err == nil { return auth, nil } } @@ - if helper, ok := credHelpers[httpsRegistry]; ok { - if auth, err := tryCredentialHelper(registry, httpsRegistry, helper); err == nil { + if helper, ok := credHelpers[httpsRegistry]; ok { + if auth, err := tryCredentialHelper(httpsRegistry, helper); err == nil { return auth, nil } } @@ - if helper, ok := credHelpers[httpRegistry]; ok { - if auth, err := tryCredentialHelper(registry, httpRegistry, helper); err == nil { + if helper, ok := credHelpers[httpRegistry]; ok { + if auth, err := tryCredentialHelper(httpRegistry, helper); err == nil { return auth, nil } }Also applies to: 125-147
209-212: Fix doc comment punctuation.Comply with godot; make it a complete sentence.
-// getDockerAuth attempts to get Docker authentication for a registry -// Supports DOCKER_CONFIG environment variable, global credential stores (credsStore), +// getDockerAuth attempts to get Docker authentication for a registry. +// It supports DOCKER_CONFIG environment variable, global credential stores (credsStore), // and per-registry credential helpers (credHelpers).
248-260: Nit: precompile regex and tighten comment punctuation.Avoid recompiling on each call and satisfy godot.
- // Validate registry using an allowlist approach - // Registry may only include letters, digits, dots, colons, slashes, and hyphens - validRegistry := regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) + // Validate registry using an allowlist approach. + // Registry may only include letters, digits, dots, colons, slashes, and hyphens. + validRegistry := validRegistryREAdditional change outside this hunk:
+// Precompiled validation regexes. +var ( + validRegistryRE = regexp.MustCompile(`^[A-Za-z0-9./:-]+$`) + validCredsStoreName = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) +)- if !regexp.MustCompile(`^[A-Za-z0-9_-]+$`).MatchString(credsStore) { + if !validCredsStoreName.MatchString(credsStore) { return nil, fmt.Errorf("%w: %s", errInvalidCredentialStoreName, credsStore) }
276-279: Capture helper stderr for better diagnostics (kept at debug level).CombinedOutput preserves stderr; keep error wrapping unchanged, but log stderr when present.
- output, err := cmd.Output() + output, err := cmd.CombinedOutput() if err != nil { + if len(output) > 0 { + log.Debug("Credential helper stderr", "helper", helperCmd, "stderr", strings.TrimSpace(string(output))) + } return nil, fmt.Errorf("%w %s: %v", errFailedToGetCredentialsFromStore, credsStore, err) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/exec/oci_docker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Bind environment variables with viper.BindEnv() and provide an ATMOS_ alternative for every env var.
Distinguish UI output (stderr) from structured logging; never use logging for UI prompts/errors; data/results go to stdout for piping.
Use structured logging without message interpolation and respect logging level hierarchy; logging must not affect execution.
Most text UI must go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results go to stdout.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(cmd, err) or telemetry.CaptureCmdString("name", err); never capture user data.
Files:
internal/exec/oci_docker.go
🧠 Learnings (10)
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow the precedence order: flags > environment variables > config file > defaults
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-09-05T14:40:05.107Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T14:40:05.107Z
Learning: Applies to **/*.go : Always wrap errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users
Applied to files:
internal/exec/oci_docker.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages
Applied to files:
internal/exec/oci_docker.go
🧬 Code graph analysis (1)
internal/exec/oci_docker.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(846-850)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/oci_docker.go (4)
23-27: LGTM: testability hooks for exec are solid.Overridable lookPath/commandContext enable clean mocking in tests.
29-36: LGTM: DockerConfig schema mirrors config.json.Fields/JSON tags look correct for auths, credsStore, and credHelpers.
301-316: LGTM: auth decoding is correct.Covers base64 decode and username:password split defensively.
59-78: resolveDockerConfigPath verification complete
- bindEnv defined in internal/exec/oci_utils.go at lines 55–57
- Settings.OCI.DockerConfig present in pkg/schema/schema.go at line 279
- getRegistryAuth in internal/exec/oci_utils.go treats errNoAuthenticationFound as a soft-fail (logs Debug then continues)
- No Cobra-registered CLI flag for Docker config path detected
What
This PR enhances Atmos's OCI vendoring capabilities by implementing authentication support for private registries and robust layer extraction for multiple OCI artifact formats. The changes resolve the
401 Unauthorizederror when pulling from private registries and support both TAR and ZIP layer formats.Why
Previously, Atmos's OCI vendoring was limited to:
ghcr.io) authentication viaGITHUB_TOKENapplication/vnd.atmos.component.terraform.v1+tar+gzip)This resulted in:
401 Unauthorizederrors when pulling from private registriesReferences
Solution Overview
1. Multi-Source Authentication Framework
Implemented a comprehensive authentication system that attempts credentials from multiple sources in order of preference:
GITHUB_TOKENenvironment variable~/.docker/config.jsonand credential helpersREGISTRY_NAME_USERNAME/PASSWORD2. Enhanced Layer Extraction
Added support for multiple OCI layer formats:
.tarand.tar.gzlayersarchive/zipmedia types3. Extended Artifact Type Support
Added support for additional OCI artifact types:
application/vnd.atmos.component.terraform.v1+tar+gzip(original)application/vnd.opentofu.modulepkg(OpenTofu modules)application/vnd.terraform.module.v1+tar+gzip(Terraform modules)Files Changed
Core Implementation
internal/exec/oci_utils.go- Enhanced authentication and extraction logicTest Coverage
internal/exec/oci_utils_test.go- Comprehensive test suite (new file)Key Changes
Authentication Enhancements
New Functions Added:
getRegistryAuth()- Centralized authentication logicgetDockerAuth()- Docker config file integrationgetCredentialStoreAuth()- Docker credential helper supportdecodeDockerAuth()- Base64 auth string decodinggetECRAuth(),getACRAuth(),getGCRAuth()- Cloud provider placeholdersEnhanced
pullImage()Function:Layer Extraction Improvements
New Functions Added:
extractZipFile()- ZIP archive extractionextractRawData()- Raw data fallback extractionEnhanced
processLayer()Function:Artifact Type Support
Enhanced
checkArtifactType()Function:Test Coverage
Created comprehensive test suite covering:
Authentication Tests
Extraction Tests
Integration Tests
Test Results
Performance Benchmarks
Usage Examples
Environment Variable Authentication
Docker Credential Store
# Uses existing Docker credentials from ~/.docker/config.json docker login my-registry.comGitHub Container Registry
Security Considerations
Testing Instructions
Manual Testing
Private Registry Test:
Docker Credential Test:
OpenTofu Module Test:
# Vendor an OpenTofu module (ZIP format) atmos vendor pull oci://registry.defenseunicorns.com/navy-pbme/terraform-aws-uds-s3irsa:v1.0.0Automated Testing
Future Enhancements
The authentication framework is designed for easy extension:
Breaking Changes
None - This is a purely additive enhancement that maintains backward compatibility.
Checklist
Result
This PR transforms Atmos's OCI vendoring from a basic GitHub-only feature into a robust, enterprise-ready solution that supports:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores