pkg/store/google_secret_manager_store.go (4)

23-36: Consider environment-based credential management
Storing credentials as a JSON string in configuration risks accidental exposure. Adopting environment variables or secret references can enhance security.

---

38-76: Consider allowing caller-supplied context
Currently, a background context is used to initialize the store. Passing a custom context would allow more flexibility for cancellation and timeouts.

---

99-190: Refactor creation & versioning flow

1. Consider splitting secret creation and version addition into separate methods for clarity and improved testability.
2. Restricting values to strings might be limiting if you need to store JSON or other data.

---

192-237: Support specifying secret versions
Always retrieving "latest" can be restrictive. Consider allowing a version parameter for more advanced workflows.

pkg/store/google_secret_manager_store_test.go (3)

51-55: Remove or utilize the unused helper
Static analysis flags “newMockGSMClient” as unused. Consider removing or integrating it in tests.

🧰 Tools

🪛 golangci-lint (1.62.2)

52-52: func newMockGSMClient is unused

(unused)

---

56-272: Comprehensive sub-tests
“TestGSMStore_Set” covers multiple paths effectively. If concurrency is a concern, consider adding parallel or race tests.

---

376-429: Handling environment credentials
Providing credentials from the environment is pragmatic. You might mock environment variables for consistent CI runs.

website/docs/core-concepts/stacks/yaml-functions/store.mdx (1)

68-74: Effective Google Secret Manager Example Addition
This set of changes demonstrates the usage of Google Secret Manager for various scenarios—including direct retrieval, default values, and cross-stack access. The examples are concise and correctly use templating for dynamic stack names. One minor suggestion would be to include a brief inline note (or reference) on when to prefer using the alias “gsm” compared to “google-secret-manager” for added clarity.

website/docs/core-concepts/projects/configuration/stores.mdx (1)

204-248: Detailed GSM Options and Authentication Instructions
The expanded documentation following the configuration sample thoroughly explains the options for GSM—covering the required project_id, optional prefix and credentials, as well as the stack_delimiter. The authentication section is particularly useful, outlining Application Default Credentials, Direct Credentials, and Workload Identity methods. This level of detail greatly aids users. A small suggestion: consider emphasizing best practices for secure handling of credentials (especially when embedding JSON strings), which can further educate users on avoiding potential pitfalls.

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

pkg/store/google_secret_manager_store.go (1)

99-190: Add context timeout for GSM operations.

Using context.Background() without a timeout could lead to hanging operations. Consider adding a reasonable timeout.

