Skip to content

feat(RHOAIENG-54609): add --no-color flag and centralized color package#54

Merged
StevenTobin merged 1 commit intoopendatahub-io:mainfrom
ShettyGaurav:feat/add-no-color-flag
Mar 26, 2026
Merged

feat(RHOAIENG-54609): add --no-color flag and centralized color package#54
StevenTobin merged 1 commit intoopendatahub-io:mainfrom
ShettyGaurav:feat/add-no-color-flag

Conversation

@ShettyGaurav
Copy link
Copy Markdown

@ShettyGaurav ShettyGaurav commented Mar 23, 2026

Description

Add --no-color flag to the lint command with automatic color detection following the NO_COLOR standard.

Color is disabled automatically with the following priority chain:

  1. Structured output formats (JSON/YAML) always force NoColor=true to prevent ANSI codes from corrupting machine-readable output
  2. --no-color flag explicitly disables color when set
  3. NO_COLOR environment variable disables color when set to any non-empty value
  4. TTY detection disables color when stdout is not a terminal (e.g., piped or redirected)

Additionally, this PR refactors color formatting by extracting inline color.New(...) calls from output_table.go into a centralized pkg/util/color/symbols.go package. This ensures symbols and severity labels are generated at render time (respecting the color.NoColor global), rather than at init time when the color state is not yet determined.

Changes

  • pkg/lint/command.go: Add --no-color flag registration, color detection logic in Complete(), and IsTerminal() method using golang.org/x/term
  • pkg/lint/command_options.go: Add NoColor field to SharedOptions, add WithOutputFormat and WithIO functional options
  • pkg/lint/constants.go: Add flagDescNoColor constant
  • pkg/lint/output_table.go: Replace inline color symbols with centralized utilcolor package calls
  • pkg/util/color/symbols.go: New package providing status symbols, severity labels, and verdict strings as functions
  • go.mod: Promote golang.org/x/term from indirect to direct dependency

JIRA ISSUE

RHOAIENG-54609

How Has This Been Tested?

Unit tests:

  • Updated TestCommand_AddFlags to verify --no-color flag registration

Manual testing against a live OpenShift cluster with odh-operator installed

  • ./bin/kubectl-odh lint --target-version 3.0.0 -- colors rendered on TTY
  • ./bin/kubectl-odh lint --target-version 3.0.0 --no-color -- colors stripped
  • NO_COLOR=1 ./bin/kubectl-odh lint --target-version 3.0.0 -- colors stripped via env var
  • unset NO_COLOR && ./bin/kubectl-odh lint --target-version 3.0.0 -- colors restored
  • ./bin/kubectl-odh lint --target-version 3.0.0 -o json -- no ANSI codes in JSON output
  • make lint -- 0 issues
  • make build -- builds successfully

Without no color flag

Screenshot From 2026-03-23 16-05-38

With no color flag

Screenshot From 2026-03-23 15-55-30

with NO_COLOR env set

Screenshot From 2026-03-23 16-14-35

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

Release Notes

  • New Features

    • Added --no-color CLI flag to disable colored output in lint results. The flag respects the NO_COLOR environment variable and is automatically enabled for JSON and YAML structured output formats.
  • Refactor

    • Reorganized color output handling into centralized utility functions for improved code maintainability and consistency.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

This PR adds a --no-color CLI flag and refactors color symbol generation. The flag is wired to Command.NoColor and automatically forced to true when output format is JSON or YAML. A new pkg/util/color/symbols.go module consolidates previously hardcoded colored strings and status/severity/verdict helpers. The global fatih/color behavior is synchronized via color.NoColor = c.NoColor assignment. Test coverage validates flag registration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Issues

Potential data race in Command.Complete()

The assignment color.NoColor = c.NoColor mutates global package state. If the lint command runs concurrently or if the package is used as a library in multi-threaded contexts, this creates a data race on a shared global variable. The fatih/color package does not provide synchronization. Consider thread-local or context-based color handling instead of global mutation.

Undocumented behavior in symbols.go

The new helper functions (StatusPass(), StatusFail(), etc.) return formatted strings but their output format is not documented. Callers cannot determine whether they receive plain text, ANSI-escaped strings, or empty strings based on color.NoColor state. Document the contract clearly.

Missing validation in flag integration

No validation ensures Command.NoColor respects or reflects the underlying color.NoColor state after external mutations. If code directly modifies color.NoColor elsewhere, Command.NoColor becomes stale. Clarify ownership and synchronization guarantees in comments.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description is comprehensive, covering all template sections: clear explanation of the feature, testing performed (unit tests and manual testing with screenshots), and merge criteria checklist completed.
Title check ✅ Passed The title accurately summarizes the main changes: adding a --no-color flag and centralizing the color package into pkg/util/color/symbols.go.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ugiordan ugiordan requested a review from davidebianchi March 25, 2026 10:42
Comment thread pkg/lint/command.go Outdated
Comment thread pkg/lint/command_test.go Outdated
@ShettyGaurav ShettyGaurav force-pushed the feat/add-no-color-flag branch from 4c4f493 to 374d524 Compare March 25, 2026 12:45
@ShettyGaurav ShettyGaurav changed the title feat(RHOAIENG-54609): add --no-color flag with TTY detection, NO_COLOR support, and centralized color package feat(RHOAIENG-54609): add --no-color flag and centralized color package Mar 25, 2026
@StevenTobin StevenTobin merged commit d91f4f5 into opendatahub-io:main Mar 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants