-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: status command #768
cli: status command #768
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick code review, will test out the command and give a more thorough review soon! Great work on this so far!!!
About to start looking at the code. I pulled this down and tried to run it but I got this response: Error returned from `consul-k8s status`
|
Note that I only saw this running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are doing great so far, Saad. I added some comments and will re-review later.
Outside of the scope of this PR, some of the content has me thinking that logic could be separated out so that a consul
and a helm
package exist. I think you'll have a lot of repeated logic across the different commands which might be consolidated using a cleaner architecture.
@ishustava and @NicoletaPopoviciu's bug fix while completing the following asana task fixed this issue! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty solid on the whole! Definitely fix the time formatting and make the linter happy, then it's good in my book.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming together great Saad! I think the date/time rendering is the main fix, everything else is small nits!
Edit: I see you got to the date/time rendering before I submitted to the review :)
cli/cmd/status/status.go
Outdated
|
||
func (c *Command) Help() string { | ||
c.once.Do(c.init) | ||
s := "Usage: consul-k8s status" + "\n\n" + "Get the status of the current Consul installation." + "\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s := "Usage: consul-k8s status" + "\n\n" + "Get the status of the current Consul installation." + "\n" + | |
s := "Usage: consul-k8s status" + "\n" + "Get the status of the current Consul installation." + "\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little lost here, why do we need two new-lines? Interestingly enough the help command outputs the same thing either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, nvm then. I noticed that the Global options displayed right underneath the help text so I thought adding an extra newline would make the output look more consistent with the install command's help message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work on this! Just needs a changelog entry before merging
…s into the common/utils.go file.
7769e9a
to
763006d
Compare
Hey Saad! I just rebased the branch and pushed up a changelog so we can merge! |
Based off of a demo branch worked on here: https://github.com/hashicorp/consul-k8s-cli/tree/status
Changes proposed in this PR:
consul-k8s status
checks the status of your Helm install and some Kubernetes components like the clients, servers, and webhook deployments.checkHelmInstallation()
function uses the Helm Go SDK to run ahelm status
, printing the status of the release, the version, and the config. Config is all of the values overridden fromvalues.yaml
. This function also iterates through each of the hooks and prints the phase of each 'pre-install' or 'pre-upgrade' hook.checkConsulServers()
andcheckConsulClients()
lists the Stateful Sets and Daemon Sets and compares the desired replicas to the ready ones. This determines if all the clients/servers are healthy or not.How I've tested this PR:
Manual testing, along with writing a few unit tests.
How I expect reviewers to test this PR:
cd cli
go build -o ./bin/consul-k8s
./bin/consul-k8s status
cli/cmd/status/status_test.go
Checklist: