fix: Workflow command parsing for quoted arguments #1961
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
|
@coderabbitai full review please |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR updates four Go dependencies to patch versions, replaces the basic string parsing method with a shell-aware argument parser in the atmos command execution path with fallback logic, and adds comprehensive test coverage across unit tests, integration tests, and test fixtures to validate proper quoted argument handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✏️ Tip: You can disable this entire section by setting 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1961 +/- ##
==========================================
+ Coverage 74.31% 74.33% +0.01%
==========================================
Files 774 774
Lines 70114 70117 +3
==========================================
+ Hits 52103 52118 +15
+ Misses 14588 14579 -9
+ Partials 3423 3420 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR updates Go dependency versions in NOTICE and go.mod, replaces basic argument parsing with shell-aware parsing in workflow utilities using mvdan.cc/sh/v3/shell.Fields with fallback support, and adds comprehensive test coverage for workflow parsing and execution across multiple scenarios including quoted arguments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@NOTICE`:
- Line 298: The NOTICE file is out of date and must be regenerated; run the
programmatic generator script locally by executing ./scripts/generate-notice.sh,
verify the updated NOTICE changes, stage and commit the updated NOTICE file, and
push the commit so CI picks up the regenerated NOTICE (ensure you include any
submodule or vendored license updates like the referenced renameio/LICENSE if
the script preserves those).
In `@tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml`:
- Around line 109-111: Update the placeholder issue reference in the comment
that currently shows `#XXXX` (the third comment line in the test YAML block
describing the -var flag parsing issue) by replacing `#XXXX` with the actual
GitHub issue number if one exists, or remove the
`https://github.com/cloudposse/atmos/issues/XXXX` line entirely if no specific
issue should be referenced.
🧹 Nitpick comments (1)
internal/exec/workflow_utils_test.go (1)
977-1030: Test name suggests fallback testing but only tests successful parsing.The test is named
TestShellFieldsFallbackbut all cases haveshouldParse: true, meaning the fallback tostrings.Fieldsisn't actually exercised here. The else branch (lines 1026-1027) is dead code in the current test configuration.Consider either:
- Renaming to
TestShellFieldsParsingto match actual behavior- Adding a test case that triggers a parse failure to actually test the fallback
The actual fallback logic in
workflow_utils.gohandles this at runtime, but verifying it with a test would strengthen coverage.
📜 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 ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
NOTICEgo.modinternal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gotests/fixtures/scenarios/workflows/stacks/workflows/test.yamltests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_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 fixturesPrefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with
go.uber.org/mock/mockgen, use table-driven tests, target >80% coverage
Files:
internal/exec/workflow_test.gointernal/exec/workflow_utils_test.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.go
{go.mod,go.sum}
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Files:
go.mod
tests/snapshots/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit golden snapshot files - always use
-regenerate-snapshotsflag when updating test output
Files:
tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
🧠 Learnings (38)
📓 Common learnings
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
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.
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 {go.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
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 **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Ensure all tests pass, verify code coverage meets targets, run golangci-lint and fix any issues, and update documentation before submitting pull requests
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
📚 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:
internal/exec/workflow_test.gointernal/exec/workflow_utils_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:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gotests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 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:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gotests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
internal/exec/workflow_test.gointernal/exec/workflow_utils.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:
internal/exec/workflow_test.gointernal/exec/workflow_utils_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 cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
internal/exec/workflow_test.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
internal/exec/workflow_test.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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/workflow_test.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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/workflow_test.gogo.mod
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 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:
internal/exec/workflow_test.gointernal/exec/workflow_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/workflow_test.gointernal/exec/workflow_utils.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 **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
internal/exec/workflow_test.gogo.modNOTICE
📚 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:
internal/exec/workflow_test.gointernal/exec/workflow_utils_test.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gotests/fixtures/scenarios/workflows/stacks/workflows/test.yamltests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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/workflow_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:
internal/exec/workflow_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 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:
internal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gotests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 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/workflow_utils.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 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:
internal/exec/workflow_utils.go
📚 Learning: 2024-10-21T17:51:07.087Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:07.087Z
Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 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/workflows/stacks/workflows/test.yamltests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 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:
internal/exec/workflow_utils_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:
internal/exec/workflow_utils_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:
internal/exec/workflow_utils_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:
internal/exec/workflow_utils_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 {go.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: Go version 1.23.0 was deliberately introduced by the maintainer (aknysh) in January 2025. While this might be a pre-release or development version of Go, it has been approved for use in this project.
Applied to files:
go.mod
📚 Learning: 2025-10-11T04:03:41.651Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1611
File: go.mod:3-3
Timestamp: 2025-10-11T04:03:41.651Z
Learning: The Go module "go" directive supports three-part version numbers (e.g., "go 1.24.8") since Go 1.21+. The format "go 1.N.P" is valid and recommended for specifying patch versions. Earlier Go versions only accepted major.minor format, but modern tooling fully supports the patch component.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: The project uses Go version 1.23.0 which has been confirmed by the maintainer to be working in production for months. Do not flag this as an invalid Go version.
Applied to files:
go.mod
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2025-10-11T19:12:38.832Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden:0-0
Timestamp: 2025-10-11T19:12:38.832Z
Learning: Usage Examples sections in error output are appropriate for command usage errors (incorrect syntax, missing arguments, invalid flags) but not for configuration validation errors (malformed workflow files, invalid settings in atmos.yaml). Configuration errors should focus on explaining what's wrong with the config, not command usage patterns.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 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:
tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2025-02-14T23:12:38.030Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 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_workflow_not_found.stderr.golden
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden
📚 Learning: 2025-11-01T20:24:29.557Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1714
File: NOTICE:0-0
Timestamp: 2025-11-01T20:24:29.557Z
Learning: In the cloudposse/atmos repository, the NOTICE file is programmatically generated and should not be manually edited. Issues with dependency license URLs in NOTICE will be resolved when upstream package metadata is corrected.
Applied to files:
NOTICE
🧬 Code graph analysis (2)
internal/exec/workflow_utils.go (1)
pkg/logger/log.go (1)
Debug(24-26)
internal/exec/workflow_utils_test.go (3)
pkg/schema/schema.go (1)
ConfigAndStacksInfo(729-826)pkg/schema/workflow.go (2)
WorkflowDefinition(17-24)WorkflowStep(7-15)internal/exec/workflow_utils.go (1)
ExecuteWorkflow(231-401)
🪛 GitHub Actions: Dependency Review
NOTICE
[error] 1-1: NOTICE file is out of date. Run './scripts/generate-notice.sh' locally and commit the changes.
⏰ 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 (15)
go.mod (1)
54-86: LGTM on dependency updates.Version bumps for
mapstructure,go-yaml,renameio, andopaalign with the PR's scope. Themvdan.cc/sh/v3dependency was already present, so no new dependencies introduced.tests/snapshots/TestCLICommands_atmos_workflow_not_found.stderr.golden (1)
24-30: Snapshot reflects new test workflows.New hints for
shell-var-echoandterraform-var-*variants match the test fixtures added for quoted argument parsing validation. Consistent with the PR scope.internal/exec/workflow_utils.go (2)
16-16: Import correctly placed.The
mvdan.cc/sh/v3/shellimport is properly grouped with third-party packages.
336-342: Solid fix for quoted argument parsing.Using
shell.Fieldscorrectly handles shell-style quoting, so-var="enabled=false"becomes-var=enabled=falseas expected. The fallback tostrings.Fieldswith debug logging maintains backward compatibility for edge cases. Passingnilas the second argument disables variable expansion, which is the right choice here.internal/exec/workflow_test.go (2)
11-11: Import appropriately added.Third-party import for
shellpackage correctly grouped.
244-312: Comprehensive test coverage for the parsing fix.Table-driven tests cover the key scenarios: double/single quotes, multiple
-varflags, spaces in values, equals signs in values, and complex flag combinations. Good documentation of the fix in the function comment.tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml (1)
113-158: Good coverage for the -var flag parsing scenarios.These workflow definitions comprehensively test the shell.Fields parsing behavior with:
- Double-quoted values
- Single-quoted values
- Multiple -var flags with mixed quote styles
- Values containing spaces
- Values containing equals signs
The shell-var-echo workflow at the end is a nice addition for verifying the actual parsed output.
internal/exec/workflow_utils_test.go (8)
938-975: LGTM!These tests properly verify that global environment variables are included in step environments. The iteration approach to check for specific env var entries is clear and aligns with how
prepareStepEnvironmentreturns the environment as a slice. Good coverage for both populated and empty global env scenarios.
1032-1083: LGTM!Good tests for stack precedence. Using shell type steps avoids unnecessary terraform execution while still exercising the stack resolution logic. The step-level stack test correctly verifies that step stack overrides workflow-level stack.
1085-1135: LGTM!Dry run tests for both shell and atmos command types are well-structured. These ensure the
dryRunflag properly prevents actual execution.
1137-1171: LGTM!The from-step test correctly verifies that execution can start from an arbitrary step. The comment clearly documents the expected behavior (skip step1).
1173-1201: LGTM, though output verification could strengthen confidence.This test exercises the quoted var flag parsing path which is central to this PR. The test verifies no error occurs during execution. For stronger assertions, you could capture stdout and verify the parsed arguments don't contain literal quotes, but the current level of coverage is reasonable.
1203-1231: LGTM!Good test for identity precedence. The logic correctly verifies that step-level identity takes priority over command-line identity, matching the behavior in
ExecuteWorkflowwhere command-line identity is only used when step identity is empty.
1233-1270: LGTM!Solid test for mixed step types. Using
versionfor the atmos step keeps execution lightweight while still exercising the step type switching logic.
1272-1338: LGTM!Good tests for command-line stack precedence and auto-generated step names. The assertion after
ExecuteWorkflowinTestExecuteWorkflow_AutoGeneratedStepNamescorrectly verifies thatcheckAndGenerateWorkflowStepNamespopulates missing names during execution.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add tests that exercise the fallback code path when shell.Fields fails to parse malformed commands (e.g., unclosed quotes). - TestShellFieldsParseErrors: Verifies shell.Fields fails on various malformed shell syntax - TestExecuteWorkflow_ShellFieldsFallbackWithMalformedCommand: Exercises the actual fallback path in ExecuteWorkflow Co-Authored-By: Claude <[email protected]>
what
-var="key=value"strings.Fields()was used which doesn't handle shell quoting, causing quoted arguments to be passed with literal quote charactersshell.Fields()frommvdan.cc/sh/v3/shellwhich properly parses shell-style quotingwhy
-var="access_roles_enabled=false"were failing with:strings.Fields()splits on whitespace but preserves quote characters, so-var="enabled=false"was passed to Terraform with the literal quotes includedshell.Fields()correctly parses shell quoting and strips quotes, matching the behavior users expectExample
Before (broken):
After (fixed):
Testing
Added comprehensive tests for the fix:
Unit Tests (
internal/exec/workflow_test.go)TestShellFieldsQuoteParsing- verifiesshell.Fields()correctly parses various quoted argument patternsUnit Tests (
internal/exec/workflow_utils_test.go)TestShellFieldsFallback- tests shell parsing with various argument patternsTestExecuteWorkflow_WithQuotedVarFlag- tests-var="key=value"pattern in workflowsTestPrepareStepEnvironment_WithGlobalEnv- tests global env mergingTestExecuteWorkflow_*- multiple tests for stack overrides, dry run, from-step, identity precedenceWorkflow Test Fixtures (
tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml)terraform-var-double-quotes--var="key=value"patternterraform-var-single-quotes--var='key=value'patternterraform-var-multiple- multiple var flagsterraform-var-with-spaces- values containing spacesterraform-var-equals-in-value- values containing=references
pkg/workflow/executor.go:332-337which already had the correct implementationmvdan.cc/sh/v3/shellis already a dependency used in other parts of the codebaseSummary by CodeRabbit
Release Notes
Bug Fixes
Tests
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.