Add no-color support for atmos #1227
Conversation
…ttps://github.com/cloudposse/atmos into feature/dev-3177--no-color-should-be-a-global-flag
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
+ Coverage 33.56% 34.68% +1.11%
==========================================
Files 226 226
Lines 24219 24258 +39
==========================================
+ Hits 8130 8413 +283
+ Misses 14875 14606 -269
- Partials 1214 1239 +25
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:
|
…ttps://github.com/cloudposse/atmos into feature/dev-3177--no-color-should-be-a-global-flag
d1297fb to
14534fb
Compare
00c0e40 to
131249b
Compare
| @@ -1,35 +0,0 @@ | |||
| package cmd | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
For some reason this test does not work in our windows workflow after our update.
I tried to check in my windows machine and this test works. Suggesting some strange issue with the worker windows version.
There was a problem hiding this comment.
I added the test to the yaml.
There was a problem hiding this comment.
Why add to yaml? That doesn’t increase or count towards test coverage and deleting it reduces test coverage.
There was a problem hiding this comment.
This is the most basic of tests, so we can just reimplement if it is wrong.
There was a problem hiding this comment.
For example, look at how you are implementing the tests for color. You are taking a different approach than what we see in this test. Maybe use the same approach.
There was a problem hiding this comment.
I already tried reimplementing it in different ways.
Just not sure why this is happening here.
The issue is outputting to the pipe buffer that we create.
But i am not sure why only this test case out of the many we have.
While i understand your perspective that this does not add coverage in yaml mode.
It covers only one line. So not sure worth the effort in debugging it for long.
📝 WalkthroughWalkthroughThis update introduces a new global Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant Logger
participant Renderer
User->>CLI: Run command with/without --no-color
CLI->>Config: Load config, parse env/flags
Config->>CLI: Provide no_color setting
CLI->>Logger: Setup logger (disable color if no_color)
CLI->>Renderer: Initialize renderer (disable color if no_color)
CLI->>User: Output (colored or plain, per config)
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🪛 GitHub Check: codecov/patchcmd/validate_editorconfig.go[warning] 120-120: cmd/validate_editorconfig.go#L120 [warning] 122-122: cmd/validate_editorconfig.go#L122 ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ 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: 4
🔭 Outside diff range comments (1)
pkg/ui/markdown/renderer.go (1)
140-147:⚠️ Potential issueUse the injected TTY helpers everywhere, not the global ones
Inside the rendering loop you fall back to
term.IsTTYSupportForStdout(), bypassing the injectedr.isTTYSupportForStdoutthat was introduced precisely to make TTY detection replaceable (e.g. for tests).
This breaks the abstraction and makes the behaviour harder to stub in unit tests.- if strings.HasPrefix(trimmed, "$") && term.IsTTYSupportForStdout() { + if strings.HasPrefix(trimmed, "$") && r.isTTYSupportForStdout() {A quick replace keeps the logic intact while preserving testability.
🧹 Nitpick comments (4)
tests/test-cases/empty-dir.yaml (1)
17-27: Add basic integration test for thesupportcommand
This new test ensures theatmos supportcommand executes successfully. Consider adding expected stdout patterns or snapshots to validate the output content in future iterations.cmd/root_test.go (1)
12-37: Good test coverage for no-color functionality.The test correctly verifies that setting the
NO_COLORenvironment variable disables color output in logs. This ensures the feature works as expected.There's a commented-out line at line 14 that appears to be duplicated by line 16 and could be removed:
- // t.Setenv("NO_COLOR", "1")pkg/config/config_test.go (1)
441-488: Comprehensive testing of no-color flag combinations.The test cases effectively verify that the
--no-colorflag is properly propagated to the configuration, including testing various combinations with log level settings. I particularly like the inclusion of the explicit false case (--no-color=false).Note that the variable name
expectetdNoColorhas a typo and should beexpectedNoColor, but it doesn't affect functionality.- expectetdNoColor bool + expectedNoColor boolpkg/ui/markdown/renderer.go (1)
80-109: Renderer re-creation on every call hurts performance
RenderWithoutWordWrapspins up a brand-new Glamour renderer every single invocation.
For large documentation generation loops this is needlessly expensive; the renderer can be prepared once (at construction time) with word-wrap0and reused, or at minimum cached behind async.Once.If immutability of
r.rendereris desired, consider adding a small cache:// inside Renderer asciiNoWrap *glamour.TermRenderer // lazily initialisedand populate it only the first time the path is taken.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-92: pkg/ui/markdown/renderer.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 97-98: pkg/ui/markdown/renderer.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 107-108: pkg/ui/markdown/renderer.go#L107-L108
Added lines #L107 - L108 were not covered by tests🪛 GitHub Check: golangci-lint
[warning] 83-83:
if r.atmosConfig.Settings.Terminal.NoColorhas complex nested blocks (complexity: 4)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
cmd/root.go(3 hunks)cmd/root_test.go(1 hunks)cmd/support_test.go(0 hunks)cmd/validate_editorconfig.go(1 hunks)pkg/config/config.go(1 hunks)pkg/config/config_test.go(1 hunks)pkg/config/default.go(0 hunks)pkg/config/load.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/ui/markdown/renderer.go(4 hunks)pkg/ui/markdown/renderer_test.go(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Command_Line_Flag.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(2 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(0 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(0 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_support.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden(1 hunks)tests/test-cases/empty-dir.yaml(1 hunks)website/docs/cli/configuration/configuration.mdx(1 hunks)website/docs/cli/configuration/terminal.mdx(2 hunks)
💤 Files with no reviewable changes (4)
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- pkg/config/default.go
- cmd/support_test.go
🧰 Additional context used
🧠 Learnings (2)
pkg/config/load.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
cmd/validate_editorconfig.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
🧬 Code Graph Analysis (4)
pkg/config/config.go (1)
pkg/schema/schema.go (2)
Settings(716-720)Terminal(188-194)
cmd/validate_editorconfig.go (1)
pkg/schema/schema.go (1)
Terminal(188-194)
pkg/config/config_test.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(13-45)Logs(304-307)
pkg/ui/markdown/renderer.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(13-45)Settings(716-720)Terminal(188-194)internal/tui/templates/term/term_writer.go (2)
IsTTYSupportForStdout(83-87)IsTTYSupportForStderr(90-94)pkg/ui/markdown/styles.go (1)
GetDefaultStyle(24-90)
🪛 GitHub Check: codecov/patch
pkg/config/config.go
[warning] 83-84: pkg/config/config.go#L83-L84
Added lines #L83 - L84 were not covered by tests
cmd/validate_editorconfig.go
[warning] 121-121: cmd/validate_editorconfig.go#L121
Added line #L121 was not covered by tests
pkg/ui/markdown/renderer.go
[warning] 50-51: pkg/ui/markdown/renderer.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 91-92: pkg/ui/markdown/renderer.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 97-98: pkg/ui/markdown/renderer.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 107-108: pkg/ui/markdown/renderer.go#L107-L108
Added lines #L107 - L108 were not covered by tests
🪛 GitHub Check: golangci-lint
pkg/ui/markdown/renderer.go
[warning] 83-83:
if r.atmosConfig.Settings.Terminal.NoColor has complex nested blocks (complexity: 4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (39)
tests/snapshots/TestCLICommands_Valid_Log_Level_in_Command_Line_Flag.stdout.golden (1)
2-2: Snapshot environment label updated
The platform identifier was changed fromdarwin/arm64tolinux/amd64to match the test environment. No functional changes here.tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden (1)
2-2: Snapshot environment label updated
This updates the platform label tolinux/amd64, aligning with the environment where tests ran. No logic affected.tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stdout.golden (1)
2-2: Snapshot environment label updated
Platform identifier updated tolinux/amd64for consistency with the CI environment. No functional change.tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden (1)
2-2: Snapshot environment label updated
Changed tolinux/amd64to reflect the testing environment. Functional behavior remains unchanged.tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden (1)
2-2: Snapshot environment label updated
Updated the platform tag tolinux/amd64, matching the environment. No logic modification.tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden (1)
7-7: Update snapshot platform identifier
The change from “darwin/arm64” to “linux/amd64” aligns with the current CI environment and has no impact on functionality.tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden (1)
111-113: Add global--no-colorflag to help output
The new flag is correctly placed under Global Flags with consistent indentation and description, matching the PR’s goal of disabling colored output.tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden (1)
49-49: Add global--no-colorflag to apply help
The flag is properly integrated into the Global Flags section and mirrors the formatting in other commands.tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden (1)
7-7: Update snapshot platform identifier
Switching to “linux/amd64” reflects the testing environment update and does not affect functionality.tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden (1)
37-37: Add global--no-colorflag to about help
Placement under Global Flags is consistent with other help outputs, ensuring uniform documentation of the new flag.tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden (1)
56-56: Approved: Snapshot updated foratmos helmfile --help
The new--no-colorglobal flag is present with(default false)as expected.tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden (1)
45-45: Approved: Snapshot updated foratmos atlantis --help
The--no-colorglobal flag appears correctly with default false.tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden (1)
45-45: Approved: Snapshot updated foratmos atlantis help
The global--no-coloroption is correctly shown with(default false).tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden (1)
38-38: Approved: Snapshot updated foratmos helmfile apply --help
The--no-colorflag is now listed under Global Flags with default false.tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden (1)
56-56: Approved: Snapshot updated foratmos helmfile help
The global--no-colorflag appears as intended with(default false).tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden (1)
38-39: LGTM! No-color global flag is properly implemented.The
--no-colorflag has been added as a global flag with clear documentation and default value (false), addressing the accessibility needs mentioned in the PR objectives.tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden (1)
111-112: Global flag consistently applied to terraform command.The
--no-colorflag is consistently documented with the same description and default value as in other commands, maintaining a unified CLI interface.tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden (1)
49-50: No-color flag correctly added to terraform apply subcommand.The global
--no-colorflag is properly documented in the terraform apply help output, ensuring users know about this accessibility option when viewing subcommand help.tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden (1)
48-49: No-color flag correctly implemented in atlantis command.The global
--no-colorflag has been consistently applied to the atlantis generate command help output. This completes the implementation across all reviewed CLI commands.tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden (1)
48-49: New global flag looks good.The
--no-colorflag addition is consistent with the PR objective to provide a way to disable colored output. This feature will help users with custom terminals who find colored output difficult to read.tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden (1)
85-86: Global flag placement is consistent.The
--no-colorflag appears in the correct section under Global Flags with appropriate formatting and consistent description.tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden (1)
85-86: Flag description is clear and properly positioned.The
--no-colorflag has been properly added to the Global Flags section with a clear description that aligns with its purpose.tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden (1)
116-117: Global flag consistently implemented.The
--no-colorflag has been properly added to the Terraform command help output, maintaining consistency across all command help displays.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden (1)
73-73: Include global--no-colorflag in help snapshot
The updated snapshot correctly reflects the addition of the global--no-coloroption with its description and default.tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (1)
60-60: Update global help snapshot to include--no-color
The addition of the--no-colorflag to the global help output is correct and matches the new CLI behavior.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (2)
80-81: Reflect unifiedno_colorsetting in configuration snapshot
The snapshot now includes"no_color": falseundersettings.terminalas expected.
150-150: Remove expliciteditorconfig.colorproperty
Theeditorconfigsection is correctly simplified to{}, relying on the globalno_colorsetting instead of a per-command color flag.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden (1)
73-73: Looks good - supports the new no-color global flagThe addition of
--no-coloras a global flag enhances accessibility for users with custom terminals where colored output might be hard to read.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
123-123: Configuration schema correctly updated for no-colorThe addition of
no_color: falsesetting in the terminal configuration section ensures that the no-color option is properly represented in the config schema, while maintaining colored output as the default behavior.pkg/schema/schema.go (1)
193-193: Schema update looks goodThe
NoColorfield added to theTerminalstruct properly supports the new feature with appropriate struct tags for YAML, JSON, and mapstructure.cmd/root.go (2)
99-108: Well implemented color disabling for logs.The implementation properly disables colors in logs when
NoColoris set, while preserving other style attributes. This provides good accessibility for users with custom terminals who have difficulty reading colored output.
208-208: Good addition of global --no-color flag.Adding this as a persistent flag at the root command level is the right approach, as it applies globally across all commands rather than requiring command-specific implementations.
website/docs/cli/configuration/terminal.mdx (2)
30-32: Properly updated terminal settings documentation.The configuration example has been correctly updated to use
no_color: falseinstead of the previouscolorssetting, maintaining consistency with the implementation.
81-83: Clear documentation for the new no_color option.The added documentation for the
no_colorsetting is clear and concise, making it easy for users to understand how to disable colored output.pkg/config/load.go (2)
101-101: Good environment variable binding for no_color setting.Binding both
ATMOS_NO_COLORand standardNO_COLORenvironment variables follows best practices and provides users with flexibility. This correctly usesviper.BindEnvrather than directos.Getenvcalls as recommended.
118-118: Appropriate default for no_color setting.Setting the default to
falseensures colors remain enabled by default, which is the expected behavior.tests/snapshots/TestCLICommands_atmos_support.stdout.golden (1)
1-19: New snapshot file looks good.This new snapshot captures the expected plain-text output for the
atmos supportcommand, which aligns with the PR objective of adding no-color support for terminals that have difficulty with colored output.pkg/config/config_test.go (1)
407-439: Good test coverage for flag parsing.The test cases comprehensively cover parsing different flag configurations, which is important for validating the new
--no-colorflag functionality.pkg/ui/markdown/renderer_test.go (1)
1-101: Well-structured tests for color rendering.These tests comprehensively verify that the markdown renderer properly handles the no-color setting. Good practices observed:
- Testing both color and no-color scenarios
- Mocking TTY support functions for consistent testing
- Properly restoring original functions after tests
- Testing multiple rendering methods including error rendering
This ensures the no-color support works correctly throughout the rendering system.
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
what
why
references
--no-colorshould be a global flag #1224Summary by CodeRabbit
New Features
--no-colorflag to disable colored output in logs and terminal display.no_colorconfiguration option with environment variable support (ATMOS_NO_COLOR,NO_COLOR).Bug Fixes
Documentation
no_coloroption and flag usage.Tests
--no-colorflag, configuration parsing, and markdown rendering without color.Refactor
no_colorsetting for clarity and consistency.