Update overrides section behavior in imports and inline
#1122
Conversation
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports # Conflicts: # examples/quick-start-advanced/Dockerfile # website/docs/integrations/atlantis.mdx
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
…verrides-in-imports
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
📝 WalkthroughWalkthroughThis pull request updates how markdown content is formatted by modifying Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Stack Processor
participant YAMLProcessor as ProcessYAMLConfigFile
Caller->>YAMLProcessor: Call with inline & import override maps
YAMLProcessor-->>Caller: Return updated override maps
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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 (3)
internal/exec/workflow.go (1)
120-128: Enhanced error message clarity for workflows.
The updated error message now constructs a detailed list of available workflows using the sorted keys, making it easier for users to see what options are available. One minor suggestion: if no string formatting is needed later in the error handling, consider usingerrors.New(errorMarkdown)for simplicity, though the current use offmt.Errorfis perfectly acceptable.tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml (1)
10-10: Remove trailing spaces.
There are trailing spaces on line 10. Please remove these spaces to adhere to clean YAML formatting standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
website/docs/core-concepts/share-data/remote-state.mdx (1)
353-363: Enhance Punctuation in Remote State ActionCard Block
The ActionCard block clearly illustrates the alternative methods using the!terraform.outputYAML function and theatmos.Componenttemplate function. To further polish the presentation, please add a punctuation mark (e.g., a period) at the end of the text in thePrimaryCTAsection on line 363.🧰 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/about.go(1 hunks)cmd/support.go(1 hunks)examples/quick-start-advanced/Dockerfile(1 hunks)go.mod(11 hunks)internal/exec/stack_processor_utils.go(18 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_utils.go(1 hunks)internal/exec/workflow.go(1 hunks)pkg/stack/stack_processor.go(2 hunks)tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/catalog/c1.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/dev.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/prod.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/sandbox.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/staging.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/test.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/teams/team1.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/teams/team2.yaml(1 hunks)tests/test-cases/atmos-overrides-section.yaml(1 hunks)website/docs/core-concepts/share-data/remote-state.mdx(1 hunks)website/docs/core-concepts/share-data/share-data.mdx(1 hunks)website/docs/integrations/atlantis.mdx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
website/docs/core-concepts/share-data/share-data.mdx (2)
Learnt from: aknysh
PR: cloudposse/atmos#810
File: website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx:0-0
Timestamp: 2024-12-03T04:01:16.446Z
Learning: In the `terraform.output.mdx` documentation file (`website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx`), the cache invalidation and cache scope behavior for the `!terraform.output` function are already described.
Learnt from: aknysh
PR: cloudposse/atmos#810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
examples/quick-start-advanced/Dockerfile (2)
Learnt from: aknysh
PR: cloudposse/atmos#775
File: examples/quick-start-advanced/Dockerfile:9-9
Timestamp: 2024-11-12T05:52:05.088Z
Learning: It is acceptable to set `ARG ATMOS_VERSION` to a future version like `1.105.0` in `examples/quick-start-advanced/Dockerfile` if that will be the next release.
Learnt from: osterman
PR: cloudposse/atmos#801
File: examples/quick-start-advanced/Dockerfile:9-9
Timestamp: 2024-11-23T00:13:22.004Z
Learning: When updating the `ATMOS_VERSION` in Dockerfiles, the team prefers to pin to the next future version when the PR merges, even if the version is not yet released.
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 LanguageTool
website/docs/core-concepts/share-data/remote-state.mdx
[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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: [lint] golangci
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (61)
website/docs/integrations/atlantis.mdx (1)
675-677: Atmos Version Update in GitHub Action EnvironmentThe update of the
ATMOS_VERSIONfrom1.164.0to1.165.3in the environment variables looks consistent with our overall version bump. Please double-check that any references to the older version in related tests or documentation have also been updated accordingly.examples/quick-start-advanced/Dockerfile (1)
9-9: Updated ARG ATMOS_VERSIONChanging the
ARG ATMOS_VERSIONfrom1.164.0to1.165.3in the Dockerfile is aligned with the coordinated version update seen in the docs. This change is consistent with our current release strategy.tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/sandbox.yaml (1)
1-9: Configuration structure looks good.This sandbox manifest properly defines the environment stage and imports team configurations. The structure follows Atmos best practices with a clear separation of concerns.
cmd/support.go (1)
23-23: Properly updated PrintfMarkdown call.The change adds a format specifier to the PrintfMarkdown call, which is the correct approach for string formatting. This matches similar changes in other commands.
tests/fixtures/scenarios/atmos-overrides-section/stacks/teams/team2.yaml (1)
1-12: Effective overrides demonstration.This file correctly demonstrates the overrides functionality by importing a base configuration and providing team-specific variable values. The empty top-level overrides section and the terraform-specific overrides are both structured properly.
tests/fixtures/scenarios/atmos-overrides-section/stacks/catalog/c1.yaml (1)
1-12: Base component definition is well-structured.This file properly defines a base Terraform component with test variables that can be overridden by team configurations. The schema reference is correctly included.
tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/staging.yaml (1)
1-17: Clean and well-structured fixture for testing overridesThis new YAML file provides a clear test case for the updated overrides section behavior. It properly demonstrates the staging environment configuration with team imports and environment-specific terraform variable overrides.
cmd/about.go (1)
20-20: Format specifier added for PrintfMarkdownThe change adds a proper format specifier when using PrintfMarkdown, improving string handling consistency.
tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/prod.yaml (1)
1-19: Clear production test fixture with complete overridesThis fixture defines a production environment with multiple variable overrides (a, b, c) that will properly test the overrides section behavior across different levels.
tests/fixtures/scenarios/atmos-overrides-section/stacks/teams/team1.yaml (1)
1-13: Appropriate team-level configuration with overridesThis team configuration imports component definitions and specifies team-specific variable values that will be used in testing the inheritance and override behavior across different configuration levels.
tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/dev.yaml (1)
1-15: New YAML manifest for the dev environment added.
This file clearly defines the environment variables (stage: dev), imports for teams, and a Terraform overrides section. Ensure that the referenced team directories (e.g.teams/team1andteams/team2) exist and that the override structure aligns with the expectations in other environments.tests/fixtures/scenarios/atmos-overrides-section/stacks/deploy/test.yaml (1)
1-20: New YAML manifest for the test environment added.
This file correctly sets the stage to "test" and defines both inline and Terraform override sections. The structure is consistent with the dev manifest, which helps maintain clarity across environments.tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml (1)
1-22: New base configuration file for Atmos introduced.
The configuration is well-structured, defining the base path, components, stack configurations (including included/excluded paths and a naming pattern), and logging settings. This will serve as a strong foundation supporting the environment-specific overrides defined in the stack manifests.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
internal/exec/vendor_utils.go (1)
136-137: Enhanced error message for missing '--component' flag.
The revised error message now not only informs the user that the--componentflag is required but also provides an example usage, which helps clarify the expected input. This improvement in user guidance is appreciated.internal/exec/validate_stacks.go (1)
155-169: Add usage note for the override maps
These new parameters are passed as empty maps. Document their intended usage or provide an inline comment about how these maps will be populated.tests/test-cases/atmos-overrides-section.yaml (5)
1-25: Validate negative scenarios
These tests for the 'prod' stack look solid. Consider adding negative or edge case scenarios for a more robust test suite.
[approve]
26-49: Confirm coverage for staging
The staging test can also exercise partial or conflicting overrides for additional confidence.
50-73: Good demonstration of dev stack
This test covers dev overrides effectively. If feasible, add a check for scenario with no overrides applied.
74-97: Sandbox test is comprehensive
All required keys are shown. The approach appears consistent with other tests. Keep up the coverage.
98-121: Thorough test for 'test' stack
This neatly verifies override inheritance. Everything else seems aligned.pkg/stack/stack_processor.go (3)
47-50: Function signature extension
The inline and import-based override parameters clarify usage. Confirm these are consistently passed from all call sites.
58-59: Return maps confirmation
Returning two extra maps for overrides is logical. Ensure downstream callers consume them appropriately.
72-75: Param forwarding confirmed
Forwarding the new override maps toexec.ProcessYAMLConfigFileis consistent with the new design. Looks good.internal/exec/stack_processor_utils.go (4)
76-89: Expanded return values
You're capturing additional override maps fromProcessYAMLConfigFile. This should help unify override handling.
439-476: Ensuring overrides pass through
The function call now passes all override parameters, forming a proper override chain. Keep an eye on performance if there's heavy merging in large configs.
488-505: Terraform and Helmfile overrides merging
Merging inline and imported overrides is a strong approach. Just confirm these merges do not generate unintended key collisions.
541-548: Return statements handle new parameters
The updated return logic appropriately includes the final override maps. No issues spotted.go.mod (32)
3-3: Bump Go Version to 1.24.0
The Go version was updated to 1.24.0. This helps leverage improvements in the toolchain but be sure to run the full test suite to catch any subtle compatibility issues.
11-13: Update AWS SDK Core and Modules
The AWS SDK dependencies (core, config, and SSM service) have been bumped to v1.36.3, v1.29.9, and v1.57.2 respectively. Verify that these versions align with your AWS integrations and that no breaking changes affect your functionality.
16-16: Upgrade Charm Bubbletea Package
Thebubbleteapackage has been updated to v1.3.4. Check the release notes to ensure there are no breaking API changes that might affect the UI behavior.
25-25: Update go-git Dependency
Thego-gitpackage version was updated to v5.14.0, a minor bump that likely includes improvements and bug fixes. It’s a good idea to verify any git-related workflows.
38-38: Bump JFrog Client Version
Thejfrog-client-goversion has been updated to v1.51.0. Ensure that any functionality relying on JFrog services works as expected with this new version.
42-42: Update Bubblezone Module
The commit forbubblezonehas been updated to a newer reference (v0.0.0-20250301021021-ab7b445e9861). Confirm that this version is well-tested in your UI or CLI contexts.
47-47: Upgrade Termenv Dependency
Thetermenvpackage was bumped to v0.16.0. This should bring newer terminal styling features without breaking existing functionality.
48-48: Update OPA Version
The Open Policy Agent (OPA) dependency has been updated to v1.2.0. Please double-check that any policy evaluations or integrations remain consistent with your configuration handling.
51-51: Bump go-redis Version
Theredis/go-redis/v9dependency was updated to v9.7.1. Ensure that this change does not introduce compatibility issues with your Redis integrations.
60-60: Update OAuth2 Package
Thegolang.org/x/oauth2package was bumped to v0.28.0. This update likely contains improvements and security patches. It’s worth running authentication integration tests after this change.
61-61: Upgrade x/term Package
Thegolang.org/x/termpackage was updated to v0.30.0. This is a minor update—just confirm that terminal input/output operations still behave as expected.
62-62: Bump x/text Dependency
Thegolang.org/x/textpackage was updated to v0.23.0. This should bring enhancements in text processing; verifying non-regression in i18n or formatting is a good step.
66-66: Update mvdan.cc/sh Version
Themvdan.cc/sh/v3dependency was bumped to v3.11.0. If your project makes use of shell parsing/formatting, please run the related tests to confirm everything works as before.
83-83: Update Agnivade Levenshtein Library
Theagnivade/levenshteinindirect dependency is now at v1.2.1. This minor bump should improve text distance calculations without introducing breaking changes.
96-96: Bump AWS Credentials Package
Theaws-sdk-go-v2/credentialsdependency was updated to v1.17.62. Confirm that your AWS authentication flows remain secure and functional.
97-97: Update EC2 IMDS Feature
Theaws-sdk-go-v2/feature/ec2/imdspackage was bumped to v1.16.30. Validate that fetching instance metadata works as expected after the update.
99-99: Upgrade AWS Internal Config Sources
The internal configuration sources dependency was updated to v1.3.34. As this is an internal package of the AWS SDK, testing any configuration load paths is recommended.
100-100: Update AWS Endpoints Package
Theaws-sdk-go-v2/internal/endpoints/v2package now at v2.6.34 should ensure better endpoint resolution. Make sure that your AWS service calls resolve endpoints correctly.
105-105: Bump Presigned-URL Internal Package
Theaws-sdk-go-v2/service/internal/presigned-urldependency was updated to v1.12.15. Verify that URL signing and pre-signed operations still behave as expected.
108-108: Upgrade AWS SSO Service Package
Theaws-sdk-go-v2/service/ssopackage was updated to v1.25.1. Double-check that any Single Sign-On integrations continue to function correctly.
109-109: Update AWS SSO OIDC Service
Theaws-sdk-go-v2/service/ssooidcdependency is now at v1.29.1. Ensure that OIDC-based SSO flows are thoroughly tested with this update.
110-110: Bump AWS STS Dependency
Theaws-sdk-go-v2/service/stsdependency was updated to v1.33.17. This change should improve stability in security token services; testing the STS integration is advised.
128-128: Update Containerd Dependency
Thecontainerd/containerdindirect dependency was bumped to v1.7.26. Verify that any functionalities depending on container runtime interactions remain unaffected.
166-166: Bump go-cmp Version
Thegoogle/go-cmpdependency was updated to v0.7.0. This should enhance the diffing and comparison capabilities in tests, so ensure your test assertions behave as expected.
244-244: Update Prometheus Client Library
Theprometheus/client_golangdependency has been updated to v1.21.0. Check that custom metrics and scraping integrations continue to work seamlessly.
246-246: Upgrade Prometheus Common Package
Theprometheus/commondependency was bumped to v0.62.0. This should provide better support for Prometheus integrations—verify your monitoring dashboards if applicable.
289-289: Update go4.org Unsafe GC Assumption Package
Thego4.org/unsafe/assume-no-moving-gcdependency was updated to a more recent commit. Given its low-level nature, ensure that any performance-sensitive areas that depend on GC behaviors are adequately tested.
291-291: Bump x/crypto Package
Thegolang.org/x/cryptodependency was updated to v0.35.0. This update likely includes important security fixes; reviewing the release notes for any breaking changes is recommended.
294-294: Upgrade x/net Package
Thegolang.org/x/netdependency was bumped to v0.35.0. Ensure that any network communication or proxy functionalities continue to work as expected.
295-295: Update x/sync Package
Thegolang.org/x/syncdependency was updated to v0.12.0. This minor version bump should provide some performance improvements with low risk.
296-296: Bump x/sys Package
Thegolang.org/x/sysdependency is now at v0.31.0. Check that any system-level calls or integrations remain stable after this update.
297-297: Upgrade x/time Package
Thegolang.org/x/timedependency was updated to v0.10.0. This update is generally non-disruptive but verify that any time-based utilities used in your code still operate correctly.website/docs/core-concepts/share-data/share-data.mdx (2)
63-65: Documentation Update for!storeActionCard
The updated ActionCard block for the!storeYAML function is clear and directs the reader to the detailed documentation, which is in line with the revised behavior in overrides.
67-97: New!terraform.outputYAML Function Section
The new section on the!terraform.outputfunction is well-written and provides clear examples for both accessing outputs from the current stack and from a specified stack. This addition not only improves clarity for users but also aligns with the updated behavior objectives in override handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1122 +/- ##
==========================================
- Coverage 17.44% 16.44% -1.01%
==========================================
Files 170 170
Lines 18764 18789 +25
==========================================
- Hits 3274 3090 -184
- Misses 14892 15107 +215
+ Partials 598 592 -6
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cmd/support_test.go (2)
15-15: Consider handling the error from os.Pipe()This line ignores the potential error returned from os.Pipe(). While this is common in test code, it's generally better to assert or handle errors properly.
-r, w, _ := os.Pipe() +r, w, err := os.Pipe() +assert.NoError(t, err, "Failed to create pipe for stdout capture")
20-20: Use distinct error messages for different assertion pointsThe same error message is used for three different assertions. Using distinct messages would make it clearer which assertion failed when troubleshooting test failures.
-assert.NoError(t, err, "'atmos support' command should execute without error") +assert.NoError(t, err, "'atmos support' command execution failed") -assert.NoError(t, err, "'atmos support' command should execute without error") +assert.NoError(t, err, "Failed to close stdout pipe writer") -assert.NoError(t, err, "'atmos support' command should execute without error") +assert.NoError(t, err, "Failed to read captured stdout")Also applies to: 24-24, 31-31
cmd/about_test.go (3)
15-16: Check error from os.Pipe() callThe error return value from
os.Pipe()is discarded with_, but should be checked to ensure the pipe was created successfully.-r, w, _ := os.Pipe() +r, w, err := os.Pipe() +assert.NoError(t, err, "Failed to create pipe for stdout capture")
24-24: Use a more specific error messageThe error message is identical to the one used in line 20, but it's for a different operation (closing the writer vs. executing the command).
-assert.NoError(t, err, "'atmos about' command should execute without error") +assert.NoError(t, err, "Failed to close the stdout pipe writer")
31-31: Use a more specific error messageSimilarly, this error message could be more specific about the operation being performed.
-assert.NoError(t, err, "'atmos about' command should execute without error") +assert.NoError(t, err, "Failed to read captured output from pipe")internal/exec/workflow.go (1)
128-128: Fix dynamic error construction to address linter warning.The change addresses the
err113linter warning by usingfmt.Errorf("%s", errorMarkdown)instead of directly passingerrorMarkdownto the error.While this change addresses the linter warning, consider using
errors.New(errorMarkdown)instead for cleaner error creation when no formatting is needed:-return fmt.Errorf("%s", errorMarkdown) +return errors.New(errorMarkdown)🧰 Tools
🪛 GitHub Check: golangci
[failure] 128-128: [golangci] internal/exec/workflow.go#L128
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("%s", errorMarkdown)" (err113)
🛑 Comments failed to post (1)
internal/exec/vendor_utils.go (1)
136-137: 💡 Verification agent
🧩 Analysis chain
Consider using wrapped static errors instead of dynamic error messages
The static analysis tool flagged this pattern as it violates the err113 rule. For better error handling, consider defining static errors as constants and wrapping them with additional context.
🏁 Script executed:
#!/bin/bash # Search for other similar error patterns in the codebase to understand the project's error handling conventions echo "Searching for error handling patterns in the codebase..." rg -A 1 -B 1 "var Err[A-Z][a-zA-Z0-9]+ = errors\.New\(" --type go echo "Searching for uses of fmt.Errorf..." rg "fmt\.Errorf\(" --type go | head -n 10Length of output: 1411
Attention: Refactor to Employ Wrapped Static Errors
It looks like our project already standardizes error handling by defining static error constants (e.g., in
internal/exec/terraform.go). To comply with the err113 rule and improve consistency, please replace the dynamic error construction ininternal/exec/vendor_utils.go(lines 136–137) with a wrapped static error.
Actionable Change:
Define a package-level error constant—something like:var ErrMissingComponentFlag = errors.New("to vendor a component, the '--component' (shorthand '-c') flag needs to be specified. Example: atmos vendor pull -c <component>")Then update the error return to wrap this constant:
- return fmt.Errorf("to vendor a component, the '--component' (shorthand '-c') flag needs to be specified.\n"+ - "Example: atmos vendor pull -c <component>\n%s", q) + return fmt.Errorf("%w\n%s", ErrMissingComponentFlag, q)This change will align error handling with the conventions already present in the codebase and address the static analysis concerns.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 136-136: [golangci] internal/exec/vendor_utils.go#L136
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("to vendor a component, the '--component' (shorthand '-c') flag needs to be specified.\n"+\n\t"Example: atmos vendor pull -c \n%s", q)" (err113)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml (1)
10-10: Remove trailing whitespaceThere's a trailing space at the end of this line that should be removed.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
internal/exec/validate_stacks_test.go (1)
26-44: Consider extracting test configuration setup to helper functionAs you add more tests for different override section scenarios, this configuration setup might be reused. Consider extracting it to a helper function to reduce duplication in future tests.
tests/fixtures/scenarios/atmos-overrides-section/stacks/workflows/workflows.yaml (1)
20-32: Consider using workflow composition for DRY principlesThe
show-all-describe-component-commandsworkflow repeats the same pattern with only environment differences. If Atmos supports workflow composition or parameterization, you could make this more maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/support_test.go(1 hunks)cmd/workflow_test.go(1 hunks)internal/exec/validate_stacks_test.go(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-overrides-section/stacks/workflows/workflows.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (8)
cmd/support_test.go (1)
12-35: Good implementation of test for the support commandThe test correctly captures stdout, executes the command, and validates output. The approach of using pipes to capture output is a robust pattern for testing CLI commands that write to stdout.
A few observations:
- Good error handling at each step
- Clear assertions with descriptive messages
- Proper cleanup by restoring stdout
cmd/workflow_test.go (4)
12-24: Well-structured test setup for workflow commandThe test properly sets up the environment variables needed for the workflow command test. Using a relative path to test fixtures is a good practice for maintainability.
26-31: Clear definition of expected outputThe expected output is well-defined as a multi-line string, making it clear what the test is checking for. This makes the test intention obvious.
33-36: Effective command execution patternUsing
RootCmd.SetArgsfollowed byExecute()properly simulates how the command would be run in production, which is better than testing internal methods directly.
38-51: Good output validation approachThe output capture, restoration, and validation follow best practices for CLI testing. The assertion clearly expresses the expected behavior.
tests/fixtures/scenarios/atmos-overrides-section/atmos.yaml (1)
1-25: Clean configuration structure with good organizationThis Atmos configuration file establishes a solid foundation for the overrides section testing scenario with proper path configurations and logical organization of components, stacks, logs, and workflows.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
internal/exec/validate_stacks_test.go (1)
12-48: Well-structured test for stack validationThe test effectively validates stacks with overrides section configurations across multiple environments. Good use of filepath handling and clear configuration setup.
tests/fixtures/scenarios/atmos-overrides-section/stacks/workflows/workflows.yaml (1)
1-32: Comprehensive workflow definitions for testing overrides behaviorThe workflows provide good coverage for testing component behavior across multiple environments, which will help validate the changes to the overrides section functionality.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (7)
internal/exec/stack_processor_utils_test.go (2)
13-58: Test looks solid, consider documenting parameters for improved readabilityThe test provides good coverage for the
ProcessYAMLConfigFilefunction, ensuring it produces the expected structure. However, the numerous parameters in the function call (lines 31-46) could use comments explaining their purpose, especially since most are set to nil or false.Consider adding parameter documentation:
_, _, stackConfigMap, _, _, _, _, err := ProcessYAMLConfigFile( - atmosConfig, - stacksBasePath, - filePath, - map[string]map[string]any{}, - nil, - false, - false, - true, - false, - nil, - nil, - nil, - nil, - "", + atmosConfig, // Atmos configuration + stacksBasePath, // Base path for stacks + filePath, // Path to the YAML file to process + map[string]map[string]any{}, // Additional configurations + nil, // Inline Terraform overrides + false, // Skip template processing + false, // Skip template functions processing + true, // Process component vars + false, // Show template processing + nil, // Imported Terraform overrides + nil, // Inline Helmfile overrides + nil, // Imported Helmfile overrides + nil, // Context for template processing + "", // Component type
48-57: Consider enhancing assertions to validate map contentThe test currently verifies the map keys exist, but doesn't check the actual content of these map entries. Adding assertions for the expected values would improve test coverage.
assert.Equal(t, "components", mapResultKeys[0]) assert.Equal(t, "import", mapResultKeys[1]) assert.Equal(t, "vars", mapResultKeys[2]) + + // Verify the content of the map entries + componentsMap, ok := stackConfigMap["components"].(map[string]interface{}) + assert.True(t, ok, "components should be a map") + // Add assertions for specific component entriescmd/workflow_test.go (2)
26-31: Consider using a constant or extracting this to a shared test utilThis expected output string is duplicated in internal/exec/workflow_test.go. Consider extracting it to a shared constant or test utility to avoid duplication.
Move this to a shared test utility file that both tests can import.
49-50: Use more specific assertion to improve test clarityThe Contains assertion is useful but provides limited feedback. Consider using a more specific assertion that clearly shows what was expected vs. received.
- assert.Contains(t, output.String(), expectedOutput, "'atmos workflow' output should contain information about workflows") + // Normalize whitespace in output for comparison + normalizedOutput := strings.TrimSpace(output.String()) + normalizedExpected := strings.TrimSpace(expectedOutput) + assert.Equal(t, normalizedExpected, normalizedOutput, "'atmos workflow' output should match expected commands")internal/exec/workflow_test.go (3)
40-63: Consider extracting workflow definition to test fixtureThe hardcoded workflow definition would be better placed in a test fixture file, making the test more maintainable and focused on test logic rather than test data.
Move this workflow definition to a YAML fixture file and load it during the test, similar to how you're using other fixtures in this PR.
80-80: Fix typo in assertion messageThere's a minor typo in the assertion message - missing "about" between "information" and "workflows".
- assert.Contains(t, output.String(), expectedOutput, "'ExecuteWorkflow' output should contain information workflows") + assert.Contains(t, output.String(), expectedOutput, "'ExecuteWorkflow' output should contain information about workflows")
15-81: Reduce duplication with the TestWorkflowCmd testThis test has significant duplication with the cmd/workflow_test.go test. Consider extracting common setup and assertion code to shared test utilities.
Create a shared test utility package with functions for:
- Setting up and tearing down environment variables
- Capturing and restoring stdout
- Defining common expected outputs
- Common assertion helpers
This would make both tests more concise and maintainable.
🛑 Comments failed to post (2)
cmd/workflow_test.go (1)
15-19: 🛠️ Refactor suggestion
Remember to restore environment variables after test
Environment variables are set but not restored, which could affect other tests running in the same process. Consider using defer to restore them.
err := os.Setenv("ATMOS_CLI_CONFIG_PATH", stacksPath) assert.NoError(t, err, "Setting 'ATMOS_CLI_CONFIG_PATH' environment variable should execute without error") + oldCliConfigPath := os.Getenv("ATMOS_CLI_CONFIG_PATH") + defer os.Setenv("ATMOS_CLI_CONFIG_PATH", oldCliConfigPath) err = os.Setenv("ATMOS_BASE_PATH", stacksPath) assert.NoError(t, err, "Setting 'ATMOS_BASE_PATH' environment variable should execute without error") + oldBasePath := os.Getenv("ATMOS_BASE_PATH") + defer os.Setenv("ATMOS_BASE_PATH", oldBasePath)Committable suggestion skipped: line range outside the PR's diff.
internal/exec/workflow_test.go (1)
19-23: 🛠️ Refactor suggestion
Remember to restore environment variables after test
Environment variables are set but not restored, which could affect other tests running in the same process. Consider using defer to restore them.
err := os.Setenv("ATMOS_CLI_CONFIG_PATH", stacksPath) assert.NoError(t, err, "Setting 'ATMOS_CLI_CONFIG_PATH' environment variable should execute without error") + oldCliConfigPath := os.Getenv("ATMOS_CLI_CONFIG_PATH") + defer os.Setenv("ATMOS_CLI_CONFIG_PATH", oldCliConfigPath) err = os.Setenv("ATMOS_BASE_PATH", stacksPath) assert.NoError(t, err, "Setting 'ATMOS_BASE_PATH' environment variable should execute without error") + oldBasePath := os.Getenv("ATMOS_BASE_PATH") + defer os.Setenv("ATMOS_BASE_PATH", oldBasePath)Committable suggestion skipped: line range outside the PR's diff.
what
overridessection behavior in imports and inlineoverridessection for many scenarios (overridesat the stack level, team level, inline and in imports)Goto1.24.0and fix some issues in the code (1.24.0is more strict about theGoformat functions always requiring theformatas the first argument)why
overridesaffected the imported components, but the inlineoverridesfor the same stack were not taken into account). This PR fixes this and adds acceptance tests for all scenarios!terraform.outputYAML function can (and should be) used to to read the outputs (remote state) of components directly in Atmos stack manifestsSummary by CodeRabbit
Summary by CodeRabbit
New Features
!terraform.outputfor improved data sharing in stack manifests.Documentation
Chores
Tests
aboutCmdandsupportCmdcommands to ensure correct output.ProcessYAMLConfigFile,ValidateStacks, andworkflowcommand to enhance coverage.