Configure YAML formatting in atmos describe commands#1180
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces CLI configuration capabilities to the file output functions and adds logic to optionally skip empty sections in the describe commands. Key changes include:
- Adding functions (PrintAsYAMLWithConfig and ConvertToYAML with options) to support custom YAML formatting.
- Introducing new CLI config elements (DescribeSettings, Describe) and a TabWidth option in the Terminal settings.
- Adjusting various describe command functions to pass the CLI configuration to output functions.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/yaml_utils.go | Added custom YAML output functions and support for configurable indentation options. |
| pkg/schema/schema.go | Introduced DescribeSettings and Describe; added TabWidth to Terminal settings. |
| internal/exec/file_utils.go | Modified printOrWriteToFile signature and logic to use atmosConfig and YAML options. |
| internal/exec/describe_workflows.go | Updated printOrWriteToFile calls to pass atmosConfig. |
| internal/exec/describe_stacks.go | Updated printOrWriteToFile calls and added logic to optionally skip empty sections. |
| internal/exec/describe_dependents.go | Updated printOrWriteToFile calls to pass atmosConfig. |
| internal/exec/describe_config.go | Updated printOrWriteToFile calls to pass atmosConfig. |
| internal/exec/describe_component.go | Updated variable ordering and printOrWriteToFile calls to pass atmosConfig. |
| internal/exec/describe_affected.go | Updated printOrWriteToFile calls to pass atmosConfig and CLIConfig consistency. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
+ Coverage 48.24% 48.43% +0.19%
==========================================
Files 232 233 +1
Lines 25329 25428 +99
==========================================
+ Hits 12219 12316 +97
+ Misses 11516 11505 -11
- Partials 1594 1607 +13
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.
Pull Request Overview
This PR enhances YAML output functionality by introducing CLI configuration for YAML printing, handling nil configurations, and skipping empty sections in describe commands.
- Adds a configurable indentation option and refactors YAML conversion to accept custom options.
- Updates multiple functions to pass pointers for configuration and adds error handling for nil configurations.
- Introduces new Describe settings in the schema for controlling empty section inclusion.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/utils/yaml_utils.go | Refactors YAML conversion to use encoder with optional indentation. |
| pkg/schema/schema.go | Adds Describe and DescribeSettings types for CLI config. |
| internal/exec/* | Updates functions to pass a pointer for CLI config and adds nil checks. |
Files not reviewed (1)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden: Language not supported
Comments suppressed due to low confidence (2)
internal/exec/describe_stacks.go:421
- [nitpick] Consider updating or removing the placeholder comment ("pending Erik accept") to clarify the intended behavior for including empty sections. Use a clear TODO or remove it if no longer needed.
includeEmpty := true // Default to true if setting is not provided // pending Erik accept
internal/exec/describe_affected.go:317
- Ensure consistent use of the CLI configuration pointer in this function; if both calls are meant to use the same configuration, consider using '&a.CLIConfig' for both printOrWriteToFile calls.
err = printOrWriteToFile(&atmosConfig, a.Format, a.OutputFile, res)
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
📝 Walkthrough""" WalkthroughThe changes introduce configurable YAML indentation and the ability to exclude empty sections in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant FilterUtils
participant YAMLUtils
User->>CLI: Run describe command
CLI->>Config: Load AtmosConfiguration
CLI->>FilterUtils: Get include_empty setting
CLI->>FilterUtils: FilterEmptySections(data, include_empty)
FilterUtils-->>CLI: Filtered data
CLI->>YAMLUtils: Get tab_width from config
CLI->>YAMLUtils: ConvertToYAML(filtered data, indent=tab_width)
YAMLUtils-->>CLI: YAML string
CLI->>User: Output YAML
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 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 (2)
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
♻️ Duplicate comments (1)
internal/exec/file_utils.go (1)
52-56:⚠️ Potential issueFile permissions could be more restrictive
When writing files that might contain sensitive configuration data, it's best to use more restrictive permissions.
- err = os.WriteFile(file, []byte(y), 0o644) + err = os.WriteFile(file, []byte(y), 0o600)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-55: internal/exec/file_utils.go#L54-L55
Added lines #L54 - L55 were not covered by tests
🧹 Nitpick comments (7)
internal/exec/describe_stacks.go (1)
421-426: ClarifyIncludeEmptydefault handling.The logic defaults
includeEmptyto true ifatmosConfig.Describe.Settings.IncludeEmptyis nil. Confirm that this behavior aligns with user expectations and is tested to prevent surprises.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 421-424: internal/exec/describe_stacks.go#L421-L424
Added lines #L421 - L424 were not covered by testspkg/utils/yaml_utils.go (2)
30-41: Expose additional tags if needed.Adding
AtmosYamlTagsandErrNilAtmosConfigis helpful. Consider documentingAtmosYamlTagsusage in your README or wiki to guide future maintainers.
49-73: Encourage test coverage on indentation logic.
PrintAsYAMLWithConfigreadsTabWidthfromatmosConfig.Settings.Terminal; coverage ensures that unexpected zero or negative values default to 2. Please add tests for these edge cases.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 69-69: error is not nil (line 65) but it returns nil
(nilerr)
🪛 GitHub Check: codecov/patch
[warning] 52-53: pkg/utils/yaml_utils.go#L52-L53
Added lines #L52 - L53 were not covered by testspkg/describe/describe_stacks_test.go (4)
269-299: Good approach for total section counting.
countSectionshelps validate empty-section filtering. Consider also counting sections for Helmfile components for completeness if applicable.
300-324: Restrict repeated lookups.
countComponentSectionsrepeatedly checks map presence. Minor efficiency could be gained by short-circuiting if any submap is nil. Optional improvement.
338-359: Solid test for stack-level filtering.
TestDescribeStacksWithEmptySectionFilteringAllStacksverifies total sections. Possibly add a negative test verifying no sections appear if everything is empty.
360-383: Great coverage for component filtering.
TestDescribeStacksWithEmptySectionFilteringComponentis similarly thorough. Adding more varied components could better stress-test the logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
internal/exec/describe_affected.go(2 hunks)internal/exec/describe_component.go(2 hunks)internal/exec/describe_config.go(1 hunks)internal/exec/describe_dependents.go(1 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/describe_workflows.go(1 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/file_utils_test.go(1 hunks)internal/exec/helmfile.go(1 hunks)internal/exec/helmfile_generate_varfile.go(1 hunks)internal/exec/terraform.go(2 hunks)internal/exec/terraform_generate_varfile.go(1 hunks)internal/exec/utils.go(1 hunks)internal/exec/workflow_utils.go(1 hunks)pkg/describe/describe_stacks_test.go(1 hunks)pkg/schema/schema.go(3 hunks)pkg/schema/schema_test.go(1 hunks)pkg/utils/yaml_utils.go(6 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_stacks_with_relative_paths.stdout.golden(1 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
internal/exec/workflow_utils.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/terraform.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/terraform_generate_varfile.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/helmfile.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/helmfile_generate_varfile.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/describe_component.go (2)
pkg/config/config.go (1)
InitCliConfig(25-62)pkg/schema/schema.go (1)
ConfigAndStacksInfo(375-443)
internal/exec/utils.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAMLToFileDescriptor(76-87)
internal/exec/file_utils.go (2)
pkg/schema/schema.go (3)
AtmosConfiguration(23-56)Settings(717-721)Terminal(200-207)pkg/utils/yaml_utils.go (3)
YAMLOptions(103-105)PrintAsYAMLWithConfig(50-73)ConvertToYAML(108-125)
pkg/utils/yaml_utils.go (3)
pkg/utils/config_utils.go (1)
ExtractAtmosConfig(8-17)pkg/schema/schema.go (3)
AtmosConfiguration(23-56)Settings(717-721)Terminal(200-207)pkg/utils/highlight_utils.go (1)
HighlightCodeWithConfig(75-147)
🪛 GitHub Check: codecov/patch
internal/exec/describe_config.go
[warning] 45-45: internal/exec/describe_config.go#L45
Added line #L45 was not covered by tests
internal/exec/terraform.go
[warning] 181-181: internal/exec/terraform.go#L181
Added line #L181 was not covered by tests
internal/exec/terraform_generate_varfile.go
[warning] 80-80: internal/exec/terraform_generate_varfile.go#L80
Added line #L80 was not covered by tests
internal/exec/helmfile.go
[warning] 89-89: internal/exec/helmfile.go#L89
Added line #L89 was not covered by tests
internal/exec/helmfile_generate_varfile.go
[warning] 65-65: internal/exec/helmfile_generate_varfile.go#L65
Added line #L65 was not covered by tests
internal/exec/describe_component.go
[warning] 74-77: internal/exec/describe_component.go#L74-L77
Added lines #L74 - L77 were not covered by tests
[warning] 90-90: internal/exec/describe_component.go#L90
Added line #L90 was not covered by tests
internal/exec/utils.go
[warning] 320-320: internal/exec/utils.go#L320
Added line #L320 was not covered by tests
internal/exec/describe_affected.go
[warning] 274-274: internal/exec/describe_affected.go#L274
Added line #L274 was not covered by tests
[warning] 317-317: internal/exec/describe_affected.go#L317
Added line #L317 was not covered by tests
internal/exec/file_utils.go
[warning] 54-55: internal/exec/file_utils.go#L54-L55
Added lines #L54 - L55 were not covered by tests
internal/exec/describe_dependents.go
[warning] 79-79: internal/exec/describe_dependents.go#L79
Added line #L79 was not covered by tests
internal/exec/describe_stacks.go
[warning] 139-139: internal/exec/describe_stacks.go#L139
Added line #L139 was not covered by tests
[warning] 421-424: internal/exec/describe_stacks.go#L421-L424
Added lines #L421 - L424 were not covered by tests
[warning] 428-433: internal/exec/describe_stacks.go#L428-L433
Added lines #L428 - L433 were not covered by tests
internal/exec/describe_workflows.go
[warning] 79-79: internal/exec/describe_workflows.go#L79
Added line #L79 was not covered by tests
pkg/utils/yaml_utils.go
[warning] 52-53: pkg/utils/yaml_utils.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 76-79: pkg/utils/yaml_utils.go#L76-L79
Added lines #L76 - L79 were not covered by tests
[warning] 225-226: pkg/utils/yaml_utils.go#L225-L226
Added lines #L225 - L226 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (43)
tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden (1)
3-63: Indentation formatting looks consistentThe changes show a uniform indentation style across all YAML sections. This aligns with the PR objective to standardize YAML formatting.
tests/snapshots/TestCLICommands_atmos_stacks_with_relative_paths.stdout.golden (1)
2-96: Consistent YAML indentation across all environment sectionsThe YAML structure maintains uniform indentation across all components sections for dev, prod, and staging environments. This improves readability and follows best practices.
tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
3-166: Unified indentation style across configuration sectionsThe formatting changes maintain consistent indentation across all configuration sections. This standardization improves readability without altering functionality.
website/docs/cli/configuration/terminal.mdx (1)
33-33: Documentation added for the new tab_width settingThe new
tab_widthconfiguration option is properly documented with a clear explanation of its purpose and default value. This helps users understand how to customize YAML indentation.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
119-121: New "describe" configuration section addedA new section for "describe" settings has been added to the configuration structure. This sets up the foundation for the configurable YAML formatting features introduced in this PR.
internal/exec/describe_config.go (1)
45-45: Looking good - pointer passing for configuration control.The change from passing a format string to passing a pointer to the entire configuration structure allows for more fine-grained control over output formatting based on user preferences. This aligns with the PR objective of enabling configurable indentation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: internal/exec/describe_config.go#L45
Added line #L45 was not covered by testsinternal/exec/helmfile_generate_varfile.go (1)
65-65: Correctly updated to pass configuration by reference.This change matches the updated function signature in
PrintAsYAMLToFileDescriptorthat now expects a pointer toatmosConfig, ensuring consistent formatting control across the application.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-65: internal/exec/helmfile_generate_varfile.go#L65
Added line #L65 was not covered by teststests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (8)
3-12: Consistent indentation improves YAML readability.The updated YAML formatting with consistent indentation provides better readability while maintaining the same data structure.
15-21: Uniformly indented YAML structure.The consistent indentation in the helmfile section matches the formatting applied to other sections, creating a more unified appearance.
23-29: Standardized indentation in stacks section.The indentation changes here maintain consistency with the rest of the document.
31-32: Log section formatting updated for consistency.The log section now follows the same indentation pattern as other sections.
34-35: Settings section properly aligned.Indentation consistency applied to the settings section.
39-39: Path array elements consistently indented.The includeStackAbsolutePaths array elements follow the same indentation pattern.
41-41: Exclude paths follow consistent formatting.The excludeStackAbsolutePaths array elements are properly indented.
46-47: Editorconfig section formatted with consistent style.The validate section maintains the consistent indentation pattern established throughout the document.
internal/exec/workflow_utils.go (1)
46-46: Pointer to config passed correctly.Changed to pass a pointer to
atmosConfiginstead of the value directly, matching the updated signature ofPrintAsYAMLToFileDescriptorwhich now accepts a pointer to allow for configuration-driven formatting.internal/exec/terraform.go (2)
132-132: Code updated to pass atmosConfig by referenceThe change from
atmosConfigto&atmosConfigupdates the argument to match the updated function signature that now expects a pointer to AtmosConfiguration.
181-181:Details
✅ Verification successful
Code updated to pass atmosConfig by reference
Similar to the previous change, this updates the call to use a pointer to atmosConfig to match the updated function signature.
🏁 Script executed:
#!/bin/bash # Verify the signature of PrintAsYAMLToFileDescriptor to confirm it requires a pointer rg -A 3 "func PrintAsYAMLToFileDescriptor" pkg/utils/yaml_utils.goLength of output: 214
Action: Code update verified – atmosConfig is now correctly passed by pointer.
The change in
internal/exec/terraform.gonow aligns perfectly with the function signature inpkg/utils/yaml_utils.go(i.e.,func PrintAsYAMLToFileDescriptor(atmosConfig *schema.AtmosConfiguration, data any) error). The update to pass&atmosConfigis accurate and consistent with the expectations set by the function definition.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-181: internal/exec/terraform.go#L181
Added line #L181 was not covered by testsinternal/exec/terraform_generate_varfile.go (1)
80-80: Code updated to pass atmosConfig by referenceThis change modifies the argument to match the updated function signature that now expects a pointer to AtmosConfiguration. The change ensures code consistency with the revisions to the PrintAsYAMLToFileDescriptor function.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-80: internal/exec/terraform_generate_varfile.go#L80
Added line #L80 was not covered by testsinternal/exec/helmfile.go (1)
89-89: Code updated to pass atmosConfig by referenceThis change aligns with the updated PrintAsYAMLToFileDescriptor function signature. Using a pointer allows the function to access the YAML formatting settings from the configuration, supporting the configurable indentation feature mentioned in the PR objectives.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-89: internal/exec/helmfile.go#L89
Added line #L89 was not covered by testsinternal/exec/utils.go (1)
320-320: Code updated to pass atmosConfig by referenceThis change mirrors the other updates, ensuring the parameter is passed by reference to match the revised function signature. This is part of the broader change to enable configurable YAML formatting through atmos.yaml.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 320-320: internal/exec/utils.go#L320
Added line #L320 was not covered by testsinternal/exec/describe_workflows.go (1)
79-79:Details
✅ Verification successful
Function modified to use configuration object instead of raw format string
The change from passing the format string directly to passing the full configuration object gives the function access to YAML formatting settings like tab width and component display preferences.
🏁 Script executed:
#!/bin/bash # Verify the signature of printOrWriteToFile has been updated to accept the atmosConfig parameter grep -A 5 "func printOrWriteToFile" internal/exec/file_utils.goLength of output: 182
Verified: Configuration Object Passed to printOrWriteToFile
The function signature now accepts the full configuration object (atmosConfig *schema.AtmosConfiguration) as intended. The code in internal/exec/describe_workflows.go (line 79) is correctly updated to call the function with the configuration object, ensuring YAML formatting settings (like tab width and component display preferences) are properly handled.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-79: internal/exec/describe_workflows.go#L79
Added line #L79 was not covered by testsinternal/exec/describe_dependents.go (1)
79-79: Function signature updated to pass configuration settingsThis change aligns with the PR objective of making YAML formatting configurable through
atmos.yamlsettings. The function now has access to tab width and empty component display preferences.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-79: internal/exec/describe_dependents.go#L79
Added line #L79 was not covered by testsinternal/exec/describe_component.go (2)
74-78: Configuration initialization moved outside conditional blockGood change - moving the atmosConfig initialization before the query conditional ensures it's always available regardless of whether a query is provided.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-77: internal/exec/describe_component.go#L74-L77
Added lines #L74 - L77 were not covered by tests
90-90: Function signature updated to pass configuration pointerConsistent with changes in other files, now passing the full config object rather than just format string.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: internal/exec/describe_component.go#L90
Added line #L90 was not covered by testspkg/schema/schema_test.go (1)
40-72: Test added for new configuration optionsWell-structured test that validates both TabWidth and IncludeEmpty settings. The test covers:
- Direct struct creation with TabWidth=4
- IncludeEmpty=false validation
- IncludeEmpty=true validation
- Complete AtmosConfiguration with both settings
This provides good coverage for the new configuration options introduced in this PR.
internal/exec/file_utils_test.go (1)
13-68: Good test coverage, well structured test casesThe test file provides thorough testing of the
printOrWriteToFilefunction with different scenarios including:
- Writing YAML and JSON to stdout
- Writing to temporary files with proper cleanup
- Error handling for invalid formats
- Testing default behavior when TabWidth is 0
The test initializes the configuration properly with a custom tab width of 4 and verifies all expected outcomes.
internal/exec/describe_affected.go (2)
274-274:Details
❓ Verification inconclusive
Function signature update correctly implemented
The code correctly passes a pointer to the configuration structure rather than just the format string, aligning with the updated function signature in file_utils.go.
🏁 Script executed:
#!/bin/bash # Check for test coverage of this line which static analysis flagged as untested grep -r "ExecuteDescribeAffectedCmd.*test" --include="*.go" .Length of output: 63
Function signature update correctly implemented; please confirm test coverage manually
The updated call to
printOrWriteToFilewith a pointer to the configuration structure is correct and aligns with the signature change infile_utils.go. However, the grep check for tests coveringExecuteDescribeAffectedCmdreturned no results. Could you please verify manually that this functionality is covered by tests—or add appropriate tests if they're missing?
- Verify or add test coverage for
ExecuteDescribeAffectedCmd, especially around the call ininternal/exec/describe_affected.go:274.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 274-274: internal/exec/describe_affected.go#L274
Added line #L274 was not covered by tests
317-317:Details
❓ Verification inconclusive
Function signature update correctly implemented
The code correctly passes a pointer to the configuration structure rather than just the format string, maintaining consistency with the updated function signature in file_utils.go.
🏁 Script executed:
#!/bin/bash # Check if this code path is tested grep -r "atmosConfig.*printOrWriteToFile.*a.Query" --include="*.go" . | grep -v "describe_affected.go"Length of output: 102
Attention: Verify Test Coverage for Updated Function Signature
- The function signature update is correctly implemented. The updated call in
internal/exec/describe_affected.go(line 317) passes a pointer toatmosConfig, consistent with the changes infile_utils.go.- However, the grep command did not yield any results outside
describe_affected.go. This makes it unclear whether additional code paths invokingprintOrWriteToFileare covered by tests.- Please manually verify that the relevant code path is adequately exercised by the test suite.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 317-317: internal/exec/describe_affected.go#L317
Added line #L317 was not covered by testspkg/schema/schema.go (4)
14-16: Well-structured settings for describe command outputThe
DescribeSettingstype is properly defined with appropriate YAML, JSON, and mapstructure tags. Using a pointer to bool forIncludeEmptyis a good choice as it allows for three states (true, false, nil) which enables proper handling of unset values.
19-21: Clean implementation of Describe structureThe
Describetype follows the established pattern in the codebase, with appropriate serialization tags and a nested Settings field.
35-35: Configuration structure properly extendedThe
AtmosConfigurationstruct has been correctly extended with the newDescribefield, keeping consistent with existing patterns and properly tagged for serialization.
206-206: Tab width setting added to Terminal configurationGood addition of the
TabWidthfield to theTerminalstruct with appropriate serialization tags. Making it optional (withomitempty) allows for backward compatibility.internal/exec/file_utils.go (3)
33-33: Updated function signature with pointer parameterGood change to use a pointer to
schema.AtmosConfigurationrather than passing the entire structure by value, which would be inefficient for large structs.
40-44: Tab width handling with sensible defaultNice implementation to extract the tab width from configuration with a fallback to the default value of 2 when not specified or set to an invalid value (≤ 0).
47-47: Updated YAML printing to use configurationGood update to use the configuration-aware YAML printing function.
internal/exec/describe_stacks.go (1)
139-139: Validate coverage for the new pointer usage.Consider adding a test to confirm that
printOrWriteToFileis invoked with a valid pointer toatmosConfig, ensuring no regressions when the config is nil or modified at runtime.Would you like a script that checks for tests covering this line?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-139: internal/exec/describe_stacks.go#L139
Added line #L139 was not covered by testspkg/utils/yaml_utils.go (4)
4-5: Imports look good.No concerns about
bytesorerrorsusage.
43-48: Add coverage for the fallback logic.
PrintAsYAMLcallsExtractAtmosConfigand then delegates toPrintAsYAMLWithConfig. Include a test verifying correct behavior when data lacks config or is nil.
75-79: Check usage ofPrintAsYAMLToFileDescriptor.We see a nil-check and direct logging. Consider adding tests around scenarios like missing
atmosConfigor unusual file descriptors to ensure robust error-handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-79: pkg/utils/yaml_utils.go#L76-L79
Added lines #L76 - L79 were not covered by tests
225-226: Test nil config error path.
UnmarshalYAMLFromFilereturnsErrNilAtmosConfigifatmosConfig == nil. Confirm test coverage for that scenario, preventing unhandled edge cases.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 225-226: pkg/utils/yaml_utils.go#L225-L226
Added lines #L225 - L226 were not covered by testspkg/describe/describe_stacks_test.go (2)
325-337: Nice setup for test config.
setupEmptySectionFilteringTestis straightforward. If feasible, confirm it also handles partial initialization or override scenarios forincludeEmpty.
384-389: Clever aggregator test.
TestDescribeStacksWithEmptySectionFilteringcalls the other two tests, ensuring comprehensive coverage. This approach is maintainable and clear.
|
I think include empty should also filter out |
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
…-format # Conflicts: # internal/exec/describe_affected.go # internal/exec/describe_component.go # internal/exec/describe_config.go # internal/exec/file_utils.go # pkg/schema/schema.go # pkg/schema/schema_test.go # pkg/utils/yaml_utils.go # tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden # tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden # tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/utils/yaml_utils.go (2)
65-83:⚠️ Potential issueFix error handling in PrintAsYAMLWithConfig.
The function should return the error from HighlightCodeWithConfig instead of nil.
highlighted, err := HighlightCodeWithConfig(atmosConfig, y, "yaml") if err != nil { PrintMessage(y) - return nil + return err }🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 79-79: error is not nil (line 76) but it returns nil
(nilerr)
128-146: 💡 Verification agent🧩 Analysis chain
Secure the file path in logs.
Consider sanitizing the file path before logging to avoid exposing sensitive directory structures.
- log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", filePath) + log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", filepath.Base(filePath))
🏁 Script executed:
#!/bin/bash # Check for other similar logging patterns to ensure consistent sanitization grep -r "log.*filePath" --include="*.go" . | grep -v "filepath.Base"Length of output: 1360
Sanitize logged file paths across the codebase
To avoid exposing full directory structures in logs, please sanitize all
filePathfields (for example, usingfilepath.Base(filePath)or a shared helper) rather than logging the raw path.Affected locations include, but may not be limited to:
- internal/exec/tar_utils.go
- pkg/config/imports.go
- pkg/config/load.go
- pkg/config/load_config_args.go
- pkg/list/list_vendor.go
- pkg/utils/yaml_utils.go
Example diff for yaml_utils.go:
- log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", filePath) + log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", filepath.Base(filePath))Please apply a similar change (or introduce a helper like
sanitizePath) in each of the listed files to ensure consistency.
🧹 Nitpick comments (1)
pkg/utils/yaml_utils.go (1)
168-283: Consider refactoring processCustomTags function.The processCustomTags function has high cognitive complexity (77 > 20). Consider splitting it into smaller, more focused functions to improve maintainability.
You could extract the !include tag handling into a separate function:
func processIncludeTag(atmosConfig *schema.AtmosConfiguration, n *yaml.Node, val string, file string) error { // Extract the include tag handling logic // ... }Then simplify the main function:
func processCustomTags(atmosConfig *schema.AtmosConfiguration, node *yaml.Node, file string) error { // Document node handling... for _, n := range node.Content { tag := strings.TrimSpace(n.Tag) val := strings.TrimSpace(n.Value) if SliceContainsString(AtmosYamlTags, tag) { n.Value = getValueWithTag(n) } // Handle the !include tag if tag == AtmosYamlFuncInclude { if err := processIncludeTag(atmosConfig, n, val, file); err != nil { return err } } // Recursively process child nodes if len(n.Content) > 0 { if err := processCustomTags(atmosConfig, n, file); err != nil { return err } } } return nil }🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 168-168: cognitive complexity 77 of func
processCustomTagsis high (> 20)(gocognit)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/exec/describe_component.go(1 hunks)internal/exec/file_utils.go(3 hunks)pkg/schema/schema.go(3 hunks)pkg/schema/schema_test.go(1 hunks)pkg/utils/yaml_utils.go(7 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(2 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/docs/cli/configuration/terminal.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden
- internal/exec/describe_component.go
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- internal/exec/file_utils.go
- pkg/schema/schema_test.go
- pkg/schema/schema.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/yaml_utils.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(23-57)Settings(757-761)Terminal(200-207)pkg/utils/highlight_utils.go (1)
HighlightCodeWithConfig(75-147)pkg/utils/log_utils.go (1)
PrintMessage(26-28)
🪛 golangci-lint (1.64.8)
pkg/utils/yaml_utils.go
[error] 79-79: error is not nil (line 76) but it returns nil
(nilerr)
[error] 168-168: cognitive complexity 77 of func processCustomTags is high (> 20)
(gocognit)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (10)
pkg/utils/yaml_utils.go (10)
4-4: Good import standardization.Clean import changes - adding bytes for buffer support and standardizing to charmbracelet/log.
Also applies to: 11-11
20-29: Good constant organization.Added comment for clarity and defined a sensible default YAML indentation (2 spaces) as a constant.
45-45: Error definition looks good.Added properly defined error constant for nil configuration checks.
58-63: Nice helper function for config validation.This helper properly handles the edge cases where config might be nil or tab width might be invalid.
98-112: Good config validation and logging.Added proper nil checks and debug logging for tab width setting.
116-116: Good default indentation.Explicit use of DefaultYAMLIndent improves code clarity.
148-150: Good struct design for options.Simple and focused struct for YAML formatting options.
152-166: Improved YAML encoding with buffer.The new implementation using a buffer and encoder provides better control over indentation and formatting. The code maintains backward compatibility by handling default indentation properly.
173-173: Minor loop improvement.Small readability improvement in the loop structure.
298-300: Good error handling for nil config.Added proper validation of atmosConfig parameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/fixtures/scenarios/indentation/README.md (1)
1-5: Fix typos and improve grammar.The heading and a couple of sentences contain minor spelling and grammatical issues. Applying this diff will correct them and add missing punctuation:
-# Identation +# Indentation -We use this for the tests in `tests/test-cases/indentation.yaml` +We use this for the tests in `tests/test-cases/indentation.yaml`. -This is ran by `TestCLICommands` in `tests/cli_test.go` +This is run by `TestCLICommands` in `tests/cli_test.go`.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Consider using either the past participle “run” or the present participle “running” here.
Context: ...ts/test-cases/indentation.yamlThis is ran byTestCLICommandsintests/cli_test...(BEEN_PART_AGREEMENT)
[grammar] ~5-~5: Please add a punctuation mark at the end of paragraph.
Context: ....yamlThis is ran byTestCLICommandsintests/cli_test.goHere we runatmos...(PUNCTUATION_PARAGRAPH_END)
tests/fixtures/scenarios/indentation/atmos.yaml (1)
10-10: Remove trailing whitespace.Line 10 contains unnecessary spaces on an otherwise blank line, which triggers a YAML lint error. Please remove the trailing spaces:
@@ -8,4 +8,3 @@ terminal: tab_width: 4 # Number of spaces for YAML indentation (default: 2) - +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 10-10: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/fixtures/scenarios/indentation/README.md(1 hunks)tests/fixtures/scenarios/indentation/atmos.yaml(1 hunks)tests/snapshots/TestCLICommands_indentation.stderr.golden(1 hunks)tests/test-cases/indentation.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/test-cases/indentation.yaml
- tests/snapshots/TestCLICommands_indentation.stderr.golden
🧰 Additional context used
🧠 Learnings (1)
tests/fixtures/scenarios/indentation/atmos.yaml (1)
Learnt from: RoseSecurity
PR: cloudposse/atmos#797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
🪛 LanguageTool
tests/fixtures/scenarios/indentation/README.md
[grammar] ~5-~5: Consider using either the past participle “run” or the present participle “running” here.
Context: ...ts/test-cases/indentation.yamlThis is ran byTestCLICommandsintests/cli_test...
(BEEN_PART_AGREEMENT)
[grammar] ~5-~5: Please add a punctuation mark at the end of paragraph.
Context: ....yamlThis is ran byTestCLICommandsintests/cli_test.go Here we runatmos...
(PUNCTUATION_PARAGRAPH_END)
🪛 YAMLlint (1.37.1)
tests/fixtures/scenarios/indentation/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
what
This PR makes the indentation and empty components configurable trough atmos.yaml file.
why
Evidence:
custom tab width: 4
tab width: 2 (default config)
Include Empty False (default config)
references
Summary by CodeRabbit
New Features
tab_widthsetting.Improvements
Bug Fixes
Documentation
tab_widthconfiguration option.Tests