Support Trailing Args for Custom Commands

[samtholiya]

what

• We would add support for double dash argument in custom command. So from now on, customer can easily use {{ .TrailingArgs }} in template to get the commands as is after the --

why

• Currently we do not have a defined way of passing all the native commands of a step that might require native args like we do for terraform cli.
• this will help users know how to use double dash to get the native args of a step.

references

• DEV-112

Summary by CodeRabbit

• New Features

  • Enhanced command execution logic to better handle command-line arguments, including a new function for extracting trailing arguments.
  • Introduced a new custom command named "echo" that demonstrates handling of trailing arguments.

• Documentation

  • Added a new section explaining trailing arguments in the Atmos custom commands documentation, including usage examples.

• Tests

  • Added a test case to validate that the custom command processes inputs following the double dash correctly.

[osterman]

• update docs to explain usage of .TrailingArgs with custom commands

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

tests/fixtures/scenarios/custom-command/atmos.yaml (2)

13-21: Improve command definition formatting and clarity.

The command definition needs some adjustments:

1. Fix indentation for the command entry (should be 2 spaces)
2. Enhance the description to be more specific about double dash usage

Apply this diff to improve the command definition:

-# # Use positional arguments with default values
-- name: echo
+  # Use positional arguments with default values
+  - name: echo
   description: "Displays a args before and after double dash."

Also, consider updating the description to be more specific:

-  description: "Displays a args before and after double dash."
+  description: "Demonstrates argument handling by displaying arguments before and after the double dash (--) separator."

🧰 Tools

🪛 YAMLlint (1.35.1)

[warning] 13-13: wrong indentation: expected 2 but found 0

(indentation)

---

22-23: Add newline at end of file and verify template variables.

The echo steps look good and align with the PR objectives of supporting double dash arguments. The template variables .Arguments.name and .TrailingArgs are used appropriately to demonstrate the separation of arguments.

Add a newline at the end of the file to comply with YAML best practices:

     - "echo Cobra Args: [{{ .Arguments.name }}]"
-    - "echo Trailing Args: [{{ .TrailingArgs }}]"
+    - "echo Trailing Args: [{{ .TrailingArgs }}]"
+

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 23-23: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57cf738 and 56d9ec5.

📒 Files selected for processing (2)

• tests/fixtures/scenarios/custom-command/atmos.yaml (1 hunks)
• tests/test-cases/custom-command.yaml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test-cases/custom-command.yaml

🧰 Additional context used

🪛 YAMLlint (1.35.1)

tests/fixtures/scenarios/custom-command/atmos.yaml

[warning] 13-13: wrong indentation: expected 2 but found 0

(indentation)

---

[error] 23-23: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (4)

• GitHub Check: Build (windows-latest, windows)
• GitHub Check: website-deploy-preview
• GitHub Check: Analyze (go)
• GitHub Check: Summary

🔇 Additional comments (3)

tests/fixtures/scenarios/custom-command/atmos.yaml (3)

1-2: LGTM! Base path configuration is clear and concise.

---

3-6: Verify if stacks configuration is required.

Based on the PR objectives focusing on custom commands and double dash arguments, the stacks configuration might be unnecessary for this scenario. However, I understand from the past review comments that removing stacks.base_path and stacks.included_paths is currently not possible due to a potential bug.

Would you like me to help investigate why the stacks configuration cannot be removed or create an issue to track this technical debt?

---

7-9: LGTM! Logging configuration is appropriate for testing.

[osterman]

LGTM

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

cmd/cmd_utils.go (2)

76-77: Consider updating the TODO comment.

The TODO comment references PR #959, but based on the PR objectives, this implementation is part of PR #1046. Consider removing or updating the comment to maintain accuracy.

-// TODO: we need to update this post https://github.com/cloudposse/atmos/pull/959 gets merged
+// Flag hint for double dash argument support (PR #1046)

---

412-439: Consider optimizing the argument processing logic.

The current implementation has a nested loop for filtering arguments which could be optimized for better performance, especially with large argument lists.

Here's a more efficient implementation using a single pass:

 func extractTrailingArgs(args []string, osArgs []string) ([]string, string) {
 	doubleDashIndex := lo.IndexOf(osArgs, "--")
 	mainArgs := args
 	trailingArgs := ""
 	if doubleDashIndex > 0 {
 		mainArgs = lo.Slice(osArgs, 0, doubleDashIndex)
 		trailingArgs = strings.Join(lo.Slice(osArgs, doubleDashIndex+1, len(osArgs)), " ")
-		result := []string{}
-		lookup := make(map[string]bool)
-
-		// Populate a lookup map for quick existence check
-		for _, val := range mainArgs {
-			lookup[val] = true
-		}
-
-		// Iterate over leftArr and collect matching elements in order
-		for _, val := range args {
-			if lookup[val] {
-				result = append(result, val)
-			}
-		}
-		mainArgs = result
+		// Create a set of mainArgs for O(1) lookup
+		mainArgsSet := make(map[string]struct{}, len(mainArgs))
+		for _, arg := range mainArgs {
+			mainArgsSet[arg] = struct{}{}
+		}
+		// Filter args in a single pass
+		filteredArgs := make([]string, 0, len(args))
+		for _, arg := range args {
+			if _, exists := mainArgsSet[arg]; exists {
+				filteredArgs = append(filteredArgs, arg)
+			}
+		}
+		mainArgs = filteredArgs
 	}
 	return mainArgs, trailingArgs
 }

This optimization:

1. Uses a single map for lookups
2. Pre-allocates the result slice
3. Performs filtering in a single pass

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dec140a and 981ba59.

📒 Files selected for processing (1)

• cmd/cmd_utils.go (4 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (2)

• GitHub Check: Build (windows-latest, windows)
• GitHub Check: Summary

🔇 Additional comments (1)

cmd/cmd_utils.go (1)

274-274: LGTM! Clean integration of trailing args.

The changes effectively integrate the trailing args feature into the command execution context, making them available via .TrailingArgs in templates.

Also applies to: 315-317

[github-actions]

These changes were released in v1.161.0.