Improve addons list table design by pavansaikrishna78 · Pull Request #21288 · kubernetes/minikube

pavansaikrishna78 · 2025-08-11T13:45:38Z

If status is enabled then the whole row green

command: addons list

command: addons list -d

command: profile list

command: profile list -d

ADDON.LIST.MINIKUBE.docx

k8s-ci-robot · 2025-08-11T13:45:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pavansaikrishna78
Once this PR has been reviewed and has the lgtm label, please assign spowelljr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-08-11T13:45:48Z

Hi @pavansaikrishna78. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

minikube-bot · 2025-08-11T13:48:03Z

Can one of the admins verify this patch?

medyagh · 2025-08-11T19:24:57Z

 	// Mirror CN
 	AliyunMirror = "registry.cn-hangzhou.aliyuncs.com/google_containers"
+
+	//TABLE WRITER COLORS


is there a way to show how it looks like maybe a link or copy to the comment ?
is a library to use?

https://gist.github.com/JBlond/2fea43a3049b38287e5e9cefc87b2124

medyagh · 2025-08-11T19:26:52Z

 			enabled := addonBundle.IsEnabled(cc)
-			temp = []string{addonName, cc.Name, fmt.Sprintf("%s %s", stringFromStatus(enabled), iconFromStatus(enabled)), maintainer}
+			if enabled{
+				status := fmt.Sprintf("%s%s%s", constants.Enabled, iconFromStatus(enabled), constants.Default)


add a helper func to Format the text

we have a style package
style.Enable(.....)

medyagh · 2025-08-11T19:32:43Z

-				cpIP, strconv.Itoa(cpPort), k8sVersion, p.Status, strconv.Itoa(len(p.Config.Nodes)), c, k})
+			if p.Status == "OK" {
+				data = append(data, []string{
+					fmt.Sprintf("%s%s%s", constants.Enabled, p.Name, constants.Default),


we should have a style func that does
style.Normal( white color)
style.Enabled(
Style.Trouble(

medyagh · 2025-08-11T19:33:46Z

 		if isDetailed {
-			data = append(data, []string{p.Name, p.Config.Driver, p.Config.KubernetesConfig.ContainerRuntime,
-				cpIP, strconv.Itoa(cpPort), k8sVersion, p.Status, strconv.Itoa(len(p.Config.Nodes)), c, k})
+			if p.Status == "OK" {


this should be an iteration loop that calls the and based on the status it will call the Style Function in the package.

nirs

Please split the logical changes to separate commits instead of posing a new commit on every change.

If tests fail fix the failing commit. Don't post another commit fixing the first commit. This makes reviewing the changes much harder. Adding fix commit locally is fine, but before you post a new version you should squash the fix commit into the commit that introduced the problem.

Please remove all the unrelated and unnecessary changes. Focus on only on the design change to keep the change small, easy to review and be able to complete it quickly.

nirs · 2025-08-16T18:51:30Z

 	AliyunMirror = "registry.cn-hangzhou.aliyuncs.com/google_containers"
+
+	//TABLE WRITER COLORS
+	Enabled = "\033[32m"


This is green forground color. I will help to add a comment like:

Enabled = "\033[32m" // green forground

nirs · 2025-08-16T18:53:47Z

+
+	//TABLE WRITER COLORS
+	Enabled = "\033[32m"
+	Default = "\033[0m"


This is not a color but a reset command, removing all attributes (foreground color, background color, effects). We don't need a constant for default since it does not change the text.

nirs · 2025-08-16T18:54:05Z

+	//TABLE WRITER COLORS
+	Enabled = "\033[32m"
+	Default = "\033[0m"
+	Disabled = "\033[0m"


Same, since this is using the same value as default.

nirs · 2025-08-16T18:54:34Z

+	Enabled = "\033[32m"
+	Default = "\033[0m"
+	Disabled = "\033[0m"
+	Error = "\033[31m"  


This is red foreground, a comment will help.

But I don't think this is the way to go. What we need is a new package for color that can be reused by code writing output for tables or other output.

This package can use constants to the define the color of enabled, disabled, or errors, and provide functions using this signature:

Enabled(s string) string

Code rendering UI should use the functions to decorate fragments of text.

To implement this we can use a library like this:
https://github.com/fatih/color?tab=readme-ov-file#insert-into-noncolor-strings-sprintfunc

The advantage of using a library is using a well tested code that works on all platforms.

nirs · 2025-08-16T19:05:12Z

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors All rights reserved.
+Copyright 2025 The Kubernetes Authors All rights reserved.


We don't need to update the dates, that's pointless effort. The best way is to remove the date form the copyright headers, but this is not in the scope of this PR, so just remove this change.

nirs · 2025-08-16T19:21:14Z

+				applyColor = true
+			default:
+				applyColor = false
+			}


This is too complicated for no reason.

nirs · 2025-08-16T19:23:21Z

+					row[i] = fmt.Sprintf("%s%d%s", colorCode, v, constants.Default)
+				default:
+					row[i] = fmt.Sprintf("%s%v%s", colorCode, v, constants.Default)
+				}


Converting the values to string like we did before was better. If we colorize all value in a raw, we can keep the old code as is and apply the right color function to the entire slice.

nirs · 2025-08-16T19:23:57Z

 	updateProfilesStatus(validProfiles)
-
-	var body = map[string]interface{}{}
+	body := map[string]interface{}{}


Why did you change this?

nirs · 2025-08-16T19:29:20Z

 		tHeader = []string{"Addon Name", "Maintainer"}
 	} else {
-		tHeader = []string{"Addon Name", "Profile", "Status", "Maintainer"}
+		tHeader = []string{"Addon Name", "Enabled", "Maintainer"}


We have 3 logic changes:

Removing the profile column

Replacing Status with Enabled

Coloring the output

Each change should be done in a separate commit. This will make it easier to write the code without errors, test it, and review.

nirs · 2025-08-16T20:25:07Z

+				temp[i] = fmt.Sprintf("%s%s%s", valueColorCode, valueStr, constants.Default)
+			} else {
+				temp[i] = valueStr
+			}


Too complicated.

We need 2 steps:

Create list of strings for the table writer

Color the strings

For creating list of strings we can modify the previous code a little bit:

enabled := addonBundle.IsEnabled(cc) row = []string{addonName, cc.Name, iconFromStatus(enabled), maintainer}

iconForStatus() should return empty string if enabled is false. If it does not do this create a function that does this.

Since the list is so simple, we don't really need a loop to color it:

if enabled { row[0] = color.Enabled(row[0]) row[2] = color.Enabled(row[2]) }

But since wan to color longer lists (like profile list), we can add a function to color rows:

type colorFunc func(string) string func ColorRow(row []string, colored colorFunc) { for i := range row { text := row[i] if text == "" || isEmoji(text) { continue } row[i] = colored(text) } }

With this we can do:

switch status { case Broken: color.ColorRow(row, color.Broken) case Error: color.ColorRow(row, color.Error) case Enabled: color.ColorRow(row, color.Enabled) }

addons list and profile list and making pass the test

Wilson-Duncan

Reviewed and left a comment. In addition, for the run instructions it would be cleaner to update the screenshots to just be the table output. Or update listing the full command in plain text to make it easier to copy and paste.

i.e.

command: ./out/minikube addons list
...
command: ./out/minikube profile list -d

Wilson-Duncan · 2025-09-09T15:41:40Z

 }

-var printAddonsList = func(cc *config.ClusterConfig, printDocs bool) {
+func printAddonsList(cc *config.ClusterConfig, printDocs bool) {


I don't think this change is needed or relevant to the PR. Other files follow the same convention. So this should be taken care of in a different PR for refactoring once its determined why its being done this way. I think this is done to make the function easier to test but I'm not sure.

hi @Wilson-Duncan here is the output of the and correcting your above comment

I'm saying the commands should not be in the screenshots. ./out/minikube profile list, make, ./out/minikube addons list, and ./out/minikube profile list -d should be plain text and not a part of the screenshot.

addons list function correction

…to addon-list

nirs

Lets remove all unrelated changes and keep only the code needed to add colors.

We can add additional changes later, for now we need to focus on the color changes.

nirs · 2025-09-14T18:21:44Z


-	table.Options(
-		tablewriter.WithHeaderAutoFormat(tw.On),
-	)


table.Options(tablewriter.WithHeaderAutoFormat(tw.On)) replaces this code?

Is this related to the color change? If not please don't include this in this PR. I'm trying to review the color changes and I don't have time to review other changes now.

nirs · 2025-09-14T18:22:29Z

+		header = append(header, "Docs")
 	}
-	table.Header(tHeader)
+	table.Header(header)


Why did you change these names? Is this related to the color changes?

nirs · 2025-09-14T18:23:42Z

 		if maintainer == "" {
 			maintainer = "3rd party (unknown)"
 		}
+


Why to you keep adding unrelated whitespace changes? I already commented about it in previous reviews several times.

nirs · 2025-09-14T18:29:24Z

+		row[i] = colored("%s", row[i])
+	}
+}
+


Lets move the helpers to another file since they are used both in adding_list.go and profile_list.go. We can add a color.go file for these helpers. If we use colors only in the config package this is good enough for now, but I think the best place for it is a new color package in pkg/minikube/color/color.go.

nirs · 2025-09-14T18:32:10Z

+type colorFunc func(string, ...interface{}) string
+
+func isEmoji(s string) bool {
+	return strings.Contains(s, "✅")


This will not color test with emoji like "enabled ✅". Currently we don't have this but it will be more correct to use:

s == "✅"

Ideally we can use the unicode package to detect if the string contains only emoji characters.

k8s-triage-robot · 2025-12-21T19:47:06Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Copilot

Pull request overview

This pull request improves the visual design of the addons and profile list tables by adding color-coding based on status. Enabled addons and profiles with "OK" status are displayed in green, stopped/paused profiles are shown in yellow, and others in white. The PR also simplifies the addons list table structure.

Key changes:

Introduced a new pkg/minikube/color package with a ColorRow function for applying colors to table rows
Added color-coding to profile list output based on status (OK=green, Stopped/Paused=yellow, others=white)
Modified addons list table to use colored rows for enabled/disabled addons and simplified the table headers

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
pkg/minikube/color/color.go	New package providing color helper functions for table row coloring with emoji detection
cmd/minikube/cmd/config/profile_list.go	Added color-coding logic to profile list table rows based on profile status
cmd/minikube/cmd/config/addons_list.go	Refactored table building, added color-coding for enabled/disabled addons, and removed Profile column from output

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T22:15:00Z

+	if s == "✅" {
+		return true


The isEmoji function only checks for the checkmark emoji ("✅") but doesn't handle any other emojis. This is too specific and makes the function name misleading. Consider either renaming the function to isCheckmarkEmoji or extending it to handle all emojis that should be skipped during coloring.

Suggested change

if s == "✅" {

return true

for _, r := range s {

// Basic coverage for common emoji ranges:

// - Miscellaneous Symbols & Dingbats, etc. (includes ✅ U+2705)

// - Miscellaneous Symbols and Pictographs, Supplemental Symbols and Pictographs, etc.

if (r >= 0x2600 && r <= 0x27BF) || (r >= 0x1F300 && r <= 0x1FAFF) {

return true

}

Copilot · 2026-01-08T22:15:01Z

+package color
+
+// colorFunc allows generic coloring
+type colorFunc func(string, ...interface{}) string
+
+func isEmoji(s string) bool {
+	if s == "✅" {
+		return true
+	}
+	return false
+}
+
+func ColorRow(row []string, colored colorFunc) {
+	for i := range row {
+		if row[i] == "" || isEmoji(row[i]) {
+			continue
+		}
+		row[i] = colored("%s", row[i])
+	}
+}


The new ColorRow function in pkg/minikube/color lacks test coverage. Given that other packages in pkg/minikube/ have test files (e.g., out_test.go, style_test.go), this new color package should also include tests to verify the coloring behavior, especially for edge cases like empty strings and emoji handling.

Copilot · 2026-01-08T22:15:01Z

+			row = []string{p.Name, p.Config.Driver, p.Config.KubernetesConfig.ContainerRuntime,
+				cpIP, k8sVersion, p.Status, strconv.Itoa(len(p.Config.Nodes)), c, k}
 		}
+


Trailing whitespace on this empty line should be removed to maintain code cleanliness and consistency with coding standards.

Suggested change

Copilot · 2026-01-08T22:15:01Z

+		default:
+			pkgcolor.ColorRow(row, color.WhiteString)
+		}
+


Trailing whitespace on this empty line should be removed to maintain code cleanliness and consistency with coding standards.

Copilot · 2026-01-08T22:15:01Z

+func ColorRow(row []string, colored colorFunc) {
+	for i := range row {
+		if row[i] == "" || isEmoji(row[i]) {
+			continue
+		}
+		row[i] = colored("%s", row[i])
+	}
+}


The ColorRow function modifies the slice in place, which can be confusing as the function name doesn't indicate mutation. Consider either: 1) returning a new colored slice instead of modifying in place, or 2) renaming the function to something like ColorRowInPlace to make the mutation behavior explicit, or 3) adding documentation explaining the in-place modification.

Copilot · 2026-01-08T22:15:02Z

 		if cc == nil {
-			temp = []string{addonName, maintainer}
+			row = []string{addonName, maintainer}
 		} else {
 			enabled := addonBundle.IsEnabled(cc)
-			temp = []string{addonName, cc.Name, fmt.Sprintf("%s %s", stringFromStatus(enabled), iconFromStatus(enabled)), maintainer}
+			status := iconFromStatus(enabled)
+			row = []string{addonName, status, maintainer}
+			if printDocs {
+				row = append(row, docs)
+			}
+
+			// Step 2: apply coloring
+			switch {
+			case enabled:
+				pkgcolor.ColorRow(row, color.GreenString)
+			default:
+				pkgcolor.ColorRow(row, color.WhiteString)
+			}
 		}
-		if printDocs {
-			temp = append(temp, docs)
+
+		if cc == nil && printDocs {
+			row = append(row, docs)
 		}


The logic for building rows when cc == nil doesn't apply coloring but still appends the docs when printDocs is true. This creates an inconsistency where colored rows are built inside the "cc != nil" block at lines 125-144, but non-colored rows have their docs appended separately at lines 146-148. This duplicate logic for handling printDocs is harder to maintain and could lead to bugs.

Copilot · 2026-01-08T22:15:02Z

 			enabled := addonBundle.IsEnabled(cc)
-			temp = []string{addonName, cc.Name, fmt.Sprintf("%s %s", stringFromStatus(enabled), iconFromStatus(enabled)), maintainer}
+			status := iconFromStatus(enabled)
+			row = []string{addonName, status, maintainer}


The profile name (cc.Name) was removed from the table output entirely. This is a breaking change for users who may be parsing or relying on the table output to know which profile the addons belong to. Consider keeping the profile information in the table, especially when there are multiple profiles.

Suggested change

row = []string{addonName, status, maintainer}

// Include profile name in the table output to indicate which profile the addon belongs to.

row = []string{cc.Name, addonName, status, maintainer}

k8s-ci-robot · 2026-01-09T02:33:59Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

medyagh · 2026-01-09T07:21:13Z

@pavansaikrishna78 this PR replaces this #22418

but you can still make the PR to change the addons list table to be worded differently and have less columns

let me know if you intereted to do it

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2025

k8s-ci-robot requested review from nirs and prezha August 11, 2025 13:45

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2025

medyagh reviewed Aug 11, 2025

View reviewed changes

nirs reviewed Aug 16, 2025

View reviewed changes

pavansaikrishna78 force-pushed the addon-list branch 2 times, most recently from 3f61c2c to 6bc5fee Compare August 18, 2025 15:11

pavansaikrishna78 force-pushed the addon-list branch from 565498b to cd36cf8 Compare August 18, 2025 15:57

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2025

addon list for table write package and profile list

789116d

addons list and profile list and making pass the test

pavansaikrishna78 force-pushed the addon-list branch from cd36cf8 to 789116d Compare August 18, 2025 16:23

Wilson-Duncan reviewed Sep 9, 2025

View reviewed changes

pavansaikrishna78 added 3 commits September 9, 2025 22:17

addons list function correction

36a8b18

adding color to addons list and profile list

7dabf8e

addons list function correction

Merge branch 'addon-list' of github.com:pavansaikrishna78/minikube in…

b3a9d84

…to addon-list

pavansaikrishna78 force-pushed the addon-list branch from b3a9d84 to 64cb1a4 Compare September 9, 2025 16:58

pavansaikrishna78 force-pushed the addon-list branch from 64cb1a4 to b3a9d84 Compare September 9, 2025 17:01

pavansaikrishna78 requested review from Wilson-Duncan and medyagh September 9, 2025 17:03

nirs suggested changes Sep 14, 2025

View reviewed changes

addons list corrections

81cf409

pavansaikrishna78 requested review from nirs September 16, 2025 09:48

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 21, 2025

medyagh requested review from Copilot and removed request for Wilson-Duncan January 8, 2026 22:09

Copilot started reviewing on behalf of medyagh January 8, 2026 22:10 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2026

medyagh closed this Jan 9, 2026

-	if s == "✅" {
-		return true
+	for _, r := range s {
+		// Basic coverage for common emoji ranges:
+		// - Miscellaneous Symbols & Dingbats, etc. (includes ✅ U+2705)
+		// - Miscellaneous Symbols and Pictographs, Supplemental Symbols and Pictographs, etc.
+		if (r >= 0x2600 && r <= 0x27BF) || (r >= 0x1F300 && r <= 0x1FAFF) {
+			return true
+		}

	row = []string{addonName, status, maintainer}
	// Include profile name in the table output to indicate which profile the addon belongs to.
	row = []string{cc.Name, addonName, status, maintainer}

Conversation

pavansaikrishna78 commented Aug 11, 2025

Uh oh!

k8s-ci-robot commented Aug 11, 2025

Uh oh!

k8s-ci-robot commented Aug 11, 2025

Uh oh!

minikube-bot commented Aug 11, 2025

Uh oh!

medyagh Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medyagh Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wilson-Duncan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Dec 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

medyagh Aug 11, 2025 •

edited

Loading

medyagh Aug 11, 2025 •

edited

Loading