Refactor Execution YAML Function Processing, Support !env, !exec,!include,!repo-root directive in configuration files #1202
Conversation
…y into the config loading process.
📝 WalkthroughWalkthroughThis update refactors how YAML function tags for shell execution and environment variable interpolation are handled. Shell command execution logic is moved from internal packages to shared utilities, consolidating related code and improving error handling. Constants for YAML function tags are relocated, and related imports are updated. Logging is standardized using a new logger. The test suite is expanded with a new case for shell command execution in YAML. Unused or redundant code is removed, and function signatures are updated to return errors instead of terminating the process directly. Changes
Sequence Diagram(s)sequenceDiagram
participant YAML_Config
participant Utils
participant Shell
participant Logger
YAML_Config->>Utils: ProcessTagExec(input)
Utils->>Utils: getStringAfterTag(input, AtmosYamlFuncExec)
Utils->>Logger: Log execution
Utils->>Utils: ExecuteShellAndReturnOutput(command, ...)
Utils->>Utils: GetNextShellLevel()
Utils->>Shell: ShellRunner(command, ...)
Shell-->>Utils: Output or error
Utils->>Utils: Try to unmarshal output as JSON
Utils-->>YAML_Config: Return result (JSON or string)
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
pkg/utils/yaml_func_env.go (1)
55-65: New helper function with proper error handlingThe extraction of the tag processing into its own helper function is a good refactoring practice. However, the static analysis tool flagged the dynamic error on line 60.
Consider using wrapped static errors instead:
- err := fmt.Errorf("invalid Atmos YAML function: %s", input) + var ErrInvalidYamlFunction = fmt.Errorf("invalid Atmos YAML function") + err := fmt.Errorf("%w: %s", ErrInvalidYamlFunction, input)Or alternatively, declare the error at package level:
// At package level +var ErrInvalidYamlFunction = fmt.Errorf("invalid Atmos YAML function") // In the function - err := fmt.Errorf("invalid Atmos YAML function: %s", input) + err := fmt.Errorf("%w: %s", ErrInvalidYamlFunction, input)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 60-60:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid Atmos YAML function: %s", input)"pkg/utils/shell_utils.go (2)
9-10: Good constant definition with documentationThe MaxShellDepth constant is well-defined with a clear comment. However, the static analysis tool suggests the comment should end with a period.
-// MaxShellDepth is the maximum number of nested shell commands that can be executed +// MaxShellDepth is the maximum number of nested shell commands that can be executed.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 9-9:
Comment should end in a period
12-13: Function comment needs a periodSimilar to the constant comment, this function comment should end with a period.
-// getNextShellLevel increments the ATMOS_SHLVL and returns the new value or an error if maximum depth is exceeded +// GetNextShellLevel increments the ATMOS_SHLVL and returns the new value or an error if maximum depth is exceeded.Also note that the comment starts with
getNextShellLevelbut the function is namedGetNextShellLevel.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 12-12:
Comment should end in a periodinternal/exec/yaml_func_utils.go (1)
7-7: Import statement needs aliasAccording to the static analysis tool, this import requires an alias to match project conventions.
-import ( - "fmt" - "strings" - - "github.com/charmbracelet/log" +import ( + "fmt" + "strings" + + log "github.com/charmbracelet/log"🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 7-7: import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
(importas)
🪛 GitHub Check: golangci-lint
[failure] 7-7:
import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to configpkg/utils/yaml_func_exec.go (2)
40-40: Fix comment formatAdd a period at the end of the comment to follow Go style conventions.
-// ExecuteShellAndReturnOutput runs a shell script and capture its standard output +// ExecuteShellAndReturnOutput runs a shell script and captures its standard output.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 40-40:
Comment should end in a period
71-71: Fix comment formatAdd a period at the end of the comment to follow Go style conventions.
-// shellRunner uses mvdan.cc/sh/v3's parser and interpreter to run a shell script and divert its stdout +// shellRunner uses mvdan.cc/sh/v3's parser and interpreter to run a shell script and divert its stdout.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 71-71:
Comment should end in a period
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/cmd_utils.go(1 hunks)internal/exec/shell_utils.go(3 hunks)internal/exec/yaml_func_exec.go(0 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(1 hunks)internal/exec/yaml_func_utils.go(2 hunks)pkg/config/config_test.go(2 hunks)pkg/config/const.go(0 hunks)pkg/config/load.go(2 hunks)pkg/utils/shell_utils.go(1 hunks)pkg/utils/yaml_func_env.go(2 hunks)pkg/utils/yaml_func_exec.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/config/const.go
- internal/exec/yaml_func_exec.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
internal/exec/yaml_func_template.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncTemplate(21-21)
internal/exec/shell_utils.go (1)
pkg/utils/shell_utils.go (1)
GetNextShellLevel(13-31)
internal/exec/yaml_func_terraform_output.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncTerraformOutput(22-22)
pkg/utils/yaml_func_env.go (3)
pkg/utils/log_utils.go (1)
LogTrace(74-76)pkg/utils/yaml_utils.go (1)
AtmosYamlFuncEnv(23-23)pkg/utils/string_utils.go (1)
SplitStringByDelimiter(24-34)
pkg/config/config_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(13-45)
pkg/utils/yaml_func_exec.go (3)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)pkg/utils/shell_utils.go (1)
GetNextShellLevel(13-31)pkg/utils/log_utils.go (1)
LogDebug(80-82)
internal/exec/yaml_func_utils.go (2)
pkg/utils/yaml_func_exec.go (1)
ProcessTagExec(18-38)pkg/utils/yaml_func_env.go (1)
ProcessTagEnv(9-53)
🪛 GitHub Check: golangci-lint
pkg/config/load.go
[failure] 404-404:
deep-exit: calls to log.Fatal only in main() or init() functions
pkg/utils/shell_utils.go
[failure] 9-9:
Comment should end in a period
[failure] 12-12:
Comment should end in a period
[failure] 14-14:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 19-19:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
[failure] 27-27:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
pkg/utils/yaml_func_env.go
[failure] 60-60:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid Atmos YAML function: %s", input)"
pkg/utils/yaml_func_exec.go
[failure] 24-24:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 29-29:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 40-40:
Comment should end in a period
[failure] 54-54:
appendAssign: append result not assigned to the same slice
[failure] 71-71:
Comment should end in a period
internal/exec/yaml_func_utils.go
[failure] 7-7:
import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
[failure] 79-79:
deep-exit: calls to log.Fatal only in main() or init() functions
🪛 golangci-lint (1.64.8)
internal/exec/yaml_func_utils.go
[error] 7-7: import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
(importas)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (18)
pkg/utils/yaml_func_env.go (5)
9-11: Function signature properly exported and return type improvedGreat job exporting this function to make it accessible from other packages. The return type change from
anyto(string, error)is a solid improvement that allows for better error handling by callers.
12-12: Logging approach seems consistentThe direct use of
LogTraceis consistent with the function's new location in the utils package.
14-17: Good error handling patternThe error handling pattern here is appropriate - forwarding errors to the caller rather than exiting directly.
23-40: Error handling consistent throughoutThe consistent error handling pattern continues throughout the function. This is a good refactoring that improves the function's reusability.
45-53: Return values simplified and standardizedThe function now consistently returns appropriate values and errors, which matches modern Go idioms for error handling.
internal/exec/yaml_func_template.go (1)
18-18: Updated function call to use the utils packageThe change from using the config package to the utils package is consistent with the refactoring described in the PR objectives.
pkg/utils/shell_utils.go (2)
24-31: Good shell level managementThe function properly increments the shell level and checks against the maximum depth. The early exit with error on exceeding MaxShellDepth prevents potential infinite recursion.
However, the static analysis tool flagged dynamic errors at lines 19 and 27. Consider defining errors at package level:
// At package level +var ( + ErrInvalidShellLevelValue = fmt.Errorf("invalid ATMOS_SHLVL value") + ErrMaxShellDepthExceeded = fmt.Errorf("ATMOS_SHLVL exceeds maximum allowed depth") +) // In the function - return 0, fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl) + return 0, fmt.Errorf("%w: %s", ErrInvalidShellLevelValue, atmosShellLvl) - return 0, fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?", - shellVal, MaxShellDepth) + return 0, fmt.Errorf("%w (%d > %d). Infinite recursion?", + ErrMaxShellDepthExceeded, shellVal, MaxShellDepth)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 27-27:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
14-22:✅ Verification successful
Environment variable handling
The static analysis tool suggests using
viper.BindEnvinstead ofos.Getenv. However, this might be a direct access requirement given the nature of the function.
🏁 Script executed:
#!/bin/bash # Check if viper is used elsewhere for environment variables grep -r "viper.BindEnv" --include="*.go" .Length of output: 42
Environment Variable Access Is Correct as Implemented
After verifying the codebase, there’s no evidence of
viper.BindEnvusage elsewhere, and the direct access to the environment usingos.Getenvinpkg/utils/shell_utils.gois intentional given the function’s requirements.
- The static analysis suggestion to use
viper.BindEnvdoesn't align with the current project configuration.- Direct access appears to be the deliberate design choice for handling
ATMOS_SHLVL.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 14-14:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 19-19:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"internal/exec/yaml_func_terraform_output.go (1)
18-18: Updated function call to use the utils packageSimilar to the changes in yaml_func_template.go, this change is consistent with centralizing YAML function constants in the utils package.
cmd/cmd_utils.go (1)
403-403: Function signature change for ExecuteShellAndReturnOutputThe function call has been updated to use the utility package implementation (
u.ExecuteShellAndReturnOutput) without passing theatmosConfigparameter that was previously required.This change aligns with the PR objective of relocating YAML function processing from internal/exec to pkg/utils for shared functionality.
pkg/config/load.go (2)
396-396: Added support for !exec directiveThe
allowedDirectivesnow includes bothu.AtmosYamlFuncEnvandu.AtmosYamlFuncExec, enabling the configuration processing to handle both directives as stated in the PR objectives.
411-417: Added !exec directive supportImplementation for the
!execdirective has been added, fulfilling the PR objective to support dynamic configuration capabilities likeGITHUB_TOKEN: !exec gh auth token.internal/exec/yaml_func_utils.go (1)
71-71: Updated to use utility package implementationReplaced local tag processing function with the shared utility implementation, which simplifies the code and removes duplication as part of the refactoring.
internal/exec/shell_utils.go (3)
34-34: Updated to use utility package implementationThe code now uses
u.GetNextShellLevel()from the utils package instead of a local implementation, which helps eliminate code duplication and provides a centralized place for shell level management.
92-92: Updated to use utility package implementationConsistent use of
u.GetNextShellLevel()from the utils package for shell level management in theExecuteShellfunction.
119-119: Updated to use utility package implementationConsistent use of
u.GetNextShellLevel()from the utils package for shell level management in theExecuteShellAndReturnOutputfunction.pkg/config/config_test.go (2)
148-148: Clear test case description updateGood job renaming the test case to explicitly mention the YAML function env tag. This improves clarity and aligns with the new test case.
164-176: Good test coverage for new YAML function exec featureThe new test case properly verifies the shell command execution functionality in YAML configurations. Great attention to detail with the newline assertion on line 174.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1202 +/- ##
==========================================
+ Coverage 31.31% 31.43% +0.12%
==========================================
Files 206 208 +2
Lines 23044 23189 +145
==========================================
+ Hits 7216 7290 +74
- Misses 14747 14818 +71
Partials 1081 1081
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/exec/yaml_func_utils.go (1)
77-81:⚠️ Potential issueAvoid using
log.Fatalin a library function.Calling
log.Fatalabruptly ends the process and hinders graceful error handling for calling code. Returning the error is preferable.Consider applying this change:
- res, err := u.ProcessTagEnv(input) - if err != nil { - log.Fatal(err) - } - return res + res, err := u.ProcessTagEnv(input) + if err != nil { + return nil // or another sensible fallback + } + return res🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 79-79:
deep-exit: calls to log.Fatal only in main() or init() functionspkg/utils/yaml_func_exec.go (1)
28-29:⚠️ Potential issueUse error returns instead of
log.Fatal.Library code shouldn’t terminate the entire process. Return the error so callers can handle it gracefully.
- if err != nil { - log.Fatal(err) - } ... - if err != nil { - log.Fatal(err) - } + if err != nil { + return err // or wrap the error 8000 + }Also applies to: 33-34
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 28-28:
deep-exit: calls to log.Fatal only in main() or init() functions
🧹 Nitpick comments (3)
internal/exec/shell_utils.go (2)
61-61: Repeated debug logs.Multiple lines output “Executing” logs. Consider extracting this into a helper if further logging expansions are planned.
Also applies to: 67-67, 89-89, 91-91
178-182: Minor key mismatch: “workingDirector”.Consider renaming the key “workingDirector” to “workingDirectory” to avoid confusion:
- "workingDirector", workingDir, + "workingDirectory", workingDir,pkg/utils/yaml_func_exec.go (1)
102-102: Consider static error patterns.The project’s linter warns about dynamic error construction. Define these as well-known constants or wrap static errors to maintain consistency.
Also applies to: 110-110
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 102-102:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exec/shell_utils.go(10 hunks)internal/exec/yaml_func_utils.go(2 hunks)pkg/utils/yaml_func_exec.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/exec/yaml_func_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🧬 Code Graph Analysis (3)
internal/exec/shell_utils.go (1)
pkg/utils/yaml_func_exec.go (2)
GetNextShellLevel(96-114)ShellRunner(75-93)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
internal/exec/yaml_func_utils.go (3)
pkg/utils/yaml_func_exec.go (1)
ProcessTagExec(22-42)pkg/utils/yaml_utils.go (3)
AtmosYamlFuncStore(20-20)AtmosYamlFuncTerraformOutput(22-22)AtmosYamlFuncEnv(23-23)pkg/utils/yaml_func_env.go (1)
ProcessTagEnv(9-53)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 28-28:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 33-33:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 58-58:
appendAssign: append result not assigned to the same slice
[failure] 97-97:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 102-102:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
[failure] 110-110:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
internal/exec/yaml_func_utils.go
[failure] 79-79:
deep-exit: calls to log.Fatal only in main() or init() functions
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (14)
internal/exec/yaml_func_utils.go (2)
7-7: Good import alias usage for "github.com/charmbracelet/log".This aligns with the project's convention of importing this package as
log.
71-71: Proper delegation to new utility function.Replacing any local logic with
u.ProcessTagExec()promotes a consistent, reusable approach for YAML “exec” directives.internal/exec/shell_utils.go (11)
14-14: Consistency with package alias "log".This import alias is correctly following the project's convention.
19-19: Helpful documentation comment.This brief comment clearly describes the function’s purpose.
29-29: Good call to shared shell level logic.Using
u.GetNextShellLevel()centralizes recursion depth checks and prevents code duplication.
54-54: Non-blocking warning log.Logging a warning and returning an error provides a balanced approach for error reporting in library code.
85-85: Centralized shell level retrieval.Consistent usage of
GetNextShellLevel()enhances maintainability.
97-97: Delegates command execution to the new utility.Allows
u.ShellRunner()to handle the entire command lifecycle.
132-133: Graceful fallback on parse failure.Logging and returning without error is acceptable within a deferred context when
ATMOS_SHLVLis non-critical.
141-142: Non-blocking warning log for environment update.This warns the user but doesn't halt execution, maintaining user workflow continuity.
184-184: Informative debug statement.Helps users track environment variables being set in the shell.
246-246: Helpful exit log.Communicates when the interactive shell has been terminated.
291-291: Transparent environment logging.Showing each variable’s name and value supports better debugging.
pkg/utils/yaml_func_exec.go (1)
58-58: Reassigned environment slice.Appending to
envin a new variableupdatedEnvis acceptable. The linter’s note can be safely ignored if you need a separate variable.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 58-58:
appendAssign: append result not assigned to the same slice
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exec/yaml_func_utils.go (1)
81-85:⚠️ Potential issueSame termination issue as above.
Repeats thelog.Fatalcall. Please consider returning the error or logging at a suitable level and proceeding.-case strings.HasPrefix(input, u.AtmosYamlFuncEnv) && !skipFunc(skip, u.AtmosYamlFuncEnv): - res, err := u.ProcessTagEnv(input) - if err != nil { - log.Fatal(err) - } - return res +case strings.HasPrefix(input, u.AtmosYamlFuncEnv) && !skipFunc(skip, u.AtmosYamlFuncEnv): + res, err := u.ProcessTagEnv(input) + if err != nil { + log.Error("Failed to process env tag", "error", err) + return nil + } + return res🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 83-83:
deep-exit: calls to log.Fatal only in main() or init() functions
🧹 Nitpick comments (4)
pkg/config/load.go (1)
401-406: Consider surfacing errors instead of continuing silently.
Note that ifProcessTagEnvfails, the code logs at debug level and then continues. This might hide issues from users.pkg/utils/yaml_func_exec.go (3)
19-20: Maximum shell depth constant.
Consider making this limit configurable if user scenarios require different recursion depths.
74-93: ShellRunner uses context.TODO.
Providing a user-supplied context or a timeout could help manage long-running commands more gracefully.
95-114: Useos.Getenvcarefully.
Project guidelines preferviper.BindEnvfor environment variables, though this is specialized logic. If uniform environment retrieval is desired, consider migrating to viper-based lookups.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 97-97:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 102-102:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
[failure] 110-110:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exec/yaml_func_utils.go(2 hunks)pkg/config/load.go(2 hunks)pkg/utils/yaml_func_exec.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/exec/yaml_func_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🧬 Code Graph Analysis (2)
pkg/config/load.go (3)
pkg/utils/yaml_utils.go (2)
AtmosYamlFuncEnv(23-23)AtmosYamlFuncExec(19-19)pkg/utils/yaml_func_env.go (1)
ProcessTagEnv(9-53)pkg/utils/yaml_func_exec.go (1)
ProcessTagExec(22-42)
internal/exec/yaml_func_utils.go (3)
pkg/utils/yaml_func_exec.go (1)
ProcessTagExec(22-42)pkg/utils/yaml_utils.go (3)
AtmosYamlFuncStore(20-20)AtmosYamlFuncTerraformOutput(22-22)AtmosYamlFuncEnv(23-23)pkg/utils/yaml_func_env.go (1)
ProcessTagEnv(9-53)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 58-58:
appendAssign: append result not assigned to the same slice
[failure] 97-97:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 102-102:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
[failure] 110-110:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
internal/exec/yaml_func_utils.go
[failure] 73-73:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 83-83:
deep-exit: calls to log.Fatal only in main() or init() functions
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
internal/exec/yaml_func_utils.go (1)
7-7: Import alias usage confirmed.
The aliaslogforgithub.com/charmbracelet/logaligns with the project's style guidelines.pkg/config/load.go (2)
15-15: Consistent import alias usage.
Usingufor the utils package matches existing patterns in the codebase.
396-396: Newly introduced allowedDirectives for YAML tags.
This approach is clean and extensible. No concerns.pkg/utils/yaml_func_exec.go (4)
1-2: Package and file structure created.
This introduction of a dedicated file for YAML exec functionality is organized and transparent.
3-17: Imports look good.
The chosen libraries andlogalias usage follow the project conventions.
22-42: Flexible JSON decoding.
The approach of returninganyas decoded result is flexible. Ensure callers handle complex data types as needed.
44-72: Appended environment variables.
Appending toupdatedEnvis valid, but ensure consistent usage throughout. The overall function logic is straightforward.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 58-58:
appendAssign: append result not assigned to the same slice
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/utils/yaml_func_exec.go (1)
61-61: 🛠️ Refactor suggestionAssign the result of
appendback to the original slice.The result of
appendshould be assigned toenv(notupdatedEnv) to avoid confusion and potential bugs. This is a common Go pitfall.Here's a quick fix:
- updatedEnv := append(env, fmt.Sprintf("ATMOS_SHLVL=%d", newShellLevel)) + env = append(env, fmt.Sprintf("ATMOS_SHLVL=%d", newShellLevel))Then use
envin the call toShellRunner.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 61-61:
appendAssign: append result not assigned to the same slice
🧹 Nitpick comments (3)
pkg/utils/yaml_func_exec.go (3)
100-100: Preferviper.BindEnvover directos.Getenvfor new environment variables.Project guidelines recommend using
viper.BindEnvfor environment variable management. This makes configuration more flexible and testable.If possible, refactor to use
viperforATMOS_SHLVLretrieval.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 100-100:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
105-105: Use wrapped static errors instead of dynamic errors.Instead of dynamically creating errors with
fmt.Errorf, define static error variables and wrap them as needed. This helps with error comparison and handling.You could define:
var ErrInvalidShellLevel = errors.New("invalid ATMOS_SHLVL value")And then:
return 0, fmt.Errorf("%w: %s", ErrInvalidShellLevel, atmosShellLvl)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
77-96: Shell execution is well-contained, but consider context propagation.Currently,
context.TODO()is used. If you want to support timeouts or cancellation, consider passing a context from the caller.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/utils/yaml_func_exec.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 61-61:
appendAssign: append result not assigned to the same slice
[failure] 100-100:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/utils/yaml_func_exec.go (3)
25-45: Solid error handling and clear separation of concerns.The function cleanly extracts, executes, and decodes YAML exec tags, returning errors instead of terminating the process. This is a good move for library code.
47-75: Good encapsulation of shell execution with output capture.The function is straightforward and handles dry-run logic well. Consider adding a timeout/context to avoid hanging on long-running commands, but otherwise, this is tidy.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 61-61:
appendAssign: append result not assigned to the same slice
98-116: Recursion guard is clear and effective.The shell level increment and guard against runaway recursion is well-implemented. Just a reminder: static error wrapping (see above) would make this even more robust.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 100-100:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)"
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/utils/yaml_func_exec.go (2)
100-100:⚠️ Potential issueUse viper.BindEnv instead of os.Getenv
Per project guidelines, use
viper.BindEnvfor environment variables instead ofos.Getenv.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 100-100:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
47-75: 🛠️ Refactor suggestionConsider adding timeout mechanism
The shell command execution doesn't have a timeout, which could lead to hung processes if a command never completes.
Consider implementing a context with timeout:
func ExecuteShellAndReturnOutput( command string, name string, dir string, env []string, dryRun bool, + timeout time.Duration, ) (string, error) { var b bytes.Buffer newShellLevel, err := GetNextShellLevel() if err != nil { return "", err } env = append(env, fmt.Sprintf("ATMOS_SHLVL=%d", newShellLevel)) log.Debug("Executing", "command", command) if dryRun { return "", nil } + ctx := context.Background() + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(context.Background(), timeout) + defer cancel() + } - err = ShellRunner(command, name, dir, env, &b) + err = ShellRunner(command, name, dir, env, &b, ctx) if err != nil { return "", err } return b.String(), nil }
🧹 Nitpick comments (4)
pkg/utils/yaml_func_exec.go (4)
22-23: Add period to commentComment should end with a period for consistency with other code comments in the codebase.
-// MaxShellDepth is the maximum number of nested shell commands that can be executed . +// MaxShellDepth is the maximum number of nested shell commands that can be executed.
47-48: Fix comment formattingRemove extra space before the period for consistency with Go documentation standards.
-// ExecuteShellAndReturnOutput runs a shell script and capture its standard output . +// ExecuteShellAndReturnOutput runs a shell script and capture its standard output.
77-78: Fix comment formattingRemove extra space before the period for consistency with Go documentation standards.
-// ShellRunner uses mvdan.cc/sh/v3's parser and interpreter to run a shell script and divert its stdout . +// ShellRunner uses mvdan.cc/sh/v3's parser and interpreter to run a shell script and divert its stdout.
98-99: Fix comment formattingRemove extra space before the period for consistency with Go documentation standards.
-// getNextShellLevel increments the ATMOS_SHLVL and returns the new value or an error if maximum depth is exceeded . +// GetNextShellLevel increments the ATMOS_SHLVL and returns the new value or an error if maximum depth is exceeded.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/utils/yaml_func_exec.go(1 hunks)tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden(1 hunks)tests/test-cases/complete.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test-cases/complete.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 100-100:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/utils/yaml_func_exec.go (2)
113-115: Good error handling with wrappingProperly using the defined error constant with wrapping to provide context-specific details. This is a good practice.
25-45:⚠️ Potential issueReturn JSON decoding errors to caller
Currently, the function suppresses JSON parsing errors and returns the raw string. This could lead to unexpected behavior when callers expect a decoded object.
var decoded any if err = json.Unmarshal([]byte(res), &decoded); err != nil { - return nil, err + return res, nil }Likely an incorrect or invalid review comment.
tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden (1)
1-1: Improved error message formatThe error message now uses a more structured format with labeled key-value pairs (current=11, max=10) instead of parenthesized values. This improves readability and makes it clearer what each number represents.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/utils/yaml_func_exec.go (1)
99-117: 💡 Verification agent❓ Verification inconclusive
Environment variable retrieval: project prefers viper over os.Getenv.
The project guidelines recommend using
viper.BindEnvfor new environment variables instead ofos.Getenv. Also, the error message at line 105 usesshellValbefore it's set, which could be misleading. Consider using the raw string value for clarity.Here's a patch for the error message:
- return 0, fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?", - shellVal, MaxShellDepth) + return 0, fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)Let me know if you want help refactoring to use
viper.
Action Required – Refactor Environment Variable Retrieval and Update Error Message
Your changes in
pkg/utils/yaml_func_exec.gostill useos.Getenv("ATMOS_SHLVL"), whereas our guidelines recommend usingviper.BindEnvfor retrieving environment variables. In addition, the error message in the conversion failure block referencesshellVal(which is still zero) instead of the raw input, potentially causing confusion.Pl 10000 ease address the following:
Environment Variable Access:
Replace the direct call toos.Getenv("ATMOS_SHLVL")with the appropriateviper.BindEnvusage as per project guidelines.Error Message Clarity:
Change the error message when the conversion from string to integer fails. For example, update the snippet as follows:- return 0, fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?", - shellVal, MaxShellDepth) + return 0, fmt.Errorf("invalid ATMOS_SHLVL value: %s", atmosShellLvl)Let me know if you’d like further assistance with the
viperrefactoring.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 100-100:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
🧹 Nitpick comments (1)
pkg/utils/yaml_func_exec.go (1)
47-75: Nice modularization. Consider adding a timeout for shell execution.This function is tidy, but running shell commands without a timeout can lead to hanging processes. Passing a
context.Contextwith a timeout toShellRunnerwould make this more robust. Also, logging the command at debug level is a good touch.Want a code sample for adding a timeout?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/utils/yaml_func_exec.go(1 hunks)tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden(1 hunks)tests/test-cases/complete.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test-cases/complete.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 100-100:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 105-105:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("ATMOS_SHLVL (%d) exceeds maximum allowed depth (%d). Infinite recursion?",\n\tshellVal, MaxShellDepth)"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/utils/yaml_func_exec.go (1)
25-45:❓ Verification inconclusive
Solid refactor, but consider command validation and error handling tweaks.
The function is clear and concise, but a couple of things to keep in mind:
- Executing arbitrary shell from YAML is risky. Consider validating or at least logging the command for audit purposes.
- Error handling is good, but the error from
json.Unmarshalwill cause the function to fail if the shell output isn't valid JSON. If you want to support plain string outputs, you might want to returnresas a fallback instead of erroring out.Let me know if you want a code snippet for either suggestion.
Action: Enhance Shell Command Validation and Error Resilience
Overall, the refactored implementation in
pkg/utils/yaml_func_exec.gois clear and concise. However, I recommend a couple of improvements:
- Command Security: Since shell commands are invoked based on YAML inputs, it would be wise to validate or at least log the commands being executed. This audit trail can help monitor and mitigate potential security risks from arbitrary shell execution.
- Error Handling Flexibility: The current approach fails the function if
json.Unmarshalreturns an error. If supporting plain string outputs is a goal, consider returning the raw output (res) as a fallback instead of triggering an error.Let me know if you'd like a concrete code snippet for either suggestion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/utils/yaml_func_exec.go (3)
57-70: Optional: add execution timeout & cancellable contextLong‑running or hanging shell commands can block configuration loading.
Consider threading acontext.Context(with optional timeout) from the caller intoExecuteShellAndReturnOutputand down toShellRunner.
This keeps the utility responsive while remaining backward‑compatible when the caller passescontext.Background().
78-96: Replacecontext.TODO()with caller‑supplied contextInside
ShellRunnerthe hard‑codedcontext.TODO()prevents cancelation signals from propagating.
Suggestion:-func ShellRunner(command string, name string, dir string, env []string, out io.Writer) error { +func ShellRunner(ctx context.Context, command string, name string, dir string, env []string, out io.Writer) error { ... - return runner.Run(context.TODO(), parser) + return runner.Run(ctx, parser) }Caller changes are mechanical (pass the context received in the previous comment).
98-99: Doc‑comment mismatchThe comment references
getNextShellLevel(lower‑case) while the exported symbol isGetNextShellLevel.
Small rename keepsgo docoutput tidy.-// getNextShellLevel increments the ATMOS_SHLVL ... +// GetNextShellLevel increments the ATMOS_SHLVL ...cmd/cmd_utils.go (1)
178-187: Mixed exit pathsMost new branches exit with
log.Fatal, yet some older code (e.g. line 140) still usesu.LogErrorAndExit.
Using both patterns inside the same command package is slightly jarring and complicates grepping for exit points.
Might be worth standardising on one helper (even if the wider refactor is pending).🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 180-180:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 186-186:
deep-exit: calls to log.Fatal only in main() or init() functions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(11 hunks)pkg/utils/yaml_func_exec.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/cmd_utils.go (4)
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🧬 Code Graph Analysis (2)
cmd/cmd_utils.go (3)
internal/exec/shell_utils.go (1)
ExecuteShell(77-98)pkg/utils/yaml_func_exec.go (1)
ExecuteShellAndReturnOutput(48-75)internal/exec/template_utils.go (1)
ProcessTmpl(24-61)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
🪛 GitHub Check: golangci-lint
cmd/cmd_utils.go
[failure] 180-180:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 186-186:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 336-336:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 342-342:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 361-361:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 371-371:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 381-381:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 398-398:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 406-406:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 413-413:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 420-420:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 435-435:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 442-442:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 504-504:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 524-524:
deep-exit: calls to log.Fatal only in main() or init() functions
[failure] 664-664:
deep-exit: calls to log.Fatal only in main() or init() functions
pkg/utils/yaml_func_exec.go
[failure] 100-100:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: [mock-linux] examples/demo-atlantis
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [lint] Dockerfile
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/utils/yaml_func_exec.go (1)
39-44: Verify behaviour change on non‑JSON output
ProcessTagExecnow hard‑fails whenjson.Unmarshalcan’t parse the command’s stdout.
Previously callers received the raw string, which let them use plain values (e.g."v1.2.3").
Please double‑check that every existing!execusage returns valid JSON, otherwise this is a silent breaking change in configuration files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test-cases/complete.yaml (1)
18-18: Remove trailing whitespaceLine 18 has extra spaces after the closing quote, which triggers a YAMLLint trailing‑spaces error. Please trim the whitespace to satisfy the linter.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test-cases/complete.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/complete.yaml
[error] 18-18: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: website-deploy-previ 3D11 ew
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/complete.yaml (1)
15-17: Correctly updated error messagesThe stderr assertions now reflect the new infinite‐recursion check with explicit current and max depth values, matching the refactor in
yaml_func_exec.go.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test-cases/complete.yaml (1)
16-16: Minor nit: trim trailing spaces.
There are extra spaces after the closing quote on this line, which triggers atrailing-spaceserror in yamllint. You can safely delete them to keep the YAML clean.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 16-16: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test-cases/complete.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/complete.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (go)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/complete.yaml (1)
15-15: Heads-up: check the regex escape.
In single‑quoted YAML strings, backslashes are preserved literally, so'\?'becomes\?at runtime. Make sure your test harness treats these entries as regular expressions—so\?actually matches the question mark—instead of looking for a backslash-plus-?.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/utils/string_utils.go(1 hunks)pkg/utils/string_utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/string_utils_test.go (1)
pkg/utils/string_utils.go (1)
SplitStringByDelimiter(24-43)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/utils/string_utils.go (3)
23-23: Comment improvement looks good.The updated documentation now properly ends with a period, maintaining consistent comment formatting.
27-27: Good enhancement for field processing.Setting
TrimLeadingSpaceto true improves the robustness of the function by handling extra spaces in the input, which is particularly valuable for YAML function argument parsing.
34-42: Solid improvement for handling multiple delimiters.This filtering logic effectively removes empty strings that could result from consecutive delimiters in the input. The implementation is clean and efficient, using a pre-allocated slice with the correct capacity.
pkg/utils/string_utils_test.go (2)
5-69: Well-structured table-driven tests.Your test cases comprehensively cover the function's behavior including:
- Basic splitting
- Handling quoted substrings
- Edge cases (empty input, no delimiter)
- Special case with multiple spaces
- Error handling
This provides robust coverage for the enhanced
SplitStringByDelimiterfunction.
71-82: Helpful helper function.The
equalSlicesfunction provides better error messages than a simple deep equality check by logging the exact index and values that don't match. This makes test failures much easier to debug.
…ing and add unit tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/utils/yaml_func_exec.go (1)
98-101:context.TODO()still in library codeWe previously discussed replacing the placeholder with a real context passed from the caller (or at least
context.Background()), so downstream code can inject cancellation/timeout. Re‑raising here in case it was missed.
🧹 Nitpick comments (1)
pkg/utils/yaml_env_test.go (1)
41-63: RestoreATMOS_SHLVLafter each test caseMutating process‑wide env vars can leak state to subsequent tests running in the same process (or in parallel later on). Capture the original value and restore it with
t.Cleanupfor isolation.t.Run(tt.name, func(t *testing.T) { - // Set up the environment variable + // Preserve original value and restore after the sub‑test + orig := os.Getenv("ATMOS_SHLVL") + t.Cleanup(func() { // restore once the sub‑test finishes + if orig == "" { + os.Unsetenv("ATMOS_SHLVL") + } else { + os.Setenv("ATMOS_SHLVL", orig) + } + }) + + // Set up the environment variable for this case if tt.initialEnv != "" { os.Setenv("ATMOS_SHLVL", tt.initialEnv) } else { os.Unsetenv("ATMOS_SHLVL") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/utils/yaml_env_test.go(1 hunks)pkg/utils/yaml_func_exec.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/utils/yaml_func_exec.go (2)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.237Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🧬 Code Graph Analysis (2)
pkg/utils/yaml_env_test.go (1)
pkg/utils/yaml_func_exec.go (1)
GetNextShellLevel(104-124)
pkg/utils/yaml_func_exec.go (1)
pkg/utils/yaml_utils.go (1)
AtmosYamlFuncExec(19-19)
🪛 golangci-lint (1.64.8)
pkg/utils/yaml_func_exec.go
[error] 107-107: Error return value of v.BindEnv is not checked
(errcheck)
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_func_exec.go
[failure] 107-107:
Error return value of v.BindEnv is not checked
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Hadolint
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [lint] Dockerfile
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
|
These changes were released in v1.173.0. |
what
!env, !exec,!include,!repo-rootor default values.why
GITHUB_TOKEN: !exec gh auth tokenSummary by CodeRabbit
Summary by CodeRabbit
!env,!exec,!include, and!repo-root.!repo-rootYAML function.