Conversation
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds a Component Workdir feature: CLI subcommand "workdir" (list/describe/show/clean), a workdir provisioner service with filesystem/hasher/path-filter abstractions and defaults, metadata persistence and cleanup APIs, Terraform execution override via WorkdirPathKey, new sentinel errors, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgb(250,250,252)
participant CLI as Atmos CLI
participant WM as WorkdirManager (CLI)
participant Prov as workdir.Service
participant FS as FileSystem
participant H as Hasher
participant TF as Terraform exec
end
CLI->>WM: parse flags → build ConfigAndStacksInfo
CLI->>Prov: ProvisionWorkdir(ctx, atmosConfig, componentConfig)
Prov->>FS: MkdirAll(.workdir/terraform/<component>)
Prov->>FS: CopyDir(component_path → workdir)
Prov->>H: HashDir(workdir)
H-->>Prov: content_hash
Prov->>FS: WriteFile(.workdir-metadata.json)
Prov-->>CLI: set WorkdirPathKey in component config
CLI->>TF: start Terraform using WorkdirPathKey (override componentPath)
TF-->>CLI: terraform run completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/*_test.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
🧠 Learnings (45)📓 Common learnings📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-05-23T19:51:47.091ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2024-11-02T15:35:09.958ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2024-12-25T20:28:47.526ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-05-23T19:51:47.091ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2024-10-27T04:54:32.397ZApplied to files:
📚 Learning: 2024-10-27T04:28:40.966ZApplied to files:
📚 Learning: 2025-05-23T19:51:47.091ZApplied to files:
📚 Learning: 2024-10-28T01:51:30.811ZApplied to files:
📚 Learning: 2024-11-12T03:16:02.910ZApplied to files:
📚 Learning: 2025-12-13T06:10:13.688ZApplied to files:
📚 Learning: 2024-12-07T16:16:13.038ZApplied to files:
📚 Learning: 2025-12-13T06:10:25.156ZApplied to files:
📚 Learning: 2025-10-10T23:51:36.597ZApplied to files:
📚 Learning: 2025-09-08T01:25:44.958ZApplied to files:
📚 Learning: 2025-11-11T03:47:45.878ZApplied to files:
📚 Learning: 2024-12-25T20:28:19.618ZApplied to files:
📚 Learning: 2024-12-02T21:26:32.337ZApplied to files:
📚 Learning: 2025-03-18T12:26:25.329ZApplied to files:
📚 Learning: 2025-12-10T18:32:51.237ZApplied to files:
📚 Learning: 2025-08-15T14:43:41.030ZApplied to files:
📚 Learning: 2024-10-27T04:41:49.199ZApplied to files:
📚 Learning: 2025-12-21T04:10:29.030ZApplied to files:
📚 Learning: 2025-09-08T01:25:44.958ZApplied to files:
📚 Learning: 2025-09-08T01:25:44.958ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2024-11-24T19:13:10.287ZApplied to files:
📚 Learning: 2025-01-25T04:01:58.095ZApplied to files:
📚 Learning: 2025-12-26T06:31:02.244ZApplied to files:
📚 Learning: 2025-04-26T15:54:10.506ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-05-23T19:51:47.091ZApplied to files:
📚 Learning: 2025-11-11T03:47:59.576ZApplied to files:
📚 Learning: 2025-01-09T22:27:25.538ZApplied to files:
🧬 Code graph analysis (3)pkg/provisioner/workdir/integration_test.go (4)
pkg/provisioner/workdir/clean.go (7)
tests/cli_workdir_test.go (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (24)
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 |
📝 WalkthroughWalkthroughIntroduces a workdir provisioner system that creates isolated per-component Terraform working directories with JIT source vendoring, caching, and metadata management. Adds CLI subcommands for listing, describing, showing, and cleaning workdirs, plus provisioner hooks that run before terraform init to populate workdir contents from remote or local sources. Changes
Sequence DiagramsequenceDiagram
participant CLI as Atmos CLI
participant Provisioner as ProvisionWorkdir
participant Cache as Cache
participant Downloader as Downloader
participant FileSystem as FileSystem
participant Terraform as Terraform
CLI->>Provisioner: ProvisionWorkdir(componentConfig)
Provisioner->>Provisioner: Check if activation needed
alt Source metadata or workdir enabled
Provisioner->>FileSystem: Create workdir directory
FileSystem-->>Provisioner: workdir path
alt Remote source
Provisioner->>Cache: GenerateKey(source)
Cache-->>Provisioner: cache key
Provisioner->>Cache: Get(key)
alt Cache hit
Cache-->>Provisioner: CacheEntry
Provisioner->>FileSystem: Copy from cache path
else Cache miss
Provisioner->>Downloader: Download(source URI)
Downloader-->>Provisioner: Downloaded to temp
Provisioner->>Cache: Put(key, temp path)
Cache-->>Provisioner: Cached
Provisioner->>FileSystem: Copy from cache
end
else Local source
Provisioner->>FileSystem: Copy component directory
end
Provisioner->>FileSystem: HashDir(workdir)
FileSystem-->>Provisioner: content hash
Provisioner->>FileSystem: WriteFile(metadata.json)
FileSystem-->>Provisioner: Written
Provisioner-->>Terraform: _workdir_path in componentConfig
else Not activated
Provisioner-->>CLI: Skip (return nil)
end
CLI->>Terraform: Execute with overridden componentPath
Terraform->>Terraform: Use workdir path instead of component path
Terraform-->>CLI: Completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (14)
cmd/terraform/workdir/workdir_describe.go (1)
57-58: Usedata.Writefor pipeable output.Per coding guidelines, use
data.*for stdout output that should be pipeable, notfmt.Print.+ "github.com/cloudposse/atmos/pkg/data"// Output the manifest. - fmt.Print(manifest) + data.Write(manifest) return nilpkg/provisioner/workdir/workdir_test.go (1)
9-10: Remove no-op cleanup.
defer t.Cleanup(func() {})does nothing - it registers an empty cleanup function. Remove it.func TestExtractSourceConfig(t *testing.T) { - defer t.Cleanup(func() {}) - tests := []struct {pkg/provisioner/workdir/clean.go (1)
132-136: Error aggregation loses underlying errors.When multiple errors occur, only the count is reported. Consider including the first error as the cause for better debugging.
if len(errs) > 0 { return errUtils.Build(errUtils.ErrWorkdirClean). + WithCause(errs[0]). WithExplanation(fmt.Sprintf("%d error(s) occurred during cleanup", len(errs))). Err() }cmd/terraform/workdir/workdir_list.go (3)
36-43: Missing global flags parsing before config initialization.Per the established pattern, populate
ConfigAndStacksInfowith global flag values by callingflags.ParseGlobalFlags(cmd, v)beforeInitCliConfig. Without this, flags like--base-path,--config,--config-path, and--profilewill be ignored.v := viper.GetViper() format := v.GetString("format") + // Parse global flags into ConfigAndStacksInfo. + info, err := flags.ParseGlobalFlags(cmd, v) + if err != nil { + return errUtils.Build(errUtils.ErrWorkdirMetadata). + WithCause(err). + WithExplanation("Failed to parse global flags"). + Err() + } + // Initialize config. - atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + atmosConfig, err := cfg.InitCliConfig(info, false)Based on learnings about InitCliConfig requiring populated ConfigAndStacksInfo.
64-86: Use data. utilities for pipeable output.*Per coding guidelines, JSON and YAML are pipeable data outputs and should use
data.WriteJSONanddata.WriteYAMLinstead offmt.Println/fmt.Print. This ensures consistent stream handling and masking.+import "github.com/cloudposse/atmos/pkg/data" + func printListJSON(workdirs []WorkdirInfo) error { - jsonData, err := json.MarshalIndent(workdirs, "", " ") - if err != nil { - return errUtils.Build(errUtils.ErrWorkdirMetadata). - WithCause(err). - WithExplanation("Failed to marshal workdirs to JSON"). - Err() - } - fmt.Println(string(jsonData)) - return nil + return data.WriteJSON(workdirs) } func printListYAML(workdirs []WorkdirInfo) error { - yamlData, err := yaml.Marshal(workdirs) - if err != nil { - return errUtils.Build(errUtils.ErrWorkdirMetadata). - WithCause(err). - WithExplanation("Failed to marshal workdirs to YAML"). - Err() - } - fmt.Print(string(yamlData)) - return nil + return data.WriteYAML(workdirs) }
88-129: Use ui. for human-readable messages.*Per guidelines, use
ui.Writelnor similar for human messages to stderr instead offmt.Fprintln(os.Stderr, ...). This ensures consistent formatting and theme application.+import "github.com/cloudposse/atmos/pkg/ui" + func printListTable(workdirs []WorkdirInfo) { if len(workdirs) == 0 { - fmt.Fprintln(os.Stderr, "No workdirs found") + ui.Writeln("No workdirs found") return } // ... table building ... - fmt.Fprintln(os.Stderr, t) + ui.Writeln(t.String()) }cmd/terraform/workdir/workdir_clean.go (2)
62-69: Missing global flags parsing before config initialization.Same as
workdir_list.go- populateConfigAndStacksInfoviaflags.ParseGlobalFlags(cmd, v)before callingInitCliConfigto respect global CLI flags.+ // Parse global flags. + info, err := flags.ParseGlobalFlags(cmd, v) + if err != nil { + return errUtils.Build(errUtils.ErrWorkdirClean). + WithCause(err). + WithExplanation("Failed to parse global flags"). + Err() + } + // Initialize config. - atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + atmosConfig, err := cfg.InitCliConfig(info, false)
81-97: Use ui. for success messages.*Per guidelines, human-facing messages should use
ui.Successrather than directfmt.Fprintf(os.Stderr, ...).+import "github.com/cloudposse/atmos/pkg/ui" + func cleanAllWorkdirs(atmosConfig *schema.AtmosConfiguration) error { if err := workdirManager.CleanAllWorkdirs(atmosConfig); err != nil { return err } - fmt.Fprintf(os.Stderr, "%s All workdirs cleaned\n", theme.Styles.Checkmark.String()) + ui.Success("All workdirs cleaned") return nil } func cleanSpecificWorkdir(atmosConfig *schema.AtmosConfiguration, component, stack string) error { if err := workdirManager.CleanWorkdir(atmosConfig, component, stack); err != nil { return err } - fmt.Fprintf(os.Stderr, "%s Workdir cleaned for %s in %s\n", theme.Styles.Checkmark.String(), component, stack) + ui.Successf("Workdir cleaned for %s in %s", component, stack) return nil }cmd/terraform/workdir/workdir_helpers_test.go (2)
122-130: Use assert.ErrorIs for sentinel error checking.Per coding guidelines, always use
assert.ErrorIs()in tests, neverassert.Contains(err.Error(), ...)or justassert.Error()when checking for specific sentinel errors.func TestDefaultWorkdirManager_GetWorkdirInfo_NotFound(t *testing.T) { tmpDir := t.TempDir() manager := NewDefaultWorkdirManager() atmosConfig := &schema.AtmosConfiguration{BasePath: tmpDir} _, err := manager.GetWorkdirInfo(atmosConfig, "vpc", "dev") - assert.Error(t, err) + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrWorkdirMetadata) }You'll need to import
errUtils "github.com/cloudposse/atmos/errors"and verify the correct sentinel error.
189-197: Use assert.ErrorIs for sentinel error checking.Same as above - use
assert.ErrorIswith the appropriate sentinel error for consistent error verification.func TestDefaultWorkdirManager_CleanWorkdir_NotFound(t *testing.T) { tmpDir := t.TempDir() manager := NewDefaultWorkdirManager() atmosConfig := &schema.AtmosConfiguration{BasePath: tmpDir} err := manager.CleanWorkdir(atmosConfig, "vpc", "dev") - assert.Error(t, err) + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrWorkdirClean) }pkg/provisioner/workdir/cache.go (2)
315-327: Compile regexes once at package level.
regexp.MustCompileis called on every invocation. For frequently called functions, pre-compile at package scope.+var ( + semverRegex = regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$`) + commitSHARegex = regexp.MustCompile(`^[0-9a-f]{7,40}$`) +) + // isSemver checks if a string looks like a semantic version. func isSemver(s string) bool { - // Match v1.2.3, 1.2.3, v1.2.3-rc1, etc. - re := regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$`) - return re.MatchString(s) + return semverRegex.MatchString(s) } // isCommitSHA checks if a string looks like a git commit SHA. func isCommitSHA(s string) bool { - // Full SHA is 40 hex chars, short SHA is 7+. - re := regexp.MustCompile(`^[0-9a-f]{7,40}$`) - return re.MatchString(strings.ToLower(s)) + return commitSHARegex.MatchString(strings.ToLower(s)) }
296-299: Guard against short keys to prevent panic.
key[:2]will panic if key length is less than 2. WhileGenerateKeyalways produces 64-char hashes, defensive coding is advisable.func (c *DefaultCache) contentPath(key string) string { - // Use first 2 chars of hash as subdirectory for sharding. - return filepath.Join(c.basePath, "blobs", key[:2], key, "content") + // Use first 2 chars of hash as subdirectory for sharding. + prefix := key + if len(key) >= 2 { + prefix = key[:2] + } + return filepath.Join(c.basePath, "blobs", prefix, key, "content") }cmd/terraform/workdir/workdir_helpers.go (1)
258-270: Consider sync protection for global manager.The global
workdirManagervariable lacks synchronization. While typically set once during test setup, concurrent access could race. Anatomic.Valueorsync.Oncepattern would be safer.pkg/provisioner/workdir/workdir.go (1)
529-537: Usestrings.Containsinstead of manual loop.This reimplements
strings.Contains. Simpler and clearer to use the standard library.// containsParam checks if the URI contains a specific parameter. func containsParam(uri, param string) bool { - for i := 0; i <= len(uri)-len(param); i++ { - if uri[i:i+len(param)] == param { - return true - } - } - return false + return strings.Contains(uri, param) }
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
website/blog/2025-12-12-component-workdir-isolation.mdx (2)
148-148: Add hyphen to compound modifier.Change "go-getter supported" to "go-getter-supported" when used as a compound adjective.
127-135: Correct the clean command examples.The documented clean commands reference flags that don't match the actual implementation:
atmos terraform clean --allshould beatmos terraform workdir clean --all(new workdir subcommand)atmos terraform clean --cachedoesn't exist in the current implementationThe first example
atmos terraform clean vpc -s devis the traditional component-level clean.Update the examples to reflect the new workdir-specific commands introduced in this PR.
cmd/terraform/workdir/workdir_describe.go (1)
42-49: Pass global flags to InitCliConfig.This duplicates the issue flagged in previous review - global flags like
--base-pathare ignored when passing empty struct.pkg/provisioner/workdir/workdir.go (3)
203-204: Silently ignoring hash error.This was flagged in a previous review. The error from
HashDiris being discarded.
223-230:s.cache.Path("")returns empty string.This was flagged in a previous review. The temp directory path will be malformed.
311-312: Same hash error pattern.Same issue as line 204 - the error from
HashDiris silently ignored.pkg/provisioner/workdir/cache.go (2)
103-125: Data race: modifying entry under read lock.Line 123 mutates
entry.LastAccessedAtwhile holdingRLock. This was flagged in a previous review. Upgrade toLock()or remove the update.
416-421: Useio.EOFinstead of string comparison.This was flagged in a previous review. Replace
err.Error() == "EOF"witherr == io.EOF(orerrors.Is(err, io.EOF)).
🧹 Nitpick comments (9)
pkg/provisioner/workdir/downloader.go (1)
38-40: Consider enablingDisableSymlinksfor security.
DisableSymlinks: falseallows symlinks in downloaded content. This can be exploited via symlink attacks where malicious sources create symlinks pointing outside the destination. For defense-in-depth, consider setting this totrueunless symlink preservation is required.Mode: getter.ClientModeDir, - DisableSymlinks: false, + DisableSymlinks: true,cmd/terraform/workdir/workdir_clean.go (2)
86-86: Useui.*helpers instead of directfmt.Fprintfto stderr.Per coding guidelines, use
ui.Successfor human messages to stderr rather than direct stream access.- fmt.Fprintf(os.Stderr, "%s All workdirs cleaned\n", theme.Styles.Checkmark.String()) + ui.Success("All workdirs cleaned")
95-95: Same pattern - preferui.Successhere.- fmt.Fprintf(os.Stderr, "%s Workdir cleaned for %s in %s\n", theme.Styles.Checkmark.String(), component, stack) + ui.Successf("Workdir cleaned for %s in %s", component, stack)cmd/terraform/workdir/workdir_helpers_test.go (1)
122-130: Consider usingassert.ErrorIsfor error verification.Per coding guidelines, prefer
assert.ErrorIs(err, expectedSentinel)over justassert.Error(t, err)to validate the specific error type._, err := manager.GetWorkdirInfo(atmosConfig, "vpc", "dev") - assert.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrWorkdirMetadata) // or appropriate sentinelcmd/terraform/workdir/workdir_show.go (1)
69-69: Preferui.*for stderr output.Per guidelines, use
ui.Write*helpers rather than directfmt.Fprintfto stderr.pkg/provisioner/workdir/workdir_test.go (1)
9-11: Remove unnecessary empty cleanup.
defer t.Cleanup(func() {})does nothing and can be removed.func TestExtractSourceConfig(t *testing.T) { - defer t.Cleanup(func() {}) - tests := []struct {pkg/provisioner/workdir/clean.go (1)
14-44: Solid cleanup implementation.The error handling with context is well done. One consideration: this uses
os.Stat/os.RemoveAlldirectly while theServicetype uses theFileSysteminterface. For consistency and testability, you might consider either making this aServicemethod or accepting aFileSystemparameter. Not blocking though.pkg/provisioner/workdir/workdir.go (1)
519-537: Consider usingstrings.Containsfor simplicity.The manual character/substring search works but
strings.Containswould be more idiomatic.+import "strings" + // containsQuery checks if the URI contains a query string. func containsQuery(uri string) bool { - for _, c := range uri { - if c == '?' { - return true - } - } - return false + return strings.Contains(uri, "?") } // containsParam checks if the URI contains a specific parameter. func containsParam(uri, param string) bool { - for i := 0; i <= len(uri)-len(param); i++ { - if uri[i:i+len(param)] == param { - return true - } - } - return false + return strings.Contains(uri, param) }pkg/provisioner/workdir/cache.go (1)
315-327: Pre-compile regexes for better performance.
isSemverandisCommitSHAcompile their regexes on every call. These should be package-level variables.+var ( + semverRegex = regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$`) + commitSHARegex = regexp.MustCompile(`^[0-9a-f]{7,40}$`) +) + // isSemver checks if a string looks like a semantic version. func isSemver(s string) bool { - // Match v1.2.3, 1.2.3, v1.2.3-rc1, etc. - re := regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.]+)?$`) - return re.MatchString(s) + return semverRegex.MatchString(s) } // isCommitSHA checks if a string looks like a git commit SHA. func isCommitSHA(s string) bool { - // Full SHA is 40 hex chars, short SHA is 7+. - re := regexp.MustCompile(`^[0-9a-f]{7,40}$`) - return re.MatchString(strings.ToLower(s)) + return commitSHARegex.MatchString(strings.ToLower(s)) }
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
cmd/terraform/workdir/workdir_clean.go (1)
62-70: Global flags handling is now correct.The
buildConfigAndStacksInfohelper properly parses global flags before callingInitCliConfig, addressing the concern from past reviews.cmd/terraform/workdir/workdir.go (1)
15-19: Verifyworkdir.SetAtmosConfigis called in root.go.Per past review and established patterns (version, devcontainer, themeCmd, backend),
SetAtmosConfigmust be called incmd/root.goafterInitCliConfigto initialize the package-level pointer. OtherwiseatmosConfigPtrwill be nil when used in perf.Track calls.#!/bin/bash # Verify workdir.SetAtmosConfig is called in root.go rg -n "workdir.*SetAtmosConfig\|workdir\.SetAtmosConfig" cmd/root.gocmd/terraform/workdir/workdir_describe.go (1)
41-49: Global flags handling is now correct.The
buildConfigAndStacksInfohelper properly addresses the past review concern about ignoring global flags.cmd/terraform/workdir/workdir_show.go (1)
44-52: Global flags handling is now correct.The
buildConfigAndStacksInfohelper properly addresses the past review concern about ignoring global flags.pkg/provisioner/workdir/clean.go (1)
133-138: Error aggregation properly preserves individual errors.The current implementation uses
errors.Join(errs...)which correctly preserves all individual error details in the cause chain, addressing the previous review concern.pkg/provisioner/workdir/workdir.go (2)
204-207: Hash error handling is properly implemented.The function now captures the hash error and logs a warning, addressing the previous review concern about silently ignoring hash failures.
227-233: Cache base path properly retrieved.The function now uses
s.cache.BasePath()method to get the cache directory, addressing the previous review concern about usingPath("").pkg/provisioner/workdir/cache.go (4)
46-68: Thread-safe initialization properly implemented.The function now uses
sync.Onceto ensure basePath initialization happens exactly once, addressing the previous race condition concern.
98-130: Lock upgrade properly implemented.The method now uses write lock to safely update LastAccessedAt, addressing the previous data race concern.
362-407: Symlink handling properly implemented.The function now explicitly checks for symlinks and handles them correctly, addressing the previous review concern.
453-455: EOF check is idiomatic.Direct comparison with
io.EOFis the standard Go pattern for this sentinel error. Usingerrors.Iswould work but adds no value here sinceio.EOFis a simple sentinel.
🧹 Nitpick comments (11)
internal/exec/terraform_shell.go (1)
63-77: Use static error sentinel for wrapping.The provisioner integration logic is sound. One adjustment needed per coding guidelines: use static errors from
errors/errors.goinstead of dynamic error strings.+ errUtils "github.com/cloudposse/atmos/errors"Then at line 70:
- return fmt.Errorf("provisioner execution failed: %w", err) + return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrWorkdirProvision, err)This ensures consistent error handling using the newly added
ErrWorkdirProvisionsentinel.cmd/terraform/workdir/workdir_list.go (4)
72-72: Replacefmt.Printlnwith appropriate output function.Per coding guidelines,
fmt.Printlnshould not be used. For data output to stdout, use the appropriatedata.*output function.
84-84: Replacefmt.Printwith appropriate output function.Same guideline applies here for YAML data output.
88-92: Replacefmt.Fprintln(os.Stderr, ...)withui.*function.Per coding guidelines, UI output to stderr should use
ui.*functions rather than directfmt.Fprintln.
128-128: Replacefmt.Fprintln(os.Stderr, t)withui.*function.Table output is UI and should use the appropriate
ui.*function.cmd/terraform/workdir/workdir_clean.go (2)
87-87: Replacefmt.Fprintf(os.Stderr, ...)withui.*function.Per coding guidelines, UI output should use
ui.*functions rather than directfmt.Fprintf.
96-96: Replacefmt.Fprintf(os.Stderr, ...)withui.*function.Same guideline applies for the specific workdir cleaned message.
cmd/terraform/workdir/workdir_describe.go (1)
58-58: Replacefmt.Printwith appropriate data output function.Per coding guidelines, data outputs should use
data.*functions rather thanfmt.Print.cmd/terraform/workdir/workdir_show.go (2)
69-69: Replacefmt.Fprintf(os.Stderr, ...)withui.*function.Per coding guidelines, UI output to stderr should use
ui.*functions.
105-105: Replacefmt.Fprintf(os.Stderr, ...)withui.*function.Same guideline applies for the table output.
pkg/provisioner/workdir/workdir_test.go (1)
10-10: Remove unnecessary empty cleanup.The empty
t.Cleanup(func() {})serves no purpose and can be removed.func TestExtractSourceConfig(t *testing.T) { - defer t.Cleanup(func() {}) - tests := []struct {
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
internal/exec/terraform_shell.go (1)
63-77: Consider using errUtils.Build for error wrapping.The provisioner integration looks solid - timeout is reasonable, workdir path override logic is clean, and logging provides good visibility. However, per coding guidelines, errors should be wrapped using errUtils.Build for consistency across the codebase.
Optional refactor for consistency:
err = provisioner.ExecuteProvisioners(ctx, provisioner.HookEvent(beforeTerraformInitEvent), atmosConfig, info.ComponentSection, info.AuthContext) if err != nil { - return fmt.Errorf("provisioner execution failed: %w", err) + return errUtils.Build(errUtils.ErrProvisionerFailed). + WithCause(err). + WithContext("hook_event", beforeTerraformInitEvent). + Err() }The existing code works fine; this just aligns with the broader error handling pattern.
pkg/provisioner/workdir/workdir_test.go (1)
9-11: Remove the no-op cleanup call.
defer t.Cleanup(func() {})does nothing. Remove it or replace with actual cleanup logic if needed.func TestExtractSourceConfig(t *testing.T) { - defer t.Cleanup(func() {}) - tests := []struct {pkg/provisioner/workdir/workdir.go (1)
529-551: Consider usingstringspackage for simpler checks.These manual string scanning functions work correctly, but
strings.Containswould be more idiomatic and readable.+import "strings" + // containsRef checks if the URI already contains a ref parameter. func containsRef(uri string) bool { - return containsParam(uri, "ref=") + return strings.Contains(uri, "ref=") } // containsQuery checks if the URI contains a query string. func containsQuery(uri string) bool { - for _, c := range uri { - if c == '?' { - return true - } - } - return false + return strings.Contains(uri, "?") } - -// containsParam checks if the URI contains a specific parameter. -func containsParam(uri, param string) bool { - for i := 0; i <= len(uri)-len(param); i++ { - if uri[i:i+len(param)] == param { - return true - } - } - return false -}pkg/provisioner/workdir/clean.go (1)
19-22: Consider resolving absolute paths before deletion.Based on learnings, when deleting directories, resolving absolute paths using
filepath.Absis recommended. This provides safer deletion semantics and clearer logging.func CleanWorkdir(atmosConfig *schema.AtmosConfiguration, component string) error { defer perf.Track(atmosConfig, "workdir.CleanWorkdir")() basePath := atmosConfig.BasePath if basePath == "" { basePath = "." } - workdirPath := filepath.Join(basePath, WorkdirPath, "terraform", component) + workdirPath, err := filepath.Abs(filepath.Join(basePath, WorkdirPath, "terraform", component)) + if err != nil { + return errUtils.Build(errUtils.ErrWorkdirClean). + WithCause(err). + WithExplanation("failed to resolve absolute workdir path"). + WithContext("component", component). + Err() + }Based on learnings, absolute paths are preferred when deleting directories.
Also applies to: 51-54
pkg/provisioner/workdir/fs.go (1)
141-153: Consider checking Close error.While uncommon, Close can fail (e.g., on NFS). The error is currently ignored.
func (h *DefaultHasher) HashFile(path string) (string, error) { defer perf.Track(nil, "workdir.DefaultHasher.HashFile")() file, err := os.Open(path) if err != nil { return "", err } - defer file.Close() + defer func() { + _ = file.Close() + }() hash := sha256.New()This explicitly ignores the error, making it clear the decision is intentional.
cmd/terraform/workdir/workdir_helpers.go (1)
243-256: Consider wrapping errors for consistency.While this private helper is called from methods that add their own context, wrapping errors here would provide more detailed information about what failed during metadata reading.
Apply this diff to add error context:
func readWorkdirMetadata(path string) (*provWorkdir.WorkdirMetadata, error) { data, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, errUtils.Build(errUtils.ErrWorkdirMetadata). + WithCause(err). + WithContext("path", path). + Err() } var metadata provWorkdir.WorkdirMetadata if err := json.Unmarshal(data, &metadata); err != nil { - return nil, err + return nil, errUtils.Build(errUtils.ErrWorkdirMetadata). + WithCause(err). + WithExplanation("Failed to parse metadata JSON"). + WithContext("path", path). + Err() } return &metadata, nil }As per coding guidelines, all errors should be wrapped using errUtils.Build for consistency, even in private helpers.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1876 +/- ##
==========================================
- Coverage 73.58% 73.55% -0.03%
==========================================
Files 634 643 +9
Lines 59027 59723 +696
==========================================
+ Hits 43433 43930 +497
- Misses 12605 12783 +178
- Partials 2989 3010 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/provisioner/workdir/integration_test.go (1)
220-270: Consider refactoring: tests simulate rather than verify implementation.These three tests simulate the path override logic (lines 230-232, 247-249, 265-267) instead of testing the actual implementation. The comments indicate the real logic is in terraform.go. If the implementation is elsewhere, these simulation tests provide limited value and could diverge from actual behavior.
Consider moving these to integration tests that exercise the actual terraform.go code path, or verifying that the implementation matches this pattern through tests of the actual code.
tests/cli_workdir_test.go (1)
17-50: Prefert.Setenvfor environment variable cleanup.Lines 33-34 use
os.Unsetenv, but per coding guidelines and learnings, tests should uset.Setenvfor automatic test-scoped cleanup.🔎 Proposed fix
- // Clear environment variables. - require.NoError(t, os.Unsetenv("ATMOS_CLI_CONFIG_PATH")) - require.NoError(t, os.Unsetenv("ATMOS_BASE_PATH")) + // Clear environment variables with automatic cleanup. + t.Setenv("ATMOS_CLI_CONFIG_PATH", "") + t.Setenv("ATMOS_BASE_PATH", "")
…ir <subcommand>` website docs
|
@aknysh: I'll conduct a full review of the PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/provisioner/workdir/integration_test.go (3)
57-109: Solid end-to-end integration test.Validates the complete provisioning flow including directory creation, file copying, and stack-component naming pattern. Consider also verifying the metadata file content for completeness.
111-164: Good mock-based unit test, but consider enhancing coverage.The test properly exercises the Service layer with mocked dependencies. However:
- Consider verifying the metadata file content passed to WriteFile (currently uses
gomock.Any())- Add error path tests to ensure proper error handling when MkdirAll, CopyDir, HashDir, or WriteFile fail
These additions would improve coverage toward the >80% target.
192-216: LGTM: Cleanup test covers the happy path.Test properly verifies removal of all workdirs. Consider adding a test case where the
.workdirdirectory doesn't exist to ensure the function handles that gracefully (based on the code snippet, it should log "No workdirs found to clean").
📜 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 (3)
pkg/provisioner/workdir/integration_test.gotests/cli_workdir_test.gotests/fixtures/scenarios/workdir/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli_workdir_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 in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
pkg/provisioner/workdir/integration_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
pkg/provisioner/workdir/integration_test.go
🧠 Learnings (17)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/provisioner/workdir/integration_test.gotests/fixtures/scenarios/workdir/README.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to .github/workflows/*.{yml,yaml} : Configure CI to run unit tests, integration tests, golangci-lint, and coverage reporting on all pull requests
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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/provisioner/workdir/integration_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/provisioner/workdir/integration_test.go
🧬 Code graph analysis (1)
pkg/provisioner/workdir/integration_test.go (4)
pkg/provisioner/registry.go (1)
GetProvisionersForEvent(71-86)pkg/provisioner/workdir/workdir.go (3)
HookEventBeforeTerraformInit(18-18)ProvisionWorkdir(62-72)NewServiceWithDeps(47-54)pkg/provisioner/workdir/types.go (2)
WorkdirPathKey(73-73)WorkdirMetadataFile(51-51)pkg/provisioner/workdir/clean.go (2)
CleanWorkdir(16-45)CleanAllWorkdirs(48-76)
🪛 LanguageTool
tests/fixtures/scenarios/workdir/README.md
[typographical] ~7-~7: To join two clauses or introduce examples, consider using an em dash.
Context: ...ted - provision.workdir.enabled: true - Component workdir isolation - Multiple c...
(DASH_RULE)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (6)
tests/fixtures/scenarios/workdir/README.md (2)
1-45: Solid fixture documentation.The README clearly documents the feature, testing steps, and provides practical examples for manual testing. The commands appropriately showcase the new workdir CLI subcommands.
46-69: Clear directory structure visualization.The tree diagram effectively illustrates the fixture layout, including the isolated workdir execution directories. Proper use of the tree language identifier ensures readable formatting.
pkg/provisioner/workdir/integration_test.go (4)
1-15: LGTM: Imports properly organized.Follows the three-group import pattern with correct separation and sorting.
17-35: LGTM: Registration test covers the essentials.Verifies provisioner registration for the correct hook event with proper assertions.
37-55: LGTM: No-activation path properly tested.Good negative test case verifying the provisioner does nothing when not enabled.
218-220: Good documentation of related tests.Helpful pointer to extraction logic tests in another file.
|
@aknysh: I'll conduct a full review of the PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/fixtures/scenarios/workdir/components/terraform/s3-bucket/main.tf (1)
19-23: Consider adding output for encryption_enabled.The
encryption_enabledvariable doesn't have a corresponding output, while the other variables do. For test fixture symmetry and completeness, you might want to add:output "versioning_enabled" { value = var.versioning_enabled description = "Whether versioning is enabled" } + +output "encryption_enabled" { + value = var.encryption_enabled + description = "Whether encryption is enabled" +}website/src/data/roadmap.js (1)
280-280: Consider adding a changelog link.The milestone correctly reflects the shipped status, quarter, PR number, and updated description. However, other shipped milestones in this initiative (like line 276) include a
changelogfield. If there's a corresponding blog post or changelog entry for this feature, consider adding it here for consistency.🔎 Example with changelog field
- { label: 'Component workdir provisioning', status: 'shipped', quarter: 'q4-2025', pr: 1876, description: 'Isolated working directories for each component instance, eliminating conflicts when multiple instances share the same component source.', benefits: 'Enables concurrent component execution and just-in-time vendoring. Multiple component instances no longer conflict by overwriting each other in the same directory.' }, + { label: 'Component workdir provisioning', status: 'shipped', quarter: 'q4-2025', pr: 1876, changelog: 'component-workdir-provisioning', description: 'Isolated working directories for each component instance, eliminating conflicts when multiple instances share the same component source.', benefits: 'Enables concurrent component execution and just-in-time vendoring. Multiple component instances no longer conflict by overwriting each other in the same directory.' },Otherwise, the update looks solid—status, quarter, PR reference, and description all align with the PR objectives.
internal/exec/terraform_shell_test.go (1)
103-192: Refactor to test actual implementations instead of reimplementing logic.The test reimplements the workdir path extraction logic (lines 176-179) rather than calling the actual functions from
terraform_shell.go:74-77andterraform.go:409-411. This creates three copies of the same logic, which can drift over time.Consider extracting the workdir path logic into a shared helper function (e.g.,
extractWorkdirPath(componentSection map[string]any, defaultPath string) string) and testing that function directly. This ensures:
- Changes to the extraction logic are caught by tests
- Logic stays in sync across all usage sites
- Easier maintenance
🔎 Suggested refactor approach
- Extract a shared helper in
internal/exec/terraform_utils.go:// ExtractWorkdirPath retrieves the workdir path from component section if present, // otherwise returns the default path. func ExtractWorkdirPath(componentSection map[string]any, defaultPath string) string { if workdirPath, ok := componentSection[provWorkdir.WorkdirPathKey].(string); ok && workdirPath != "" { return workdirPath } return defaultPath }
Update both
terraform_shell.goandterraform.goto use this helper.Update test to call the actual function:
-// Simulate the workdir path extraction logic from terraform_shell.go:74-77 and terraform.go:409-411. -if workdirPath, ok := tt.componentSection[provWorkdir.WorkdirPathKey].(string); ok && workdirPath != "" { - componentPath = workdirPath -} +componentPath = ExtractWorkdirPath(tt.componentSection, tt.originalPath)internal/exec/terraform.go (1)
408-411: LGTM!The workdir path override logic is correctly implemented in both init paths. The intentional duplication aligns with the existing pattern for workspace cleaning in this file. Based on learnings, this duplication is by design since the code execution paths are different.
Consider adding debug logging when the override occurs, as done in
terraform_shell.go(Line 79), for debugging consistency.Also applies to: 515-518
pkg/provisioner/workdir/fs.go (1)
126-131: Cross-platform hashing may produce different results on Windows vs Unix.The relative path written to the hash (line 131) uses OS-native path separators. On Windows, this would be backslashes; on Unix, forward slashes. The same directory content would produce different hashes across platforms.
If cross-platform hash consistency matters (e.g., for CI/CD caching or cross-platform dev), consider normalizing paths:
🔎 Suggested fix
relPath, err := filepath.Rel(path, file) if err != nil { return "", err } - hash.Write([]byte(relPath)) + // Normalize to forward slashes for cross-platform consistency. + hash.Write([]byte(filepath.ToSlash(relPath)))
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
pkg/provisioner/workdir/integration_test.gotests/cli_workdir_test.gotests/fixtures/scenarios/workdir/README.md
🧰 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 in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
🧠 Learnings (20)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.gotests/fixtures/scenarios/workdir/README.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.gotests/fixtures/scenarios/workdir/README.md
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
tests/cli_workdir_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
tests/cli_workdir_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
tests/cli_workdir_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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:
tests/cli_workdir_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
tests/cli_workdir_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
tests/cli_workdir_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
tests/cli_workdir_test.gopkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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:
pkg/provisioner/workdir/integration_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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/provisioner/workdir/integration_test.go
🧬 Code graph analysis (1)
tests/cli_workdir_test.go (1)
tests/testhelpers/atmos_runner.go (1)
NewAtmosRunner(31-35)
🪛 LanguageTool
tests/fixtures/scenarios/workdir/README.md
[typographical] ~7-~7: To join two clauses or introduce examples, consider using an em dash.
Context: ...ted - provision.workdir.enabled: true - Component workdir isolation - Multiple c...
(DASH_RULE)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (16)
tests/fixtures/scenarios/workdir/README.md (1)
1-69: Well-structured fixture documentation.The README provides clear guidance for testing the workdir provisioner with logical sections covering features, manual testing workflows, and directory structure. The prior markdown linting issue (language identifier on the tree fence) has been properly addressed, and the content aligns well with the feature's design. The manual testing steps offer a solid end-to-end walkthrough.
tests/cli_workdir_test.go (12)
1-15: Well-organized imports and test setup.Import structure follows the three-group organization (stdlib, third-party, Atmos packages) and includes appropriate testing utilities.
17-50: Solid test orchestration with proper environment isolation.Good practices:
t.Setenvfor automatic environment cleanup (per learnings)t.Chdirfor fixture directory switching (valid in atmos codebase)- Lazy atmosRunner initialization with graceful skip handling
- Sequential subtests with cleanup before execution
Note that some subtests (show/describe) depend on state from earlier tests but handle missing prerequisites gracefully with
t.Skip.
52-59: Clean and safe cleanup helper.Properly uses
t.Helper(), checks for existence before removal, and usesrequire.NoErrorfor verification.
61-74: Well-designed command runner helper.Returns stdout, stderr, and error separately for flexible test assertions. Properly uses
t.Helper()for accurate error reporting.
76-94: Functional test with pragmatic error handling.The test appropriately handles both empty output and empty JSON array scenarios. The conditional JSON validation is practical for this integration test.
96-127: Thorough integration test with proper setup.Manually creates workdir structure with metadata and source files, then verifies the list command detects it. Uses
filepath.Joinfor cross-platform compatibility and flexible assertions for output validation.
129-149: Pragmatic test with dependency handling.This test depends on workdir state from earlier tests but handles missing prerequisites gracefully with
t.Skip. This sequential approach is acceptable for integration tests that verify CLI behavior.
151-170: Consistent test pattern with prerequisite checking.Follows the same pragmatic approach as
testWorkdirShowwith graceful skipping when prerequisites aren't met. Appropriate for integration tests.
172-197: Well-isolated clean test with realistic expectations.Creates dedicated test workdir and verifies cleanup command execution. The comment acknowledging potential path resolution issues shows good awareness of integration test limitations.
199-229: Comprehensive bulk cleanup test.Creates multiple workdirs and verifies the
--allflag successfully cleans all of them. Usesrequire.NoErrorappropriately to ensure the operation succeeds and validates the success message.
231-295: Solid format validation tests.Tests all three output formats (json, yaml, table) with appropriate validation levels. JSON output is verified as parseable, while yaml/table tests confirm commands execute successfully. The deferred cleanup and use of
_for intentionally ignored values are correct.
297-334: Complete help command validation.Tests help output for the main workdir command and key subcommands, verifying that documentation mentions expected commands and flags. Appropriate use of
require.NoErrorensures help commands always succeed.pkg/provisioner/workdir/integration_test.go (3)
111-164: Clean mock-based service test.Well-structured unit test using gomock for dependency injection. The mock expectations cover the full provisioning flow (directory creation, existence check, copy, hash, metadata write), and the test verifies the workdir path is correctly recorded in
componentConfig[WorkdirPathKey].
166-216: Good coverage of cleanup operations.Both
TestCleanWorkdirandTestCleanAllWorkdirsproperly validate the cleanup APIs using real filesystem operations. The tests create actual directory structures, invoke the cleanup functions, and verify removal withos.IsNotExist. Proper use oft.TempDir()ensures test isolation.
218-220: Helpful cross-reference to related tests.Good practice documenting where the
WorkdirPathKeyextraction logic is tested. This clarifies test organization and coverage boundaries between packages.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/provisioner/workdir/integration_test.go (1)
60-61: Remove unused gomock controller.The controller is created but never used since this test exercises the real filesystem.
🔎 Proposed fix
- ctrl := gomock.NewController(t) - defer ctrl.Finish() - // Create temp directories.
🧹 Nitpick comments (10)
website/src/data/roadmap.js (1)
280-280: Consider mentioning the CLI commands.The PR introduces new CLI commands (
atmos terraform workdirwith subcommands list, describe, show, clean). For consistency with other milestones that mention their CLI commands (e.g., line 196: "atmos list stacks"), consider including this in the description or label.Example enhancement
- { label: 'Component workdir provisioning', status: 'shipped', quarter: 'q4-2025', pr: 1876, description: 'Isolated working directories for each component instance, eliminating conflicts when multiple instances share the same component source.', benefits: 'Enables concurrent component execution and just-in-time vendoring. Multiple component instances no longer conflict by overwriting each other in the same directory.' }, + { label: 'Component workdir provisioning', status: 'shipped', quarter: 'q4-2025', pr: 1876, description: 'Isolated working directories for each component instance with `atmos terraform workdir` commands to manage them—eliminating conflicts when multiple instances share the same component source.', benefits: 'Enables concurrent component execution and just-in-time vendoring. Multiple component instances no longer conflict by overwriting each other in the same directory. Use list, describe, show, and clean subcommands to inspect and manage workdirs.' },internal/exec/stack_processor_process_stacks_helpers.go (1)
74-74: Add a godoc comment for the new field.The
ComponentProvisionfield is exported but lacks documentation. Add a brief comment explaining its purpose and contents.Suggested addition
+ // ComponentProvision holds provisioning configuration for the component (e.g., workdir settings). ComponentProvision map[string]anyinternal/exec/terraform.go (2)
408-411: Consider adding debug logging for consistency.The workdir path override logic is correct, but unlike the similar block in
terraform_shell.go(lines 77-80), this doesn't log the override. Consider adding debug logging for consistency:🔎 Proposed enhancement
// Check if workdir provisioner set a workdir path - if so, use it instead of the component path. if workdirPath, ok := info.ComponentSection[provWorkdir.WorkdirPathKey].(string); ok && workdirPath != "" { componentPath = workdirPath + log.Debug("Using workdir path after provisioner execution", "workdirPath", workdirPath) }
515-518: Same suggestion: add debug logging for consistency.This override block in the init case follows the same pattern as the one above. The same logging suggestion applies here for consistency with
terraform_shell.go.🔎 Proposed enhancement
// Check if workdir provisioner set a workdir path - if so, use it instead of the component path. if workdirPath, ok := info.ComponentSection[provWorkdir.WorkdirPathKey].(string); ok && workdirPath != "" { componentPath = workdirPath + log.Debug("Using workdir path in init case", "workdirPath", workdirPath) }cmd/terraform/workdir/workdir.go (2)
15-19: Missing perf.Track call in SetAtmosConfig.Per coding guidelines, all public functions should include
defer perf.Track(...)(). Since this is a simple setter andatmosConfigPtrisn't available yet, you could usenilfor the config parameter.🔎 Suggested fix
+import ( + "github.com/cloudposse/atmos/pkg/perf" + ... +) // SetAtmosConfig sets the Atmos configuration for the workdir command. // This is called from root.go after atmosConfig is initialized. func SetAtmosConfig(config *schema.AtmosConfiguration) { + defer perf.Track(nil, "workdir.SetAtmosConfig")() + atmosConfigPtr = config }
36-39: Missing perf.Track call in GetWorkdirCommand.Same as above—public function should have performance tracking.
🔎 Suggested fix
// GetWorkdirCommand returns the workdir command for parent registration. func GetWorkdirCommand() *cobra.Command { + defer perf.Track(nil, "workdir.GetWorkdirCommand")() + return workdirCmd }cmd/terraform/workdir/workdir_show.go (1)
100-107: Consider extracting the StyleFunc for reuse.This styling pattern (cyan first column, padding) appears in multiple workdir commands. Could be a shared helper, but that's optional polish.
cmd/terraform/workdir/workdir_describe_test.go (1)
24-32: Consider expanding minimal structural tests.These tests only assert non-nil but don't validate actual behavior. For instance,
TestDescribeCmd_Argscould verifycobra.ExactArgs(1)is configured (which you already do inTestDescribeCmd_ArgsValidation). These are fine as-is since they provide basic smoke coverage.pkg/provisioner/workdir/clean.go (1)
94-102: Silent no-op when no options provided.When
opts.Allis false andopts.Componentis empty, the function returnsnilwithout any action or feedback. This is reasonable behavior, though a debug log might help troubleshooting.pkg/provisioner/workdir/fs.go (1)
46-52: Minor edge case in Exists.
Existsreturnsfalsefor any error, not just "not exists". Permission errors would also returnfalse. This is typically fine for the workdir use case, but worth noting.
|
Warning Release Documentation RequiredThis PR is labeled
|
|
These changes were released in v1.203.0-rc.3. |
|
Closes #225 |
2 participants
what
WorkdirProvisioneratmos terraform workdirCLI commands: list, describe, show, cleanwhy
Multiple component instances targeting the same component caused conflicts due to shared working directories. Workdir provisioning isolates each component execution to a dedicated directory (
.workdir/terraform/<stack>-<component>/), enabling parallel execution and preventing state/artifact interference.references
Summary by CodeRabbit
New Features
Documentation
Schema
Tests
✏️ Tip: You can customize this high-level summary in your review settings.