feat: Atmos List Instances, Pro Upload #1254
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
+ Coverage 56.71% 56.98% +0.26%
==========================================
Files 280 284 +4
Lines 29708 30286 +578
==========================================
+ Hits 16850 17259 +409
- Misses 11045 11200 +155
- Partials 1813 1827 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into feat/atmos-list-upload
Member
Author
|
@coderabbitai resume |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/list/list_values_test.go (1)
154-165: “json format” also asserts prod without includeAbstract.Align with the fixture the same way as the table case.
Apply:
{ - name: "json format", - component: "vpc", - format: "json", + name: "json format", + component: "vpc", + includeAbstract: true, + format: "json",internal/exec/describe_affected.go (1)
320-327: Guard against nil head/base references (panic risk).
headHead.Hash()/baseHead.Hash()will panic if either ref is nil (e.g., shallow clones or unusual refs). Add a defensive check and clearer error.Apply:
- req := dtos.UploadAffectedStacksRequest{ - HeadSHA: headHead.Hash().String(), - BaseSHA: baseHead.Hash().String(), + if headHead == nil || baseHead == nil { + return fmt.Errorf("cannot upload affected stacks: missing git references (head=%v base=%v)", headHead, baseHead) + } + req := dtos.UploadAffectedStacksRequest{ + HeadSHA: headHead.Hash().String(), + BaseSHA: baseHead.Hash().String(), RepoURL: repoUrl, RepoName: gitURL.GetRepoName(), RepoOwner: gitURL.GetOwnerName(), RepoHost: gitURL.GetHostName(), Stacks: affected, }pkg/list/format/table.go (1)
255-271: Width estimation ignores vars, leading to mis-sized tables.
getMaxValueWidthonly checks top-level keys, but display reads fromvarswhen present. This can under/over-estimate width and trigger false ErrTableTooWide.Here’s an updated implementation to consider
varsconsistently:// Replace getMaxValueWidth with vars-aware logic. func getMaxValueWidth(stackData map[string]interface{}, valueKeys []string) int { maxWidth := 0 varsData, _ := stackData["vars"].(map[string]interface{}) for _, valueKey := range valueKeys { var val interface{} if varsData != nil { if v, ok := varsData[valueKey]; ok { val = v } else if v, ok := stackData[valueKey]; ok { val = v } } else if v, ok := stackData[valueKey]; ok { val = v } if val != nil { formattedValue := formatTableCellValue(val) if w := len(formattedValue); w > maxWidth { maxWidth = w } } } return limitWidth(maxWidth) }No other code changes are required;
calculateStackColumnWidthwill pick up the improved width.
🧹 Nitpick comments (35)
pkg/schema/vendor_component.go (1)
7-7: Use Go initialism style for exported fields (URI, APIVersion).Optional, but aligns with go vet/revive and common conventions.
- Uri string `yaml:"uri" json:"uri" mapstructure:"uri"` + URI string `yaml:"uri" json:"uri" mapstructure:"uri"`- Uri string `yaml:"uri" json:"uri" mapstructure:"uri"` + URI string `yaml:"uri" json:"uri" mapstructure:"uri"`- ApiVersion string `yaml:"apiVersion" json:"apiVersion" mapstructure:"apiVersion"` + APIVersion string `yaml:"apiVersion" json:"apiVersion" mapstructure:"apiVersion"`Note: Renaming exported fields is a breaking change; confirm internal/external usages before applying.
Also applies to: 15-15, 31-31
pkg/pro/api_client_instance_status.go (1)
22-28: Safer URL composition.Consider url.JoinPath to avoid double slashes from BaseURL/BaseAPIEndpoint and to simplify path assembly.
- targetURL := fmt.Sprintf("%s/%s/repos/%s/%s/instances/%s/%s", - c.BaseURL, c.BaseAPIEndpoint, - url.PathEscape(dto.RepoOwner), - url.PathEscape(dto.RepoName), - url.PathEscape(dto.Stack), - url.PathEscape(dto.Component)) + targetURL, err := url.JoinPath( + c.BaseURL, + c.BaseAPIEndpoint, + "repos", + url.PathEscape(dto.RepoOwner), + url.PathEscape(dto.RepoName), + "instances", + url.PathEscape(dto.Stack), + url.PathEscape(dto.Component), + ) + if err != nil { + return fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrFailedToCreateAuthRequest, err) + }pkg/schema/command.go (1)
5-15: Make fields omitempty and expose Aliases on Command.Prevents noisy serialization and surfaces aliases alongside other metadata.
Apply:
type Command struct { - Name string `yaml:"name" json:"name" mapstructure:"name"` - Description string `yaml:"description" json:"description" mapstructure:"description"` - Env []CommandEnv `yaml:"env" json:"env" mapstructure:"env"` - Arguments []CommandArgument `yaml:"arguments" json:"arguments" mapstructure:"arguments"` - Flags []CommandFlag `yaml:"flags" json:"flags" mapstructure:"flags"` - ComponentConfig CommandComponentConfig `yaml:"component_config" json:"component_config" mapstructure:"component_config"` - Steps []string `yaml:"steps" json:"steps" mapstructure:"steps"` - Commands []Command `yaml:"commands" json:"commands" mapstructure:"commands"` - Verbose bool `yaml:"verbose" json:"verbose" mapstructure:"verbose"` + Name string `yaml:"name,omitempty" json:"name,omitempty" mapstructure:"name"` + Description string `yaml:"description,omitempty" json:"description,omitempty" mapstructure:"description"` + Env []CommandEnv `yaml:"env,omitempty" json:"env,omitempty" mapstructure:"env"` + Arguments []CommandArgument `yaml:"arguments,omitempty" json:"arguments,omitempty" mapstructure:"arguments"` + Flags []CommandFlag `yaml:"flags,omitempty" json:"flags,omitempty" mapstructure:"flags"` + ComponentConfig CommandComponentConfig `yaml:"component_config,omitempty" json:"component_config,omitempty" mapstructure:"component_config"` + Steps []string `yaml:"steps,omitempty" json:"steps,omitempty" mapstructure:"steps"` + Commands []Command `yaml:"commands,omitempty" json:"commands,omitempty" mapstructure:"commands"` + Aliases CommandAliases `yaml:"aliases,omitempty" json:"aliases,omitempty" mapstructure:"aliases"` + Verbose bool `yaml:"verbose,omitempty" json:"verbose,omitempty" mapstructure:"verbose"` }pkg/schema/markdown.go (3)
3-4: Add doc comments to exported types (revive/godot).Keeps linters happy and improves godoc.
Apply:
-type MarkdownSettings struct { +// MarkdownSettings defines styles for all supported Markdown elements. +type MarkdownSettings struct {
34-35: Add doc comments to exported types (revive/godot).Apply:
-type MarkdownStyle struct { +// MarkdownStyle configures styling for a Markdown element. +type MarkdownStyle struct {
28-31: Use common initialism “HTML” in field names.Go convention prefers HTMLBlock/HTMLSpan; tags preserve wire format.
Apply:
- HtmlBlock MarkdownStyle `yaml:"html_block,omitempty" json:"html_block,omitempty" mapstructure:"html_block"` - HtmlSpan MarkdownStyle `yaml:"html_span,omitempty" json:"html_span,omitempty" mapstructure:"html_span"` + HTMLBlock MarkdownStyle `yaml:"html_block,omitempty" json:"html_block,omitempty" mapstructure:"html_block"` + HTMLSpan MarkdownStyle `yaml:"html_span,omitempty" json:"html_span,omitempty" mapstructure:"html_span"`Please confirm no external code depends on the exact exported field names; this is a new file, so now is the time to align naming before release.
pkg/schema/atlantis.go (1)
5-10: Add doc comments to exported types (revive/godot).Keeps lint clean and docs readable.
Apply:
-// Atlantis integration - -type Atlantis struct { +// Atlantis groups Atlantis integration settings. +type Atlantis struct {tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stdout.golden (1)
1-4: Consider including a Drift column in non-TTY output when drift is enabled.Matches user expectations and TTY/table parity.
Confirm whether TTY output already includes a drift/status column for this command; if yes, consider adding it here too for consistency.
tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled.stdout.golden (1)
1-7: Confirm stability of OSC sequences in TTY snapshot.The captured output includes OSC 10/11 queries. If this proves flaky across environments, consider disabling color/queries during tests or normalizing output.
What CI environments will run these tests? If multiple, verify they all produce identical sequences. If not, gating color with an env var in tests may help.
pkg/list/list_instances_pro_test.go (1)
11-53: Pro-enabled filtering test: LGTM; consider a few more edge casesNice focused coverage. Consider adding cases:
- pro.enabled=true with drift_detection missing (default behavior).
- Explicit nil/typed maps to exercise type assertions.
pkg/pro/api_client_instance_status_test.go (1)
31-33: Assert request method/headers to harden the testValidate request shape to prevent regressions (method, auth header, content-type). Example:
- mockRoundTripper.On("RoundTrip", mock.Anything).Return(mockResponse, nil) + mockRoundTripper.On("RoundTrip", mock.MatchedBy(func(req *http.Request) bool { + // Basic request shape checks + if req.Method != http.MethodPost { + return false + } + if req.Header.Get("Authorization") != "Bearer test-token" { + return false + } + if ct := req.Header.Get("Content-Type"); ct != "application/json" { + return false + } + return true + })).Return(mockResponse, nil)Also applies to: 56-58
pkg/pro/api_client_test.go (1)
27-45: Use t.Setenv to simplify env setup/teardownt.Setenv avoids manual save/restore and reduces test noise.
cmd/terraform_commands.go (1)
324-328: Update flag help to "Atmos Pro API" — parsing verifiedWording nit: change the help string; verification: parseUploadStatusFlag supports --upload-status, --upload-status=true/false and ExecuteTerraform reads/removes the flag from info.AdditionalArgsAndFlags (populated by ProcessCommandLineArgs), so the flag flows correctly despite DisableFlagParsing.
File: cmd/terraform_commands.go
- cmd.PersistentFlags().Bool(cfg.UploadStatusFlag, false, "If set atmos will upload the plan result to the pro API") + cmd.PersistentFlags().Bool(cfg.UploadStatusFlag, false, "If set atmos will upload the plan result to the Atmos Pro API")internal/exec/describe_affected.go (1)
330-330: Avoid logging full request payload at debug (noisy).Dumping
Stackscan be very large. Log a summary (counts, SHAs) instead.- log.Debug("Preparing upload affected stacks request", "req", req) + log.Debug("Preparing upload affected stacks request", + "head", req.HeadSHA, "base", req.BaseSHA, + "repo", fmt.Sprintf("%s/%s@%s", req.RepoOwner, req.RepoName, req.RepoHost), + "stacks_count", len(req.Stacks))pkg/list/format/table.go (2)
113-119: Prefer fallback to top-level when vars key missing.If a stack has a
varsmap but not the specific key, we currently emit empty. Fall back to top-level to keep tables useful.- if varsData, ok := stackData["vars"].(map[string]interface{}); ok { - if val, ok := varsData[valueKey]; ok { - value = formatTableCellValue(val) - } - } else if val, ok := stackData[valueKey]; ok { + if varsData, ok := stackData["vars"].(map[string]interface{}); ok { + if val, ok := varsData[valueKey]; ok { + value = formatTableCellValue(val) + } else if val, ok := stackData[valueKey]; ok { + value = formatTableCellValue(val) + } + } else if val, ok := stackData[valueKey]; ok { value = formatTableCellValue(val) }
54-61: Minor: remove redundant break.
breakexits the switch, but we already break the outer loop when valueKeys is populated. It’s harmless but noisy.- if varsData, ok := typedData["vars"].(map[string]interface{}); ok { + if varsData, ok := typedData["vars"].(map[string]interface{}); ok { for k := range varsData { valueKeys = append(valueKeys, k) } - break }cmd/list_instances.go (1)
34-35: Align flag naming and reuse a constant.To keep CLI flags consistent and avoid magic strings, extract a named constant for the upload flag (or reuse an existing one if present in list/flags). Also capitalize "Pro" in the help text.
Apply:
- // Add instance-specific flags - listInstancesCmd.Flags().Bool("upload", false, "Upload instances to pro API") + // Add instance-specific flags. + listInstancesCmd.Flags().Bool(UploadInstancesFlag, false, "Upload instances to Pro API")Add (outside selection, near the top of this file):
const UploadInstancesFlag = "upload"If a shared flag constant already exists in pkg/list/flags, prefer that instead.
pkg/pro/api_client_instances.go (2)
30-34: Nit: end comments with periods (godot).A few comments lack trailing periods; this trips golangci-lint godot.
- // Guard against nil HTTPClient by ensuring a default client with a sane timeout + // Guard against nil HTTPClient by ensuring a default client with a sane timeout. @@ - // Log safe metadata instead of full payload to prevent secret leakage + // Log safe metadata instead of full payload to prevent secret leakage. @@ - log.Debug("Uploading instances.", "endpoint", endpoint) + log.Debug("Uploading instances.", "endpoint", endpoint) @@ - log.Debug("Uploaded instances.", "endpoint", endpoint) + log.Debug("Uploaded instances.", "endpoint", endpoint)Also applies to: 41-44, 50-52, 61-64
28-29: Optional: harden endpoint construction.If BaseURL or BaseAPIEndpoint may contain leading/trailing slashes, consider normalizing to avoid accidental double slashes.
base := strings.TrimRight(c.BaseURL, "/") api := strings.TrimLeft(c.BaseAPIEndpoint, "/") endpoint := fmt.Sprintf("%s/%s/instances", base, api)pkg/pro/api_client_instances_test.go (1)
87-156: Error-path coverage looks good; add nil DTO test.Add a test to assert ErrNilRequestDTO when dto is nil.
Additional test (outside selection):
func TestUploadInstances_NilDTO(t *testing.T) { client := &AtmosProAPIClient{ BaseURL: "http://localhost", BaseAPIEndpoint: "api", APIToken: "test-token", HTTPClient: &http.Client{Timeout: 2 * time.Second}, } err := client.UploadInstances(nil) assert.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrNilRequestDTO) }pkg/pro/api_client_unlock_test.go (1)
20-36: Fix comments to satisfy godot (end with periods).A few comments miss trailing periods; add them to appease linters.
- // Verify request method and path + // Verify request method and path. @@ - // Verify request body + // Verify request body. @@ - // Mock response with body that will fail to read + // Mock response with body that will fail to read. @@ - // Use an invalid URL that would cause http.NewRequest to fail + // Use an invalid URL that would cause http.NewRequest to fail. @@ -// FailingReaderUnlock is a mock io.Reader that always returns an error for unlock tests +// FailingReaderUnlock is a mock io.Reader that always returns an error for unlock tests.Also applies to: 170-176, 190-198, 240-250
pkg/list/list_instances_integration_test.go (1)
11-21: Split placeholders into an integration-only file.To avoid skipped tests in CI while keeping placeholders, move these to a separate _integration build-tagged file, leaving unit tests here.
Proposed header for the new file:
//go:build integration package listI can draft the split if you want.
Also applies to: 22-31, 33-42
pkg/schema/instance.go (1)
3-13: Add omitempty to JSON tags to shrink payloads.Reduces noise and bandwidth when uploading instances.
type Instance struct { - Component string `yaml:"component" json:"component" mapstructure:"component"` - Stack string `yaml:"stack" json:"stack" mapstructure:"stack"` - ComponentType string `yaml:"component_type" json:"component_type" mapstructure:"component_type"` - Settings AtmosSectionMapType `yaml:"settings" json:"settings" mapstructure:"settings"` - Vars AtmosSectionMapType `yaml:"vars" json:"vars" mapstructure:"vars"` - Env AtmosSectionMapType `yaml:"env" json:"env" mapstructure:"env"` - Backend AtmosSectionMapType `yaml:"backend" json:"backend" mapstructure:"backend"` - Metadata AtmosSectionMapType `yaml:"metadata" json:"metadata" mapstructure:"metadata"` + Component string `yaml:"component" json:"component,omitempty" mapstructure:"component"` + Stack string `yaml:"stack" json:"stack,omitempty" mapstructure:"stack"` + ComponentType string `yaml:"component_type" json:"component_type,omitempty" mapstructure:"component_type"` + Settings AtmosSectionMapType `yaml:"settings" json:"settings,omitempty" mapstructure:"settings"` + Vars AtmosSectionMapType `yaml:"vars" json:"vars,omitempty" mapstructure:"vars"` + Env AtmosSectionMapType `yaml:"env" json:"env,omitempty" mapstructure:"env"` + Backend AtmosSectionMapType `yaml:"backend" json:"backend,omitempty" mapstructure:"backend"` + Metadata AtmosSectionMapType `yaml:"metadata" json:"metadata,omitempty" mapstructure:"metadata"` }pkg/pro/dtos/instances_test.go (1)
56-131: Add JSON round‑trip assertions to lock in API contracts.To prevent regressions in JSON tags and ensure payload shape matches Atmos Pro API, add marshal/unmarshal subtests for both DTOs.
Example additions:
t.Run("json marshal InstanceStatusUploadRequest", func(t *testing.T) { req := InstanceStatusUploadRequest{ AtmosProRunID: "run-123", GitSHA: "abc123", RepoURL: "https://example.com", RepoName: "repo", RepoOwner: "owner", RepoHost: "host", Component: "comp", Stack: "stack", HasDrift: true, } b, err := json.Marshal(req) require.NoError(t, err) assert.Contains(t, string(b), `"atmos_pro_run_id":"run-123"`) assert.Contains(t, string(b), `"git_sha":"abc123"`) assert.Contains(t, string(b), `"has_drift":true`) }) t.Run("json marshal InstancesUploadRequest", func(t *testing.T) { req := InstancesUploadRequest{ RepoURL: "https://example.com", RepoName: "repo", RepoOwner: "owner", RepoHost: "host", Instances: []schema.Instance{{Component: "c", Stack: "s"}}, } b, err := json.Marshal(req) require.NoError(t, err) assert.Contains(t, string(b), `"repo_url":"https://example.com"`) assert.Contains(t, string(b), `"instances"`) })pkg/pro/dtos/instances.go (2)
10-15: Consider omitempty on optional repo fields.Avoid sending empty strings when data is unavailable (saves bytes and avoids ambiguous “blank” values).
Confirm the Atmos Pro API tolerates omitted fields for these when unknown.
type InstancesUploadRequest struct { - RepoURL string `json:"repo_url"` + RepoURL string `json:"repo_url,omitempty"` RepoName string `json:"repo_name"` RepoOwner string `json:"repo_owner"` - RepoHost string `json:"repo_host"` + RepoHost string `json:"repo_host,omitempty"` Instances []schema.Instance `json:"instances"` }
17-28: Apply omitempty to optional status fields.Keep required identifiers as-is; elide unknowns. Especially useful for Git metadata when not running in a git-aware context.
Validate with the Pro API contract.
type InstanceStatusUploadRequest struct { - AtmosProRunID string `json:"atmos_pro_run_id"` - GitSHA string `json:"git_sha"` - RepoURL string `json:"repo_url"` + AtmosProRunID string `json:"atmos_pro_run_id,omitempty"` + GitSHA string `json:"git_sha,omitempty"` + RepoURL string `json:"repo_url,omitempty"` RepoName string `json:"repo_name"` RepoOwner string `json:"repo_owner"` - RepoHost string `json:"repo_host"` + RepoHost string `json:"repo_host,omitempty"` Component string `json:"component"` Stack string `json:"stack"` - HasDrift bool `json:"has_drift"` + HasDrift bool `json:"has_drift,omitempty"` }pkg/list/list_instances_cmd_test.go (1)
181-193: Guard type assertions to avoid panics on unexpected shapes.Use require checks before casting nested maps.
- // Verify that the template variables in settings are properly rendered - proSettings := instance.Settings["pro"].(map[string]any) - pullRequest := proSettings["pull_request"].(map[string]any) - merged := pullRequest["merged"].(map[string]any) - workflows := merged["workflows"].(map[string]any) - workflow := workflows["atmos-pro-terraform-apply.yaml"].(map[string]any) - inputs := workflow["inputs"].(map[string]any) + // Verify that the template variables in settings are properly rendered. + proSettings, ok := instance.Settings["pro"].(map[string]any) + require.True(t, ok, "settings.pro must be a map") + pullRequest, ok := proSettings["pull_request"].(map[string]any) + require.True(t, ok, "settings.pro.pull_request must be a map") + merged, ok := pullRequest["merged"].(map[string]any) + require.True(t, ok, "settings.pro.pull_request.merged must be a map") + workflows, ok := merged["workflows"].(map[string]any) + require.True(t, ok, "settings.pro.pull_request.merged.workflows must be a map") + workflow, ok := workflows["atmos-pro-terraform-apply.yaml"].(map[string]any) + require.True(t, ok, "workflow entry must be a map") + inputs, ok := workflow["inputs"].(map[string]any) + require.True(t, ok, "workflow.inputs must be a map")Add import:
import ( "errors" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require"pkg/list/list_instances_comprehensive_test.go (2)
54-83: Remove unnecessary stdout redirection in format tests.formatInstances returns a string and does not write to stdout; simplify to reduce noise.
- // Mock non-TTY environment by redirecting stdout. - originalStdout := os.Stdout - defer func() { os.Stdout = originalStdout }() - - r, w, err := os.Pipe() - require.NoError(t, err) - defer func() { - require.NoError(t, r.Close()) - }() - - os.Stdout = w - output := formatInstances(instances) - - err = w.Close() - require.NoError(t, err) - os.Stdout = originalStdout - - // Read the output from the pipe. - pipeOutput, err := io.ReadAll(r) - require.NoError(t, err) - csvOutput := string(pipeOutput) // Should return CSV format. expectedCSV := "Component,Stack\nvpc,stack1\napp,stack2\ndb,stack1\n" assert.Equal(t, expectedCSV, output) - // The function doesn't write to stdout, it only returns the formatted string - assert.Equal(t, "", csvOutput) + // The function doesn't write to stdout; it only returns the formatted string.
85-113: Same simplification for empty set case (and fix punctuation).- originalStdout := os.Stdout - defer func() { os.Stdout = originalStdout }() - - r, w, err := os.Pipe() - require.NoError(t, err) - defer func() { - require.NoError(t, r.Close()) - }() - - os.Stdout = w - output := formatInstances([]schema.Instance{}) - - err = w.Close() - require.NoError(t, err) - os.Stdout = originalStdout - - // Read the output from the pipe. - pipeOutput, err := io.ReadAll(r) - require.NoError(t, err) - csvOutput := string(pipeOutput) expectedCSV := "Component,Stack\n" assert.Equal(t, expectedCSV, output) - // The function doesn't write to stdout, it only returns the formatted string - assert.Equal(t, "", csvOutput) + // The function doesn't write to stdout; it only returns the formatted string.internal/exec/terraform_test.go (2)
590-593: Prefer t.Setenv for automatic cleanup.Keeps env scoped to this test and avoids leakage.
- err = os.Setenv("ATMOS_LOGS_LEVEL", "Debug") - assert.NoError(t, err, "Setting 'ATMOS_LOGS_LEVEL' environment variable should execute without error") + t.Setenv("ATMOS_LOGS_LEVEL", "Debug")
594-603: Remove unused testcase fields or assert them.checkDetailedExit and exitCode are never used; drop them (or assert on detailed exit code behavior).
Would you like me to wire assertions for detailed exit codes based on output, or should we prune these fields now?
internal/exec/terraform.go (2)
3-18: Add strconv import for robust flag parsing.
Needed by the boolean parsing fix below.import ( "errors" "fmt" "os" osexec "os/exec" "path/filepath" + "strconv" "strings" log "github.com/charmbracelet/log"
426-436: Avoid duplicate --detailed-exitcode by checking both sources.
If callers already added it (either via AdditionalArgsAndFlags or earlier in allArgsAndFlags), prevent duplication.- if uploadStatusFlag { - if !u.SliceContainsString(info.AdditionalArgsAndFlags, detailedExitCodeFlag) { - allArgsAndFlags = append(allArgsAndFlags, []string{detailedExitCodeFlag}...) - } - } + if uploadStatusFlag { + if !u.SliceContainsString(info.AdditionalArgsAndFlags, detailedExitCodeFlag) && + !u.SliceContainsString(allArgsAndFlags, detailedExitCodeFlag) { + allArgsAndFlags = append(allArgsAndFlags, []string{detailedExitCodeFlag}...) + } + }pkg/list/list_instances.go (1)
124-145: Support both “drift_detection” and “drift-detection” keys.
Config keys in YAML often use hyphens; accept both to avoid false negatives.func isProDriftDetectionEnabled(instance *schema.Instance) bool { proSettings, ok := instance.Settings["pro"].(map[string]any) if !ok { return false } @@ - driftDetection, ok := proSettings["drift_detection"].(map[string]any) - if !ok { - return false - } + var driftDetection map[string]any + if v, ok := proSettings["drift_detection"].(map[string]any); ok { + driftDetection = v + } else if v, ok := proSettings["drift-detection"].(map[string]any); ok { + driftDetection = v + } else { + return false + } enabled, ok := driftDetection["enabled"].(bool) return ok && enabled }internal/exec/pro.go (1)
267-275: Optimize Pro enablement check for performance.The current implementation performs type assertions and map lookups on every call to
shouldUploadStatus. Consider caching the Pro enablement state during configuration initialization to avoid repeated expensive operations.+// Add to ConfigAndStacksInfo struct in schema.go: +// ProEnabled bool `yaml:"pro_enabled" json:"pro_enabled" mapstructure:"pro_enabled"` func shouldUploadStatus(info *schema.ConfigAndStacksInfo) bool { // Only upload for plan command if info.SubCommand != "plan" { return false } - // Check if pro is enabled in component settings - if proSettings, ok := info.ComponentSettingsSection["pro"].(map[string]interface{}); ok { - if enabled, ok := proSettings["enabled"].(bool); ok && enabled { - return true - } - } + // Use cached Pro enablement state + if info.ProEnabled { + return true + } - // Log warning if pro is not enabled + // Log debug message if pro is not enabled - log.Warn("Pro is not enabled. Skipping upload of Terraform result.") + log.Debug("Pro is not enabled. Skipping upload of Terraform result.") return false }
Contributor
✅ Actions performedReviews resumed. |
aknysh
approved these changes
Sep 24, 2025
Member
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/list/format/table.go (1)
254-270: Fix width estimation: honor vars map like createRows.getMaxValueWidth ignores nested vars and only inspects top-level keys, so estimatedWidth can be too small vs. rendered content. Align it with createRows to prevent mis-sizing and spurious “table too wide” errors.
Apply this diff:
func getMaxValueWidth(stackData map[string]interface{}, valueKeys []string) int { - maxWidth := 0 - - for _, valueKey := range valueKeys { - if val, ok := stackData[valueKey]; ok { - formattedValue := formatTableCellValue(val) - valueWidth := len(formattedValue) - - if valueWidth > maxWidth { - maxWidth = valueWidth - } - } - } + maxWidth := 0 + // Prefer nested vars when present (matches createRows behavior). + var varsData map[string]interface{} + if v, ok := stackData["vars"].(map[string]interface{}); ok { + varsData = v + } + for _, valueKey := range valueKeys { + var val interface{} + if varsData != nil { + val, _ = varsData[valueKey] + } else { + val, _ = stackData[valueKey] + } + if val != nil { + formattedValue := formatTableCellValue(val) + if w := len(formattedValue); w > maxWidth { + maxWidth = w + } + } + } return limitWidth(maxWidth) }pkg/schema/schema.go (1)
50-51: Fix JSON tag typo for StackType ("StackType" → "stackType").JSON tag uses incorrect capitalization, breaking JSON (inconsistent with yaml/mapstructure).
- StackType string `yaml:"stackType,omitempty" json:"StackType,omitempty" mapstructure:"stackType"` + StackType string `yaml:"stackType,omitempty" json:"stackType,omitempty" mapstructure:"stackType"`internal/exec/describe_affected.go (1)
320-328: Guard against nil git refs before building request.
headHeadorbaseHeadcan be nil, leading to a panic onHash(). Add a defensive check.+ if headHead == nil || baseHead == nil { + return fmt.Errorf("cannot upload affected stacks: missing git refs (head/base)") + } req := dtos.UploadAffectedStacksRequest{ HeadSHA: headHead.Hash().String(), BaseSHA: baseHead.Hash().String(),pkg/list/list_values_test.go (3)
154-164: JSON test also pulls in ‘prod’ without includeAbstract.Align with behavior or remove the assertion.
Apply (remove prod):
- assert.Contains(t, result, "prod")
353-360: CSV scalar query includes prod unexpectedly.By default, abstracts are filtered; header/value counts should match dev+staging.
Apply:
- assert.Contains(t, output, "Key,dev,prod,staging") - assert.Contains(t, output, "value,us-east-1,us-east-1,us-east-1") + assert.Contains(t, output, "Key,dev,staging") + assert.Contains(t, output, "value,us-east-1,us-east-1")
382-387: TSV array query includes prod unexpectedly.Keep only non-abstract envs unless includeAbstract is true.
Apply:
- assert.Contains(t, output, "Key\tdev\tprod\tstaging") + assert.Contains(t, output, "Key\tdev\tstaging") @@ - assert.Contains(t, output, "10.2.1.0/24,10.2.2.0/24") // prod stack subnetspkg/utils/slice_utils.go (1)
10-18: Comments need periods per godot lint.All function comments must end with periods to comply with the golangci-lint godot rule.
-// SliceContainsString checks if a string is present in a slice. +// SliceContainsString checks if a string is present in a slice. -// SliceContainsInt checks if an int is present in a slice. +// SliceContainsInt checks if an int is present in a slice. -// SliceContainsStringStartsWith checks if a slice contains a string that the given string begins with. +// SliceContainsStringStartsWith checks if a slice contains a string that the given string begins with. -// SliceContainsStringHasPrefix checks if a slice contains a string that begins with the given prefix. +// SliceContainsStringHasPrefix checks if a slice contains a string that begins with the given prefix. -// SliceOfStringsToSpaceSeparatedString joins a slice of strings into a single space-separated string. +// SliceOfStringsToSpaceSeparatedString joins a slice of strings into a single space-separated string.Also applies to: 20-28, 30-38, 40-48, 50-53
🧹 Nitpick comments (27)
tests/fixtures/scenarios/atmos-pro/.gitignore (1)
3-4: Remove unlikely patterns for “.terraform.tfstate” directories.These paths don’t exist in Terraform; the non-dotted patterns below already cover state files/dirs.
-**/.terraform.tfstate/** -**/.terraform.tfstate.backup/**pkg/pro/api_client_exchange_oidc_token_test.go (1)
114-128: Rename test to reflect behavior (it’s not asserting a marshal error).The test exercises a successful path; the current name is misleading.
-func TestExchangeOIDCTokenForAtmosToken_MarshalError(t *testing.T) { +func TestExchangeOIDCTokenForAtmosToken_MarshalSuccess(t *testing.T) {pkg/pro/api_client_test.go (2)
16-24: Remove unused MockRoundTripper.This helper isn’t referenced; safe to delete.
-type MockRoundTripper struct { - mock.Mock -} - -func (m *MockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - args := m.Called(req) - return args.Get(0).(*http.Response), args.Error(1) -}
26-45: Consider t.Setenv for test isolation.It simplifies env setup/teardown per subtest and avoids global defer state juggling.
pkg/list/format/table.go (1)
108-126: DRY value resolution across renderers and estimators.Extract a shared resolver to avoid future drift between createRows and getMaxValueWidth.
Example helper:
func resolveValue(stackData map[string]interface{}, key string) (interface{}, bool) { if stackData == nil { return nil, false } if vars, ok := stackData["vars"].(map[string]interface{}); ok { v, ok := vars[key] return v, ok } v, ok := stackData[key] return v, ok }Then use it in both places before formatting the cell.
Also applies to: 254-270
pkg/pro/dtos/lock_stack_test.go (1)
55-73: Avoid anonymous struct for Data; prefer a named DTO to reduce brittleness.Anonymous struct literals must match the production anonymous type exactly, making tests fragile. Consider a named type (e.g., LockInfo) used in both the DTO and tests.
Also applies to: 96-106
tests/fixtures/scenarios/atmos-pro/stacks/catalog/mock.yaml (1)
1-1: Schema URL likely should be stack-config, not atmos-manifest.This file is a stack config (components: terraform: …). Point the schema to stack-config for correct editor validation.
-# yaml-language-server: $schema=https://atmos.tools/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json +# yaml-language-server: $schema=https://atmos.tools/schemas/atmos/stack-config/1.0/stack-config.jsonpkg/schema/schema.go (1)
733-738: Consider further splitting to reduce file length.schema.go remains quite large; continuing to move logical groups (e.g., Docs/Templates) into dedicated files will help keep within file-length limits and improve maintainability.
pkg/schema/atlantis.go (1)
3-3: End the comment with a period (godot).This project enforces comment sentences to end with periods.
-// Atlantis integration +// Atlantis integration.pkg/config/const.go (1)
103-112: UploadStatusFlag (no leading "--") is correct for Cobra; normalize flag-constant styleCobra expects plain flag names (no leading dashes) — cmd/terraform_commands.go registers cfg.UploadStatusFlag (value "upload-status"), and tests/CLI use "--upload-status" when building raw args. pkg/config/const.go is inconsistent (most flag constants include a leading "--"); recommend an optional cleanup: store canonical flag names without dashes or centralize a helper to add/remove "--" to avoid confusion.
pkg/pro/api_client_upload_affected_stacks_test.go (1)
17-26: Fix godot: end comments with periods.These comments will trip the linter. Add trailing periods.
- // Verify request method and path + // Verify request method and path. @@ - w.WriteHeader(tc.statusCode) - w.Write([]byte(`{"error": "server error"}`)) + w.WriteHeader(tc.statusCode) + w.Write([]byte(`{"error": "server error"}`)) @@ - // Use an invalid URL that would cause http.NewRequest to fail + // Use an invalid URL that would cause http.NewRequest to fail.Also applies to: 87-91, 132-139
pkg/list/list_instances_integration_test.go (1)
44-60: Unskip by DI: inject dependencies to test upload/process paths.Introduce small interfaces for git/config/API and
ExecuteDescribeStacks, and accept them in the functions. This will let you replacet.Skipwith real tests using mocks.Also applies to: 62-72, 74-91
pkg/pro/api_client_instances.go (2)
30-34: Add trailing periods to comments (godot).- // Guard against nil HTTPClient by ensuring a default client with a sane timeout + // Guard against nil HTTPClient by ensuring a default client with a sane timeout. @@ - // Log safe metadata instead of full payload to prevent secret leakage + // Log safe metadata instead of full payload to prevent secret leakage.Also applies to: 41-44
28-29: Optional: build endpoint with url.JoinPath to avoid double slashes.+ // Build endpoint robustly (handles trailing slashes). + joined, err := url.JoinPath(c.BaseURL, c.BaseAPIEndpoint, "instances") + if err != nil { + return fmt.Errorf("failed to build instances endpoint: %w", err) + } - endpoint := fmt.Sprintf("%s/%s/instances", c.BaseURL, c.BaseAPIEndpoint) + endpoint := joinedAlso add:
import ( + "net/url" )pkg/pro/api_client_instances_test.go (3)
79-80: Assert HTTP method/path/headers for stronger contract.Tighten the success test by validating the request is POST /api/instances with auth and JSON content-type.
Apply:
- mockRoundTripper.On("RoundTrip", mock.Anything).Return(mockResponse, nil) + mockRoundTripper.On("RoundTrip", mock.Anything). + Run(func(args mock.Arguments) { + req := args.Get(0).(*http.Request) + assert.Equal(t, "POST", req.Method) + assert.Equal(t, "/api/instances", req.URL.Path) + assert.Equal(t, "Bearer test-token", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + }). + Return(mockResponse, nil)
148-149: Mirror request assertions in error test.Consistency helps catch regressions on error paths, too.
Apply:
- mockRoundTripper.On("RoundTrip", mock.Anything).Return(mockResponse, nil) + mockRoundTripper.On("RoundTrip", mock.Anything). + Run(func(args mock.Arguments) { + req := args.Get(0).(*http.Request) + assert.Equal(t, "POST", req.Method) + assert.Equal(t, "/api/instances", req.URL.Path) + assert.Equal(t, "Bearer test-token", req.Header.Get("Authorization")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + }). + Return(mockResponse, nil)
18-26: Add coverage for network/read/JSON parse errors.Broaden tests to assert sentinel errors on:
- Transport error (ErrFailedToMakeRequest).
- Invalid JSON (ErrFailedToUnmarshalAPIResponse).
- Body read failure (ErrFailedToReadResponseBody).
I can draft targeted cases using MockRoundTripper returning: (nil, err), a 200 with invalid body, and a 200 with a failing io.ReadCloser.
Also applies to: 74-78, 143-147
pkg/pro/dtos/instances.go (2)
7-15: Consider omitting empty repo fields in JSON.Avoid sending empty strings when not set.
Apply:
type InstancesUploadRequest struct { - RepoURL string `json:"repo_url"` - RepoName string `json:"repo_name"` - RepoOwner string `json:"repo_owner"` - RepoHost string `json:"repo_host"` + RepoURL string `json:"repo_url,omitempty"` + RepoName string `json:"repo_name,omitempty"` + RepoOwner string `json:"repo_owner,omitempty"` + RepoHost string `json:"repo_host,omitempty"` Instances []schema.Instance `json:"instances"` }
17-28: Optionally omit optional fields on status DTO.If GitSHA/RepoURL/RepoHost are optional, omit when empty to reduce payload.
Apply:
type InstanceStatusUploadRequest struct { AtmosProRunID string `json:"atmos_pro_run_id"` - GitSHA string `json:"git_sha"` - RepoURL string `json:"repo_url"` + GitSHA string `json:"git_sha,omitempty"` + RepoURL string `json:"repo_url,omitempty"` RepoName string `json:"repo_name"` RepoOwner string `json:"repo_owner"` - RepoHost string `json:"repo_host"` + RepoHost string `json:"repo_host,omitempty"` Component string `json:"component"` Stack string `json:"stack"` HasDrift bool `json:"has_drift"` }If these are required by the API, skip this change.
pkg/pro/api_client_unlock_test.go (1)
43-45: Optional: DRY the timeout client creation.Extract a helper to reduce repetition across tests.
Add:
func newTestHTTPClient() *http.Client { return &http.Client{Timeout: 5 * time.Second} }Then use HTTPClient: newTestHTTPClient() in all cases.
pkg/schema/instance.go (1)
3-13: Trim JSON noise with omitempty on maps.Empty maps serialize as {}. If the API tolerates missing fields, omit them when empty.
Apply:
type Instance struct { Component string `yaml:"component" json:"component" mapstructure:"component"` Stack string `yaml:"stack" json:"stack" mapstructure:"stack"` ComponentType string `yaml:"component_type" json:"component_type" mapstructure:"component_type"` - Settings AtmosSectionMapType `yaml:"settings" json:"settings" mapstructure:"settings"` - Vars AtmosSectionMapType `yaml:"vars" json:"vars" mapstructure:"vars"` - Env AtmosSectionMapType `yaml:"env" json:"env" mapstructure:"env"` - Backend AtmosSectionMapType `yaml:"backend" json:"backend" mapstructure:"backend"` - Metadata AtmosSectionMapType `yaml:"metadata" json:"metadata" mapstructure:"metadata"` + Settings AtmosSectionMapType `yaml:"settings" json:"settings,omitempty" mapstructure:"settings"` + Vars AtmosSectionMapType `yaml:"vars" json:"vars,omitempty" mapstructure:"vars"` + Env AtmosSectionMapType `yaml:"env" json:"env,omitempty" mapstructure:"env"` + Backend AtmosSectionMapType `yaml:"backend" json:"backend,omitempty" mapstructure:"backend"` + Metadata AtmosSectionMapType `yaml:"metadata" json:"metadata,omitempty" mapstructure:"metadata"` }If consumers expect explicit empty objects, skip.
pkg/pro/dtos/instances_test.go (1)
63-68: Use map[string]any consistently.Minor modernization for Go 1.18+.
Apply:
- Settings: map[string]interface{}{"drift_detection_enabled": true}, - Vars: map[string]interface{}{"environment": "prod"}, - Env: map[string]interface{}{"TF_VAR_region": "us-west-2"}, - Backend: map[string]interface{}{"type": "s3"}, - Metadata: map[string]interface{}{"description": "Web application"}, + Settings: map[string]any{"drift_detection_enabled": true}, + Vars: map[string]any{"environment": "prod"}, + Env: map[string]any{"TF_VAR_region": "us-west-2"}, + Backend: map[string]any{"type": "s3"}, + Metadata: map[string]any{"description": "Web application"},Repeat similarly for the second instance and assertions.
Also applies to: 73-78, 99-104, 109-114
pkg/list/list_instances_cmd_test.go (1)
13-13: Fix godot: end comment sentences with periods.Keeps golangci-lint happy.
Apply:
-// --- Mocks +// --- Mocks. @@ -// --- Tests +// --- Tests. @@ - // describe mock + // Describe mock. @@ - // execute logic similar to command + // Execute logic similar to command. @@ - // Verify API received correct payload + // Verify API received correct payload.Also applies to: 32-32, 103-103, 109-109, 134-134
pkg/utils/slice_test.go (2)
65-65: Rebinding loop variable is unnecessary in Go 1.22+.The
tc := tcpattern is no longer needed with Go 1.22's loop variable semantics change.for _, tc := range testCases { - tc := tc // rebind to avoid range-variable capture t.Run(tc.name, func(t *testing.T) {
355-361: Consider simplifying the branching logic.The nested if-else structure could be flattened for better readability.
if tc.expectError { assertErrorCase(t, &tc, err, result) -} else { + return +} assert.NoError(t, err) assert.Equal(t, tc.expected, result) -}internal/exec/terraform_test.go (1)
682-683: Use proper YAML formatting instead of string concatenation.The current approach using
fmt.Sprintfwith embedded newlines is fragile. Consider using a YAML library or multiline string.-stackContent := fmt.Sprintf("components:\n terraform:\n %s:\n settings:\n pro:\n enabled: %v\n vars:\n foo: %s-a\n bar: %s-b\n baz: %s-c", - tc.component, tc.proEnabled, tc.component, tc.component, tc.component) +stackContent := fmt.Sprintf(`components: + terraform: + %s: + settings: + pro: + enabled: %v + vars: + foo: %s-a + bar: %s-b + baz: %s-c +`, tc.component, tc.proEnabled, tc.component, tc.component, tc.component)pkg/list/list_instances.go (1)
10-23: Separate third-party and Atmos imports.Add a blank line between third-party imports and Atmos imports for better readability.
"golang.org/x/term" errUtils "github.com/cloudposse/atmos/errors"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (65)
cmd/cmd_utils.go(0 hunks)cmd/list_instances.go(1 hunks)cmd/terraform_commands.go(2 hunks)errors/errors.go(2 hunks)internal/exec/describe_affected.go(2 hunks)internal/exec/pro.go(7 hunks)internal/exec/pro_test.go(1 hunks)internal/exec/terraform.go(5 hunks)internal/exec/terraform_test.go(2 hunks)internal/exec/validate_utils.go(3 hunks)pkg/config/const.go(1 hunks)pkg/config/load.go(1 hunks)pkg/config/utils.go(1 hunks)pkg/downloader/get_git_test.go(2 hunks)pkg/git/git.go(2 hunks)pkg/list/format/table.go(2 hunks)pkg/list/list_instances.go(1 hunks)pkg/list/list_instances_cmd_test.go(1 hunks)pkg/list/list_instances_comprehensive_test.go(1 hunks)pkg/list/list_instances_integration_test.go(1 hunks)pkg/list/list_instances_pro_test.go(1 hunks)pkg/list/list_values_test.go(4 hunks)pkg/pro/api_client.go(7 hunks)pkg/pro/api_client_exchange_oidc_token_test.go(4 hunks)pkg/pro/api_client_get_github_oidc_token_test.go(4 hunks)pkg/pro/api_client_instance_status.go(1 hunks)pkg/pro/api_client_instance_status_test.go(1 hunks)pkg/pro/api_client_instances.go(1 hunks)pkg/pro/api_client_instances_test.go(1 hunks)pkg/pro/api_client_lock_test.go(8 hunks)pkg/pro/api_client_test.go(7 hunks)pkg/pro/api_client_unlock_test.go(8 hunks)pkg/pro/api_client_upload_affected_stacks_test.go(8 hunks)pkg/pro/dtos/atmos_api_response.go(1 hunks)pkg/pro/dtos/atmos_api_response_test.go(1 hunks)pkg/pro/dtos/instances.go(1 hunks)pkg/pro/dtos/instances_test.go(1 hunks)pkg/pro/dtos/lock_stack_test.go(1 hunks)pkg/schema/atlantis.go(1 hunks)pkg/schema/command.go(1 hunks)pkg/schema/instance.go(1 hunks)pkg/schema/markdown.go(1 hunks)pkg/schema/pro.go(1 hunks)pkg/schema/schema.go(4 hunks)pkg/schema/vendor_component.go(1 hunks)pkg/schema/version.go(1 hunks)pkg/schema/workflow.go(1 hunks)pkg/utils/slice_test.go(2 hunks)pkg/utils/slice_utils.go(6 hunks)pkg/validate/validate_component_test.go(1 hunks)tests/fixtures/scenarios/atmos-pro/.gitignore(1 hunks)tests/fixtures/scenarios/atmos-pro/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/catalog/mock.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/deploy/_defaults.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/deploy/prod.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/mixins/atmos-pro.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden(0 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_list_instances.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_list_instances_no_tty.stdout.golden(1 hunks)tests/test-cases/atmos-pro.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- cmd/cmd_utils.go
- tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden
🧰 Additional context used
🧠 Learnings (68)
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
tests/fixtures/scenarios/atmos-pro/.gitignorepkg/list/list_instances.gotests/fixtures/scenarios/atmos-pro/atmos.yaml
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
tests/fixtures/scenarios/atmos-pro/.gitignoretests/test-cases/atmos-pro.yamltests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stderr.goldentests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yamltests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances_no_tty.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances.stdout.goldentests/fixtures/scenarios/atmos-pro/atmos.yamlinternal/exec/terraform_test.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
tests/fixtures/scenarios/atmos-pro/.gitignoretests/test-cases/atmos-pro.yamltests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yamltests/fixtures/scenarios/atmos-pro/stacks/deploy/_defaults.yamltests/fixtures/scenarios/atmos-pro/stacks/deploy/prod.yamltests/fixtures/scenarios/atmos-pro/atmos.yaml
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
PR: cloudposse/atmos#934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
tests/fixtures/scenarios/atmos-pro/.gitignoretests/fixtures/scenarios/atmos-pro/atmos.yaml
📚 Learning: 2025-09-07T17:38:17.619Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/src/components/Screengrabs/demo-stacks/deploy-dev.html:28-37
Timestamp: 2025-09-07T17:38:17.619Z
Learning: Screengrab files in the CloudPosse/atmos repository are programmatically generated and should be ignored during code reviews. Do not provide suggestions or comments on files in screengrab directories or screengrab-related HTML files.
Applied to files:
tests/fixtures/scenarios/atmos-pro/.gitignore
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands
Applied to files:
pkg/schema/command.gocmd/terraform_commands.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*.go : All comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
Applied to files:
pkg/pro/dtos/atmos_api_response_test.gopkg/pro/api_client_instances.gopkg/config/const.gopkg/utils/slice_utils.gopkg/pro/api_client.gopkg/list/list_instances_cmd_test.goerrors/errors.gopkg/schema/schema.gointernal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/!(*_test).go : Document all exported functions, types, and methods with Go doc comments
Applied to files:
pkg/pro/dtos/atmos_api_response_test.gopkg/list/list_instances_cmd_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
pkg/pro/dtos/atmos_api_response_test.gopkg/list/list_instances_cmd_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/pro/dtos/atmos_api_response_test.gopkg/pro/api_client_upload_affected_stacks_test.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/utils/slice_test.gopkg/pro/api_client_unlock_test.gopkg/pro/api_client_lock_test.gointernal/exec/terraform_test.gopkg/list/list_values_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/pro/dtos/atmos_api_response_test.gopkg/pro/dtos/instances_test.gopkg/pro/dtos/lock_stack_test.gopkg/list/list_instances_cmd_test.gopkg/list/list_instances_integration_test.go
📚 Learning: 2025-02-14T23:12:38.030Z
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-02-14T23:12:38.030Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Applied to files:
tests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stderr.goldentests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled_no_tty.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances_drift_enabled.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances_no_tty.stdout.goldentests/snapshots/TestCLICommands_atmos_list_instances.stdout.golden
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/pro/api_client_upload_affected_stacks_test.gointernal/exec/describe_affected.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/pro/api_client_get_github_oidc_token_test.gopkg/pro/api_client_unlock_test.gointernal/exec/validate_utils.gopkg/pro/api_client.gointernal/exec/terraform.gopkg/pro/api_client_lock_test.gointernal/exec/pro.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:
pkg/pro/api_client_upload_affected_stacks_test.gointernal/exec/describe_affected.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/pro/api_client_unlock_test.gopkg/pro/api_client.gointernal/exec/terraform.gopkg/pro/api_client_lock_test.gointernal/exec/pro.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
pkg/pro/api_client_upload_affected_stacks_test.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/pro/api_client_unlock_test.gopkg/pro/api_client_lock_test.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:
pkg/pro/api_client_upload_affected_stacks_test.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/config/const.gopkg/pro/api_client_instance_status_test.gopkg/pro/api_client_unlock_test.gopkg/pro/api_client.gointernal/exec/terraform.gopkg/pro/api_client_lock_test.gointernal/exec/pro.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/pro/api_client_test.gopkg/config/load.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
internal/exec/describe_affected.gointernal/exec/terraform.gocmd/terraform_commands.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/describe_affected.gopkg/pro/api_client.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/describe_affected.gointernal/exec/terraform.gointernal/exec/pro.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
internal/exec/describe_affected.gointernal/exec/terraform.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/describe_affected.gointernal/exec/pro.go
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
PR: cloudposse/atmos#810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Applied to files:
tests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yamltests/fixtures/scenarios/atmos-pro/stacks/deploy/prod.yaml
📚 Learning: 2025-02-24T22:46:39.744Z
Learnt from: haitham911
PR: cloudposse/atmos#1085
File: pkg/config/load.go:219-221
Timestamp: 2025-02-24T22:46:39.744Z
Learning: In the Atmos configuration system, imports from atmos.d are optional. When import errors occur, they should be logged at debug level and the process should continue, rather than failing completely.
Applied to files:
pkg/list/list_instances.go
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
pkg/config/const.gocmd/terraform_commands.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:
pkg/config/const.gopkg/config/load.gointernal/exec/pro.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Applied to files:
pkg/pro/api_client_instance_status.gopkg/pro/api_client.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to errors/errors.go : Define all static errors in errors/errors.go and use these for wrapping errors
Applied to files:
pkg/pro/api_client_instance_status.gopkg/pro/api_client.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*.go : Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
Applied to files:
pkg/pro/api_client.gopkg/git/git.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/pro/api_client.gointernal/exec/pro.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:
pkg/pro/api_client.gointernal/exec/pro.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/pro/api_client.gointernal/exec/pro.gopkg/git/git.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/list_instances.gointernal/exec/terraform.gocmd/terraform_commands.gointernal/exec/pro.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
cmd/list_instances.gointernal/exec/pro.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
cmd/list_instances.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
PR: cloudposse/atmos#825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
cmd/list_instances.gointernal/exec/terraform.goerrors/errors.gointernal/exec/terraform_test.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
cmd/list_instances.gointernal/exec/pro.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/terraform.gointernal/exec/terraform_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*.go : Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
Applied to files:
internal/exec/terraform.gointernal/exec/pro.go
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
internal/exec/terraform.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
PR: cloudposse/atmos#984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
internal/exec/terraform.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Provide clear error messages to users and troubleshooting hints where appropriate
Applied to files:
pkg/downloader/get_git_test.goerrors/errors.gointernal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Follow Go error handling idioms and use meaningful error messages
Applied to files:
errors/errors.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Use kebab-case for command-line flags
Applied to files:
errors/errors.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*.go : ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Applied to files:
pkg/config/load.gointernal/exec/pro.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:
pkg/config/load.gointernal/exec/pro.go
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
pkg/config/load.go
📚 Learning: 2025-09-23T04:43:31.857Z
Learnt from: osterman
PR: cloudposse/atmos#1430
File: pkg/config/config.go:98-107
Timestamp: 2025-09-23T04:43:31.857Z
Learning: In the Atmos codebase, NO_PAGER environment variable handling is intentionally kept as direct os.Getenv() access in pkg/config/config.go rather than using Viper binding, because adding no_pager to the config file would be confusing for users. This is an acknowledged exception to the normal Viper binding pattern for environment variables.
Applied to files:
pkg/config/load.gointernal/exec/pro.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:
tests/fixtures/scenarios/atmos-pro/atmos.yaml
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
PR: cloudposse/atmos#1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
PR: cloudposse/atmos#775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
internal/exec/terraform_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
pkg/list/list_values_test.gointernal/exec/pro_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*_test.go : Unit tests should focus on pure functions and use table-driven tests
Applied to files:
pkg/list/list_values_test.gointernal/exec/pro_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Consider using testify/mock for creating mock implementations
Applied to files:
internal/exec/pro_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Use Viper for managing configuration, environment variables, and flags in the CLI
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
internal/exec/pro.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/pro.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
PR: cloudposse/atmos#736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Wrap errors with context using fmt.Errorf("context: %w", err)
Applied to files:
pkg/git/git.go
Comment on lines
+330
to
332
| log.Debug("Preparing upload affected stacks request", "req", req) | ||
|
|
||
| return apiClient.UploadAffectedStacks(&req) |
Contributor
There was a problem hiding this comment.
Do not log full request payload; log safe metadata only.
This emits full affected details to logs (may include settings/PII). Log only repo, SHAs, and count.
- log.Debug("Preparing upload affected stacks request", "req", req)
+ log.Debug("Preparing upload affected stacks request",
+ "repo", fmt.Sprintf("%s/%s", req.RepoOwner, req.RepoName),
+ "host", req.RepoHost,
+ "head_sha", req.HeadSHA,
+ "base_sha", req.BaseSHA,
+ "stacks_count", len(req.Stacks),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log.Debug("Preparing upload affected stacks request", "req", req) | |
| return apiClient.UploadAffectedStacks(&req) | |
| log.Debug("Preparing upload affected stacks request", | |
| "repo", fmt.Sprintf("%s/%s", req.RepoOwner, req.RepoName), | |
| "host", req.RepoHost, | |
| "head_sha", req.HeadSHA, | |
| "base_sha", req.BaseSHA, | |
| "stacks_count", len(req.Stacks), | |
| ) | |
| return apiClient.UploadAffectedStacks(&req) |
🤖 Prompt for AI Agents
In internal/exec/describe_affected.go around lines 330 to 332, the current debug
log prints the entire request payload (including full affected details); change
the log to emit only safe metadata: repository identifier, base SHA, head SHA,
and the count of affected stacks. Extract those fields from req (e.g., req.Repo,
req.BaseSHA, req.HeadSHA, len(req.Affected) or equivalent) and call log.Debug
with a concise message and only those values, removing the full req from the log
to avoid exposing settings/PII.
|
These changes were released in v1.191.1-test.0. |
This was referenced Sep 26, 2025
This was referenced Oct 4, 2025
This was referenced Oct 14, 2025
This was referenced Oct 31, 2025
This was referenced Nov 7, 2025
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what
New command for
atmos list instancesIncludes option for
--upload--upload, send the result to Atmos ProNew arg for
atmos terraform plan,--upload-statusOther changes:
This pull request introduces new functionality for uploading Terraform plan results and Atmos instances to the Pro API, improves error handling throughout the codebase, and refactors several areas for clarity and maintainability. The most notable changes are the addition of status upload logic for Terraform plans, new error definitions for Pro API interactions, and the creation of a new command for listing Atmos instances. Below are the key changes grouped by theme.
Pro API Integration and Terraform Plan Status Upload:
uploadStatusfunction and a flag (upload-status) for theplancommand incmd/terraform_commands.goandinternal/exec/pro.go. This enables reporting of plan status and drift detection to the Pro API. [1] [2]shouldUploadStatusfunction to determine when status should be uploaded, based on command type and component settings.Error Handling Improvements:
errors/errors.go, and refactored error wrapping to provide more context throughout the exec logic. [1] [2] [3] [4] [5] [6]Atmos Instances Listing Command:
list instancescommand incmd/list_instances.goto list Atmos instances and optionally upload them to the Pro API. This includes flag handling and integration with the existing list command structure.API Client Construction Refactoring:
NewAtmosProAPIClientFromEnvand related usage ininternal/exec/describe_affected.goandinternal/exec/pro.go. [1] [2] [3] [4]Minor Codebase Cleanups:
cmd/cmd_utils.go,cmd/terraform_commands.go, andinternal/exec/describe_affected.go. [1] [2] [3]These changes collectively improve the reliability, maintainability, and feature set of the Atmos CLI, especially around integration with the Pro API and error visibility.
why
We want to build drift detection into Atmos Pro. In order to do this, we need to upload an object similar to the describe affected object. This object needs specific component instances for specific stacks
With
atmos list instances --upload, we can find all component and stacks with drift detection and upload to Atmos ProWith
atmos terraform plan --upload-status, we can return the result of whether or not there is drift to APreferences
screenshots
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests