Support Default Values for Arguments in Custom Commands #905
Conversation
Listener430
added
enhancement
📝 WalkthroughWalkthroughThis pull request enhances the Atmos CLI's custom command argument handling by modifying the validation logic for command arguments. The changes include a new approach to checking required arguments and their defaults, ensuring commands execute correctly based on provided configurations. Additionally, a new custom command named "greet" is introduced, which requires a positional argument. The documentation is updated to reflect these changes in command usage and argument handling. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/cmd_utils.go (2)
224-245: Thoughtful merging of supplied arguments with defaults.Storing final parameters in
resolvedArgsis practical, but using commas as a delimiter may break if any argument includes commas. Consider a safer serialization approach (e.g. JSON-encoding).- cmd.Annotations["resolvedArgs"] = strings.Join(finalArgs, ",") + data, err := json.Marshal(finalArgs) + if err == nil { + cmd.Annotations["resolvedArgs"] = string(data) + }
278-284: Split-by-comma might misread arguments containing commas.If an argument includes a comma, it splits incorrectly. Storing arguments in JSON (or a similar technique) would avoid confusion when reconstructing them here.
website/docs/core-concepts/custom-commands/custom-commands.mdx (2)
44-45: Recommend removing or adjusting the trailing punctuation.Avoiding multiple punctuation marks maintains style consistency.
🧰 Tools
🪛 LanguageTool
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ...nameargument, but uses a default of "John Doe" if none is provided:. ```yaml # subco...(EN_QUOTES)
[formatting] ~45-~45: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...efault of "John Doe" if none is provided:. ```yaml # subcommands commands: - n...(DOUBLE_PUNCTUATION_PREMIUM)
66-67: Use consistent quotes around "John Doe".Keeping quotation usage consistent improves readability.
🧰 Tools
🪛 LanguageTool
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Aliceor defaulting to "John Doe"shell atmos greet ``` ## Passing F...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/cmd_utils.go(2 hunks)examples/demo-custom-command/atmos.yaml(1 hunks)pkg/schema/schema.go(1 hunks)website/docs/core-concepts/custom-commands/custom-commands.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/custom-commands/custom-commands.mdx
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided:. ```yaml # subco...
(EN_QUOTES)
[formatting] ~45-~45: These punctuation marks differ from each other. Use only one if you are ending a sentence.
Context: ...efault of "John Doe" if none is provided:. ```yaml # subcommands commands: - n...
(DOUBLE_PUNCTUATION_PREMIUM)
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Alice or defaulting to "John Doe" shell atmos greet ``` ## Passing F...
(EN_QUOTES)
🔇 Additional comments (6)
cmd/cmd_utils.go (4)
172-195: Solid check for commands lacking arguments.
This block correctly handles zero-argument commands by validating whether they have steps or sub-commands. The approach sets user expectations appropriately by exiting when nothing is avaialble to execute.
196-203: Clear count of required arguments without defaults.
The loop neatly identifies how many mandatory arguments lack defaults. This is logically sound for subsequent checks.
204-223: Comprehensive error messaging for missing arguments.
When the user provides fewer arguments than the required count, the error message lists the missing arguments by name, which simplifies debugging.
290-290: Access to merged arguments is correct.
Index-based mapping to argumentsData is valid here, thanks to prior validations.
pkg/schema/schema.go (1)
346-347: New fields enable default values for required arguments.
This cleanly captures both the Required and Default properties. Ensure documentation clarifies that “required” arguments can also have default values.
examples/demo-custom-command/atmos.yaml (1)
60-71: Good demonstration of a required arg with a default value.
This example will help users see how to provide flexible defaults for mandatory arguments. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/docs/core-concepts/custom-commands/custom-commands.mdx (2)
42-43: Enhance clarity of positional arguments description.The sentence structure could be improved for better readability.
-Atmos also can support positional arguments. If a positional argument is required but not provided by the user, +Atmos supports positional arguments. If a required positional argument is not provided by the user,🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: A different word order might sound more natural.
Context: ...llo ``` ## Positional Arguments Atmos also can support positional arguments. If a posi...(AI_HYDRA_LEO_WORD_ORDER)
64-69: Command usage examples are comprehensive.The examples demonstrate both cases:
- Explicit argument passing
- Using the default value
Consider adding a note about the error case that this PR fixes.
Add a comment explaining that previously, this configuration would have caused an error, but now works as expected:
atmos greet Aliceor defaulting to "John Doe"
atmos greet +``` + +Previously, marking an argument as required while providing a default value would cause an error. +Now, the default value is correctly used when no argument is provided. + +```shell🧰 Tools
🪛 LanguageTool
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Aliceor defaulting to "John Doe"shell atmos greet ``` ## Passing F...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/core-concepts/custom-commands/custom-commands.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/custom-commands/custom-commands.mdx
[uncategorized] ~42-~42: A different word order might sound more natural.
Context: ...llo ``` ## Positional Arguments Atmos also can support positional arguments. If a posi...
(AI_HYDRA_LEO_WORD_ORDER)
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...
(EN_QUOTES)
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Alice or defaulting to "John Doe" shell atmos greet ``` ## Passing F...
(EN_QUOTES)
🔇 Additional comments (2)
website/docs/core-concepts/custom-commands/custom-commands.mdx (2)
44-45: Documentation aligns well with PR objectives.
The introduction of the greet command example effectively demonstrates how to handle required arguments with default values.
🧰 Tools
🪛 LanguageTool
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...
(EN_QUOTES)
50-56: Well-structured command configuration example.
The configuration clearly shows how to:
- Mark an argument as required
- Provide a default value
- Include a description
This example effectively addresses the PR's main objective.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/cmd_utils.go (3)
171-193: Enhance error messages and sub-command display for better user experience.The validation logic is solid, but we can improve the user experience with better messaging:
if len(commandConfig.Commands) > 0 { // show sub-commands - sb.WriteString("Available command(s):\n") + sb.WriteString("Available sub-commands:\n") for i, c := range commandConfig.Commands { sb.WriteString( - fmt.Sprintf("%d. %s %s %s\n", i+1, parentCommand.Use, commandConfig.Name, c.Name), + fmt.Sprintf("%d. %s %s %s\t\t%s\n", + i+1, + parentCommand.Use, + commandConfig.Name, + c.Name, + c.Description), ) } } else { // truly invalid, nothing to do u.LogError(schema.AtmosConfiguration{}, errors.New( - "invalid command: no args, no steps, no sub-commands", + fmt.Sprintf("invalid command '%s': command must define arguments, steps, or sub-commands", commandConfig.Name), )) }
223-244: Define constant for annotation key and improve error message.The argument merging logic is solid, but we can improve maintainability and error handling:
+// Define constants for annotation keys +const ( + resolvedArgsAnnotationKey = "resolvedArgs" +) // Merge user-supplied arguments with defaults finalArgs := make([]string, len(commandConfig.Arguments)) for i, arg := range commandConfig.Arguments { if i < len(args) { finalArgs[i] = args[i] } else { if arg.Default != "" { finalArgs[i] = fmt.Sprintf("%v", arg.Default) } else { // This theoretically shouldn't happen: - sb.WriteString(fmt.Sprintf("Missing required argument '%s' with no default!\n", arg.Name)) + sb.WriteString(fmt.Sprintf( + "Internal error: Required argument '%s' is missing and has no default value. "+ + "This should have been caught by earlier validation.\n", + arg.Name)) u.LogErrorAndExit(schema.AtmosConfiguration{}, errors.New(sb.String())) } } } if cmd.Annotations == nil { cmd.Annotations = make(map[string]string) } -cmd.Annotations["resolvedArgs"] = strings.Join(finalArgs, ",") +cmd.Annotations[resolvedArgsAnnotationKey] = strings.Join(finalArgs, ",")
277-282: Add warning for fallback and define separator constant.The argument retrieval logic is correct, but we can improve error handling and maintainability:
+const ( + resolvedArgsAnnotationKey = "resolvedArgs" + resolvedArgsSeparator = "," +) -mergedArgsStr := cmd.Annotations["resolvedArgs"] +mergedArgsStr := cmd.Annotations[resolvedArgsAnnotationKey] -finalArgs := strings.Split(mergedArgsStr, ",") +finalArgs := strings.Split(mergedArgsStr, resolvedArgsSeparator) if mergedArgsStr == "" { // If for some reason no annotation was set, just fallback + u.LogWarning(atmosConfig, "No resolved arguments found in annotations, falling back to original args") finalArgs = args }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cmd_utils.go(2 hunks)
🔇 Additional comments (1)
cmd/cmd_utils.go (1)
195-222: Strengthen argument validation with type checking.
The current validation ensures required arguments are provided, but we can make it more robust:
// Check if the number of arguments provided is less than the required number of arguments
if len(args) < requiredNoDefaultCount {
sb.WriteString(
- fmt.Sprintf("Command requires at least %d argument(s) (no defaults provided for them):\n",
- requiredNoDefaultCount))
+ fmt.Sprintf("Command '%s' requires %d argument(s):\n",
+ commandConfig.Name, requiredNoDefaultCount))
// List out which arguments are missing
missingIndex := 1
for _, arg := range commandConfig.Arguments {
if arg.Required && arg.Default == "" {
- sb.WriteString(fmt.Sprintf(" %d. %s\n", missingIndex, arg.Name))
+ sb.WriteString(fmt.Sprintf(" %d. %s (%s)\n",
+ missingIndex,
+ arg.Name,
+ arg.Type))
missingIndex++
}
}Let's verify if argument types are defined in the schema:
osterman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/docs/core-concepts/custom-commands/custom-commands.mdx (2)
50-56: Example configuration is well structured and clear!The YAML configuration example effectively demonstrates:
- Setting a required argument with
required: true- Providing a default value with
default: John DoeConsider using typographic quotation marks for "John Doe" to improve readability:
- default: John Doe + default: "John Doe"
64-69: Usage examples are comprehensive!The documentation includes both cases:
- Explicit argument:
atmos greet Alice- Default value:
atmos greetThis helps users understand how the default value works in practice.
Consider using typographic quotation marks for "John Doe" in the explanatory text:
-or defaulting to "John Doe" +or defaulting to "John Doe"🧰 Tools
🪛 LanguageTool
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Aliceor defaulting to "John Doe"shell atmos greet ``` ## Passing F...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/core-concepts/custom-commands/custom-commands.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/custom-commands/custom-commands.mdx
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...
(EN_QUOTES)
[typographical] ~66-~66: Consider using typographic quotation marks here.
Context: ... atmos greet Alice or defaulting to "John Doe" shell atmos greet ``` ## Passing F...
(EN_QUOTES)
🔇 Additional comments (1)
website/docs/core-concepts/custom-commands/custom-commands.mdx (1)
42-45: Documentation accurately describes the new behavior!
The explanation clearly communicates how required arguments and default values work together, which directly addresses the PR's objective.
🧰 Tools
🪛 LanguageTool
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...
(EN_QUOTES)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_cases.yaml (2)
172-185: LGTM! Well-structured test case for explicit argument passing.The test case effectively validates the custom command's behavior when an argument is provided. The expectations are clear and comprehensive.
Consider adding a comment in the description to clarify that this test validates the override of the default value, making the test's purpose more explicit:
- description: "Validate atmos custom command greet runs with argument provided." + description: "Validate atmos custom command greet runs with explicit argument, overriding the default value."
171-199: Consider adding error case test coverage.While the current test cases effectively cover the happy paths (explicit argument and default value), consider adding test cases for error scenarios:
- Invalid argument validation
- Required argument missing (when no default is set)
Example test case structure:
- name: atmos greet with invalid argument enabled: true description: "Validate atmos custom command greet fails gracefully with invalid input." workdir: "../examples/demo-custom-command/" command: "atmos" args: - "greet" - "" # or some invalid input expect: stderr: - "Invalid argument provided" # adjust based on actual error message exit_code: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_cases.yaml(1 hunks)
🔇 Additional comments (1)
tests/test_cases.yaml (1)
187-199: LGTM! Solid test coverage for default value behavior.The test case effectively validates that the command falls back to the default value ("John Doe") when no argument is provided. This is crucial for verifying the new default value support feature.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
website/docs/core-concepts/custom-commands/custom-commands.mdx (3)
42-43: Consider clarifying the relationship between required arguments and defaults.The explanation is good, but we could make it even clearer by explicitly stating that providing a default value satisfies the "required" constraint.
-Atmos also supports positional arguments. If a positional argument is required but not provided by the user, -the command will fail—unless you define a default in your config. +Atmos also supports positional arguments. A required positional argument must either be provided by the user +or have a default value defined in your config—otherwise, the command will fail.
45-46: Enhance readability with typographic quotation marks.Consider using typographic quotation marks for better readability.
-but uses a default of "John Doe" if none is provided. +but uses a default of "John Doe" if none is provided.🧰 Tools
🪛 LanguageTool
[typographical] ~46-~46: Consider using typographic quotation marks here.
Context: ...nameargument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...(EN_QUOTES)
65-70: Consider enhancing the examples with expected output.The examples are clear, but would be even more helpful if they showed the expected output. Also, consider using typographic quotation marks for consistency.
atmos greet Alice +# Output: Hello Alice!or defaulting to "John Doe"
atmos greet +# Output: Hello John Doe!🧰 Tools
🪛 LanguageTool
[typographical] ~67-~67: Consider using typographic quotation marks here.
Context: ... atmos greet Aliceor defaulting to "John Doe"shell atmos greet ``` ## Passing F...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/core-concepts/custom-commands/custom-commands.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/custom-commands/custom-commands.mdx
[typographical] ~46-~46: Consider using typographic quotation marks here.
Context: ... name argument, but uses a default of "John Doe" if none is provided. ```yaml # subcom...
(EN_QUOTES)
[typographical] ~67-~67: Consider using typographic quotation marks here.
Context: ... atmos greet Alice or defaulting to "John Doe" shell atmos greet ``` ## Passing F...
(EN_QUOTES)
🔇 Additional comments (1)
website/docs/core-concepts/custom-commands/custom-commands.mdx (1)
51-57: Well-structured example configuration!The example effectively demonstrates how to configure a command with both
required: trueand a default value. The inclusion of descriptive fields makes it very clear.
|
These changes were released in v1.134.0. |
what
The task is was "If a default is provided for a custom command, and required is true, then it shouldn't error."
before
a custom cli-command is defined
after
why
Provide a deafult value in atmos.yaml for a custom CLI command, so it doesn't error out (before it did).
references
Link to the ticket - https://linear.app/cloudposse/issue/DEV-2327/default-value-for-demo-argument-should-not-cause-an-error
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests