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 32.89% 33.06% +0.16%
==========================================
Files 223 224 +1
Lines 23791 23890 +99
==========================================
+ Hits 7827 7899 +72
- Misses 14798 14816 +18
- Partials 1166 1175 +9
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. |
📝 WalkthroughWalkthroughThis pull request updates several command and utility functions throughout the codebase to pass a pointer to the configuration structure (typically Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmd as ExecuteDescribeStacksCmd
participant Config as atmosConfig
participant Printer as printOrWriteToFile
User->>Cmd: Execute describe stacks command
Cmd->>Config: Retrieve configuration settings
Note right of Cmd: Determine includeEmpty flag from config
Cmd-->>Cmd: Iterate over stack sections
alt Section not empty or includeEmpty enabled
Cmd->>Printer: Call printOrWriteToFile(&atmosConfig, ...)
else Section is empty and includeEmpty disabled
Note right of Cmd: Skip empty section
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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:✅ 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:✅ 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:❓ 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:❓ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/filter_utils_test.go (1)
11-130: Test structure for FilterEmptySections is comprehensive.The test cases cover a wide range of scenarios including nested maps, mixed value types, and edge cases like empty inputs. This thorough testing will ensure robust filtering behavior in the codebase.
Consider using the assert package consistently instead of reflect.DeepEqual for more readable error messages:
- if !reflect.DeepEqual(result, tt.expected) { - t.Errorf("FilterEmptySections() = %v, want %v", result, tt.expected) - } + assert.Equal(t, tt.expected, result)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/filter_utils_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/filter_utils_test.go (2)
1-10: LGTM - Import structure follows package conventions.The imports are properly organized and include all necessary dependencies for testing.
173-179: TestGetIncludeEmptySetting aligns with the PR objective for configurable empty components.This test case verifies that the
include_emptysetting can be explicitly set to false, which supports the PR goal of making empty component settings configurable.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces configurable YAML formatting for atmos describe commands by adding new configuration schema types for the describe command and a customizable terminal indentation (tab_width). It also updates multiple functions to pass the atmosConfig as a pointer and replaces legacy logging calls with structured logging.
- Added DescribeSettings and Describe types to support describe command output configuration.
- Introduced a Terminal tab_width field and updated PrintAsYAML/WriteToFile calls to use the new configuration.
- Refactored logging calls and configuration handling in various commands.
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/schema/schema.go | Added ne 10000 w configuration types for describe commands and terminal tab_width setting. |
| pkg/describe/describe_stacks_test.go | Added tests and helper functions for empty section filtering. |
| internal/exec/* | Updated PrintAsYAMLToFileDescriptor and logging calls to use pointers and structured logging. |
| examples/demo-stacks/component.yaml | Updated YAML example to include new configuration fields. |
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
| AtmosYamlFuncExec = "!exec" | ||
| AtmosYamlFuncStore = "!store" | ||
| AtmosYamlFuncTemplate = "!template" | ||
| AtmosYamlFuncTerraformOutput = "!terraform.output" | ||
| AtmosYamlFuncEnv = "!env" | ||
| AtmosYamlFuncInclude = "!include" | ||
| AtmosYamlFuncGitRoot = "!repo-root" | ||
|
|
||
| // Default YAML formatting settings |
Check failure
Code scanning / golangci-lint
Comment should end in a period Error
| return PrintAsYAMLWithConfig(&atmosConfig, data) | ||
| } | ||
|
|
||
| // getIndentFromConfig returns the configured tab width or the default value |
Check failure
Code scanning / golangci-lint
Comment should end in a period Error
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/utils/yaml_utils.go (1)
249-249: 🛠️ Refactor suggestionUpdate ConvertToYAML call to use DefaultYAMLIndent.
This call to
ConvertToYAMLdoesn't use the new options parameter. Consider updating it for consistency.- y, err := ConvertToYAML(res) + y, err := ConvertToYAML(res, YAMLOptions{Indent: DefaultYAMLIndent})
♻️ Duplicate comments (2)
pkg/utils/yaml_utils.go (2)
34-40:⚠️ Potential issueInclude AtmosYamlFuncInclude in AtmosYamlTags.
The
AtmosYamlFuncIncludeis defined as a constant but not included in theAtmosYamlTagsslice, which may cause inconsistent behavior when processing YAML tags.var ( AtmosYamlTags = []string{ AtmosYamlFuncExec, AtmosYamlFuncStore, AtmosYamlFuncTemplate, AtmosYamlFuncTerraformOutput, AtmosYamlFuncEnv, + AtmosYamlFuncInclude, }
122-122: 🛠️ Refactor suggestionSanitize file paths in debug logs.
Logging full file paths can expose directory structures or sensitive information. Consider sanitizing the path before logging.
- log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", filePath) + sanitizedPath := filepath.Base(filePath) + log.Debug("WriteToFileAsYAMLWithConfig", "tabWidth", indent, "filePath", sanitizedPath)
🧹 Nitpick comments (3)
pkg/utils/yaml_utils.go (3)
29-30: Add periods to comments for consistency.The comments for new constants should end with periods to maintain consistency with other comments in the codebase.
- // Default YAML formatting settings + // Default YAML formatting settings. DefaultYAMLIndent = 2🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 29-29:
Comment should end in a period
55-61: Add period to function comment.Comment should end with a period to maintain consistency across the codebase.
-// getIndentFromConfig returns the configured tab width or the default value +// getIndentFromConfig returns the configured tab width or the default value.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 55-55:
Comment should end in a period
158-273: Consider refactoring processCustomTags function in a future PR.The
processCustomTagsfunction has high cognitive complexity (77 > 20). Consider decomposing it into smaller functions in a future PR to improve readability and maintainability.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 158-158: cognitive complexity 77 of func
processCustomTagsis high (> 20)(gocognit)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/filter_utils.go(1 hunks)pkg/utils/yaml_utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/filter_utils.go
🧰 Additional context used
🪛 GitHub Check: golangci-lint
pkg/utils/yaml_utils.go
[failure] 29-29:
Comment should end in a period
[failure] 55-55:
Comment should end in a period
🪛 golangci-lint (1.64.8)
pkg/utils/yaml_utils.go
[error] 78-78: error is not nil (line 75) but it returns nil
(nilerr)
[error] 158-158: 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 (3)
pkg/utils/yaml_utils.go (3)
136-139: Use variadic parameters for YAMLOptions consistently.The
YAMLOptionsstruct is a good pattern for extensible configuration, but ensure it's used consistently throughout the codebase.
141-156: Clean implementation of YAML conversion with configurable indentation.The new implementation of
ConvertToYAMLwith options for indentation is well-structured and provides good flexibility.
288-290: Good defensive programming check.The nil check for atmosConfig is a good defensive programming practice to prevent runtime panics.
| highlighted, err := HighlightCodeWithConfig(y, *atmosConfig, "yaml") | ||
| if err != nil { | ||
| // Fallback to plain text if highlighting fails | ||
| PrintMessage(y) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix error handling behavior.
The function ignores the error from HighlightCodeWithConfig and returns nil instead of the error.
highlighted, err := HighlightCodeWithConfig(y, *atmosConfig, "yaml")
if err != nil {
PrintMessage(y)
- return nil
+ return err
}🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 78-78: error is not nil (line 75) but it returns nil
(nilerr)
🤖 Prompt for AI Agents (early access)
In pkg/utils/yaml_utils.go around lines 75 to 79, the error returned from
HighlightCodeWithConfig is ignored and the function returns nil instead of
propagating the error. Modify the error handling to return the error received
from HighlightCodeWithConfig instead of nil, ensuring proper error propagation.
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
Refactor
Style
Tests
Documentation