Apply this diff:

 func (s *GSMStore) Set(stack string, component string, key string, value interface{}) error {
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
-	ctx := context.Background()

pkg/store/google_secret_manager_store_test.go (1)

376-429: Add test case for client cleanup.

Consider adding a test case to verify that the client is properly closed when an error occurs during initialization.

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

pkg/store/google_secret_manager_store.go (1)

108-200: Potential concurrency considerations
If multiple calls to Set() operate on the same key concurrently, consider version sprawl and race conditions. Also, storing large secrets might require additional checks.

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

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

pkg/store/google_secret_manager_store.go (3)

17-20: Make the operation timeout configurable
Currently, gsmOperationTimeout is hardcoded to 30 seconds. Exposing it in the configuration can help accommodate different performance environments.

---

49-88: Add optional debug logging
Currently, the function sets up the client without providing logs to troubleshoot credential or connectivity issues.

---

144-165: Consider logging version creation
Adding a simple debug log here would help trace newly added secret versions.

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

tests/fixtures/scenarios/invalid-stacks/stacks/catalog/invalid-template.yaml (1)

1-6: Clarify the Test Fixture's Purpose

This YAML fixture intentionally embeds an undefined templating variable ({{ .not-exist }}) in the location field to simulate an invalid template scenario for the Terraform component myapp. To improve clarity for future maintainers, consider adding a comment at the top of the file that explains its intended use for testing error handling in stack templates.

website/docs/core-concepts/share-data/remote-state.mdx (1)

363-364: Punctuation Recommendation for PrimaryCTA

A static analysis hint suggests adding a punctuation mark at the end of the PrimaryCTA content. Consider appending a period (or other appropriate punctuation) after the text in the PrimaryCTA component on these lines to ensure the paragraph ends neatly.

🧰 Tools

🪛 LanguageTool

[grammar] ~363-~363: Please add a punctuation mark at the end of paragraph.
Context: ...n how to use 'atmos.Component' template function

(PUNCTUATION_PARAGRAPH_END)

internal/exec/workflow.go (1)

119-129: Improved error messaging with available workflow options

The enhanced error handling provides useful guidance to users when a workflow isn't found by listing all available workflows. This is a significant usability improvement.

Consider simplifying the sorting operation with sort.Strings(validWorkflows) instead of sort.Sort(sort.StringSlice(validWorkflows)) for better readability.

- sort.Sort(sort.StringSlice(validWorkflows))
+ sort.Strings(validWorkflows)

pkg/utils/markdown_utils_test.go (1)

96-96: Test command using incorrect test function name.

The command is running TestPrintErrorMarkdownAndExit when testing PrintInvalidUsageErrorAndExit. This should be updated to use the correct test function name.

-cmd := exec.Command(execPath, "-test.run=TestPrintErrorMarkdownAndExit")
+cmd := exec.Command(execPath, "-test.run=TestPrintInvalidUsageErrorAndExit")

tests/fixtures/scenarios/vendor2/atmos.yaml (1)

10-10: Minor nitpick: Remove trailing spaces.

Line 10 contains trailing whitespace per the YAMLlint feedback. Please remove it for cleaner formatting.

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 10-10: trailing spaces

(trailing-spaces)

tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml (1)

10-10: Minor nitpick: Remove trailing whitespace.

Line 10 seems to have extraneous trailing spaces. Removing these will help maintain consistency with YAML lint guidelines.

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 10-10: trailing spaces

(trailing-spaces)

cmd/workflow_test.go (4)

23-23: Consider handling error from os.Pipe()

The ignored error from os.Pipe() should be captured and checked. While pipe creation rarely fails, proper error handling improves robustness.

- r, w, _ := os.Pipe()
+ r, w, err := os.Pipe()
+ assert.NoError(t, err, "Creating pipe for stdout capture should execute without error")

---

15-19: Add cleanup for environment variables

Environment variables set during tests should be restored to their original values when the test completes to prevent side effects on other tests.

+ oldAtmosCliConfigPath := os.Getenv("ATMOS_CLI_CONFIG_PATH")
+ oldAtmosBasePath := os.Getenv("ATMOS_BASE_PATH")
+ defer func() {
+   os.Setenv("ATMOS_CLI_CONFIG_PATH", oldAtmosCliConfigPath)
+   os.Setenv("ATMOS_BASE_PATH", oldAtmosBasePath)
+ }()

err := os.Setenv("ATMOS_CLI_CONFIG_PATH", stacksPath)
assert.NoError(t, err, "Setting 'ATMOS_CLI_CONFIG_PATH' environment variable should execute without error")

---

40-40: Correct the error message for pipe closure operation

The error message for closing the writer pipe incorrectly references the workflow command. Use a more descriptive message specific to the pipe closing operation.

- assert.NoError(t, err, "'atmos workflow' command should execute without error")
+ assert.NoError(t, err, "Closing stdout pipe writer should execute without error")

---

13-13: Consider using test helper for path resolution

The hardcoded relative path "../tests/fixtures/scenarios/atmos-overrides-section" might break if the directory structure changes. Consider using a test helper function to locate test fixtures relative to the repository root.

cmd/workflow_test.go (1)

38-42: 🛠️ Refactor suggestion

Use defer for stdout restoration

The current implementation could leave stdout in a corrupted state if the test fails before restoration. Using defer at the beginning of the test ensures proper cleanup regardless of execution path.

// Capture stdout
oldStdout := os.Stdout
r, w, _ := os.Pipe()
os.Stdout = w
+ defer func() {
+   os.Stdout = oldStdout
+ }()

// ...test execution...

// Close the writer and restore stdout
err = w.Close()
assert.NoError(t, err, "'atmos workflow' command should execute without error")

- os.Stdout = oldStdout

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

// Capture stdout
oldStdout := os.Stdout
r, w, _ := os.Pipe()
os.Stdout = w
defer func() {
	os.Stdout = oldStdout
}()

// ...test execution...

// Close the writer; stdout is restored automatically by the deferred call
err = w.Close()
assert.NoError(t, err, "'atmos workflow' command should execute without error")