CSL-10969: Update the status command to output the consul client & deployments status as well along with existing ones. #4790

LordAbhishek · 2025-09-22T07:55:38Z

Changes proposed in this PR

Updated the status command to output the consul client & deployments status in tabular format, along with the existing status.
Updated the consul server status to output in tabular format in terminal, too.

hashicorp-cla-app · 2025-09-22T07:55:53Z

All committers have signed the CLA.

This reverts commit b391ef8.

sanikachavan5 · 2025-09-25T06:10:51Z

cli/cmd/status/status.go

-		return 1
+	err = c.checkConsulComponentsStatus(namespace)
+	if err != nil {
+		return 1 // Only for testing


unable to infer from the command, is this for local testing? If so, do we need to keep it?

checkConsulComponentsStatus is doing the main core work along with printing the errors.
The purpose of returning error is only in unit tests, to check if command run() is exiting with correct error codes.

If we dont output error code, the unit test at line 165 will will not be able to assert if the checkConsulComponentsStatus returned any error or it passed successfully.

We don't need the comment since the method expects an integer return value anyways :D

sriramr98 · 2025-09-26T05:42:56Z

cli/cmd/status/status.go

-		return 1
+	err = c.checkConsulComponentsStatus(namespace)
+	if err != nil {
+		return 1 // Only for testing


We don't need the comment since the method expects an integer return value anyways :D

sriramr98 · 2025-09-26T05:43:50Z

cli/cmd/status/status.go

-// checkConsulServers prints the status of Consul servers if they
-// are expected to be found in the Kubernetes cluster. It does not check for
-// server status if they are not running within the Kubernetes cluster.
+// checkConsulComponentsStatus lists status of different consul components


A more detailed comment will be great

sriramr98 · 2025-09-26T05:45:19Z

cli/cmd/status/status.go

+// checkConsulServers list the Consul Servers and their ready status (number of pods ready/desired),
 func (c *Command) checkConsulServers(namespace string) error {
-	servers, err := c.kubernetes.AppsV1().StatefulSets(namespace).List(c.Ctx, metav1.ListOptions{LabelSelector: "app=consul,chart=consul-helm,component=server"})
+	servers, err := c.kubernetes.AppsV1().StatefulSets(namespace).List(c.Ctx, metav1.ListOptions{})


Is this a breaking change? The behaviour of this flow will change as we're removing the label selector filters

Thanks for catching this, added label selector for both consul servers and clients.

sriramr98 · 2025-09-26T05:46:42Z

cli/cmd/status/status.go

+}
+
+// checkConsulClients list the Consul Clients and their ready status (number of pods ready/desired)
+func (c *Command) checkConsulClients(namespace string) error {


printConsulClients might be a more apt name

updated to getConsulClientsTable

sriramr98 · 2025-09-26T05:47:05Z

cli/cmd/status/status.go

+}
+
+// checkConsulServers list the Consul Servers and their ready status (number of pods ready/desired),
 func (c *Command) checkConsulServers(namespace string) error {


printConsulServers might be a more apt name

updated to getConsulServerTable

sriramr98 · 2025-09-26T05:47:18Z

cli/cmd/status/status.go

+}

+// checkConsulDeployments list the Consul Deployed Deployments and their ready status (number of pods ready/desired),
+func (c *Command) checkConsulDeployments(namespace string) error {


printConsulDeployments might be a more apt name

Updated checkConsulDeployments to getConsulDeoploymentTable which returns tbl to caller and then caller calls printComponentStatus for printing component status.
and in testing directly using getConsulDeoploymentTable to assert the table instance instead of buffer.

sriramr98 · 2025-09-26T05:48:39Z

cli/cmd/status/status.go

+}

+// checkConsulDeployments list the Consul Deployed Deployments and their ready status (number of pods ready/desired),
+func (c *Command) checkConsulDeployments(namespace string) error {


It would be better if this returns tbl so that unit tests can just use go native structs for assertions.
The line c.UI.Table(tbl) can be moved to the caller

sriramr98 · 2025-09-26T05:52:58Z

cli/cmd/status/status_test.go

 			require.NoError(t, err)

+			actual := buf.String()
+			if tc.desired > 0 {


For testing, it'll be better if checkConsulServers returns an instance of terminal.Table and it can be used for assertions directly. Asserting the stdout as string is not a good idea.

… testcases

Copilot

Pull Request Overview

This PR enhances the Consul CLI status command to display comprehensive status information for Consul components in tabular format. The change replaces simple text-based server status output with structured tables for servers, clients, and deployments.

Updated status command to show Consul clients, servers, and deployments status in tabular format
Refactored server status checking to use table-based output instead of plain text
Added comprehensive test coverage for the new client and deployment status functionality

Reviewed Changes

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

File	Description
cli/cmd/status/status.go	Refactored status checking logic to support tabular output for clients, servers, and deployments
cli/cmd/status/status_test.go	Added comprehensive tests for client and deployment status checking, updated existing server tests
.changelog/4790.txt	Added changelog entry documenting the new feature

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-29T11:43:26Z

cli/cmd/status/status.go

+// getConsulDeploymentsTable returns the table instance with the Consul Deployed Deployments
+// and their ready status (number of pods ready/desired),
+func (c *Command) getConsulDeploymentsTable(namespace string) (*terminal.Table, error) {
+	deployments, err := c.kubernetes.AppsV1().Deployments(namespace).List(c.Ctx, metav1.ListOptions{})


The deployments query lacks a label selector to filter for Consul-related deployments only. This will return all deployments in the namespace, not just Consul ones. Add a label selector like LabelSelector: \"app=consul,chart=consul-helm\" to match the pattern used for clients and servers.

Suggested change

deployments, err := c.kubernetes.AppsV1().Deployments(namespace).List(c.Ctx, metav1.ListOptions{})

deployments, err := c.kubernetes.AppsV1().Deployments(namespace).List(c.Ctx, metav1.ListOptions{LabelSelector: "app=consul,chart=consul-helm"})

cli/cmd/status/status_test.go

sriramr98 · 2025-09-29T11:47:15Z

cli/cmd/status/status.go

+	}
+	var tbl *terminal.Table
+	if len(clients.Items) != 0 {
+		tbl = terminal.NewTable("NAME", "READY", "AGE", "CONTAINERS", "IMAGES")


FOR FUTURE : This can be constants scoped to the package so that they can be reused in tests. No need to hold the PR for this now.

updates this one too

sriramr98 · 2025-09-29T11:55:49Z

cli/cmd/status/status_test.go

+				expectedHeaders := []string{"NAME", "READY", "AGE", "CONTAINERS", "IMAGES"}
+				assert.Equal(t, expectedHeaders, tbl.Headers)
+
+				require.Len(t, tbl.Rows, 1)


How come the test case 3 clients expected, 3 healthy is passing when clearly 3 is epxected, but we assert for 1?

i am creating only one daemonset at line 100 and for that 3 replica is expected and 3 healthy..
so in table, that will be in a single row... (asserting the number of rows{daemonsets})

for expected vs healthy, i am asserting at line 117.

sriramr98 · 2025-09-29T11:59:01Z

cli/cmd/status/status_test.go

+			c.kubernetes = fake.NewSimpleClientset()
+
+			// Deploy clients if needed.
+			if name != "No deployments" {


For Future: This is not extendable code. Ideally the testCase should have a boolean parameter which defines if a deployment should be created incase the scope changes in future.

this one is also updated...

sriramr98 · 2025-09-29T11:59:44Z

cli/cmd/status/status_test.go

+				expectedHeaders := []string{"NAME", "READY", "AGE", "CONTAINERS", "IMAGES"}
+				assert.Equal(t, expectedHeaders, tbl.Headers)
+
+				require.Len(t, tbl.Rows, 1)


Same issue as above. I assume that it's 1 deployment, 3 instances, 3 healthy.

Co-authored-by: Copilot <[email protected]>

LordAbhishek added 2 commits September 21, 2025 22:06

impr: update status cmd to include client and depl status in output

d55b17b

adding changelog

b391ef8

LordAbhishek requested a review from a team as a code owner September 22, 2025 07:55

LordAbhishek added area/cli CLI pr/no-backport signals that a PR will not contain a backport label labels Sep 22, 2025

LordAbhishek added 2 commits September 22, 2025 13:34

Revert "adding changelog"

8cfce36

This reverts commit b391ef8.

adding correct changelog

217c001

LordAbhishek changed the title ~~CSL-10969: Updated the status command to output the consul client & deployments status as well along with existing ones.~~ CSL-10969: Update the status command to output the consul client & deployments status as well along with existing ones. Sep 22, 2025

sanikachavan5 reviewed Sep 25, 2025

View reviewed changes

sriramr98 requested changes Sep 26, 2025

View reviewed changes

code refactored, updated labelSelector, fixed output colour & updated…

058f137

… testcases

sriramr98 previously approved these changes Sep 26, 2025

View reviewed changes

fixed unit test

4d1a86e

LordAbhishek dismissed sriramr98’s stale review via 4d1a86e September 29, 2025 01:07

sriramr98 requested a review from Copilot September 29, 2025 11:42

Copilot AI reviewed Sep 29, 2025

View reviewed changes

sriramr98 reviewed Sep 29, 2025

View reviewed changes

sriramr98 requested changes Sep 29, 2025

View reviewed changes

sriramr98 and others added 3 commits September 29, 2025 17:34

Update cli/cmd/status/status_test.go

c9b8f2c

Co-authored-by: Copilot <[email protected]>

minor changes and updated labels for fetching deployments

9a39412

updates test cases and use labels to find deployment pods

7a163e0

	deployments, err := c.kubernetes.AppsV1().Deployments(namespace).List(c.Ctx, metav1.ListOptions{})
	deployments, err := c.kubernetes.AppsV1().Deployments(namespace).List(c.Ctx, metav1.ListOptions{LabelSelector: "app=consul,chart=consul-helm"})

CSL-10969: Update the status command to output the consul client & deployments status as well along with existing ones. #4790

Are you sure you want to change the base?

CSL-10969: Update the status command to output the consul client & deployments status as well along with existing ones. #4790

Uh oh!

Conversation

LordAbhishek commented Sep 22, 2025

Changes proposed in this PR

Uh oh!

hashicorp-cla-app bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LordAbhishek Sep 25, 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!

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

hashicorp-cla-app bot commented Sep 22, 2025 •

edited

Loading

LordAbhishek Sep 25, 2025 •

edited

Loading