Skip to content
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

Better logging for init #934

Merged
merged 9 commits into from
Oct 18, 2019
Merged

Better logging for init #934

merged 9 commits into from
Oct 18, 2019

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Oct 11, 2019

What this PR does / why we need it:
Right now when you run init and you're initializing the server side as well as local environment, the only thing the command prints out is that local was initialized. This PR adds more logging to make it obvious that server side is being initialized as well.

This is output of init before this change

➜  kudo git:(av/maintainers-array) ✗ ./bin/kubectl-kudo init
$KUDO_HOME has been configured at /Users/alenavarkockova/.kudo

After

➜  kudo git:(av/init-logging) ✗ ./bin/kubectl-kudo init
$KUDO_HOME has been configured at /Users/alenavarkockova/.kudo
✅ installing crds
✅ preparing service accounts and other requirements for controller to run
✅ installing kudo controller

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We desperately need fancy Unicode characters in the output! And not only here, but when installing instances too! And in the plan status output! Let the be an inspiration to you ;)

pkg/kudoctl/cmd/init/manager.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/init/manager.go Outdated Show resolved Hide resolved
pkg/kudoctl/cmd/init/manager.go Outdated Show resolved Hide resolved
alenkacz and others added 3 commits October 11, 2019 12:44
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
pkg/kudoctl/cmd/init.go Outdated Show resolved Hide resolved
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Comment on lines 62 to 65
clog.Printf("installing crds")
if err := installCrds(client.ExtClient); err != nil {
return err
}
Copy link
Contributor

@zen-dog zen-dog Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, that I think about it, it should be the other way around ;)

Suggested change
clog.Printf("✓ installing crds")
if err := installCrds(client.ExtClient); err != nil {
return err
}
if err := installCrds(client.ExtClient); err != nil {
return fmt.Errorf("❌failed to install CRDs: %w", err)
}
clog.Printf("✅ installed crds")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the others too ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer @zen-dog graphics... and I do not like graphics in the error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 so do we want to add the cross there or not? I am not sure I understand the conclusion of that sentence

@@ -59,19 +59,19 @@ func NewOptions(v string) Options {
// Install uses Kubernetes client to install KUDO.
func Install(client *kube.Client, opts Options, crdOnly bool) error {

clog.V(4).Printf("installing crds")
clog.Printf("installing crds")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I prefer a more quiet cli... I really preferred the verbosity at 4 rather than at 0. I do like the unicode chars!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unicode ftw!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually find it weird to run a command and have no feedback on what actually happened and if something happened. I think Zain or some of the SEs complained about it as well (that they don't have any feedback after the command is run). So I think we need to have at least a very short output for every command

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some additional thoughts...
the example output is now:

$KUDO_HOME has been configured at /Users/alenavarkockova/.kudo
installing crds
installing prereqs
installing kudo controller

when verbosity was 4... these were debugging messages to help potentially diagnose an issue. now at verbosity of 0 they are standard cli output... because of that... if we keep verbosity at 0 I think:

  1. we need to case correctly... like CRDs and KUDO
  2. I'm questioning abbrev... "prereq" has multiple issues... for one we should spell it out.. but it also begs the question what is that.

@gerred
Copy link
Member

gerred commented Oct 15, 2019

I don't have a strong opinion here. I do agree with @kensipe that we should expand the pronunciation and make those for humans.

I do like longer error messages that are more descriptive and try to help people in general. See Elm and Rust's errors for where I'm thinking there - obviously this is not that PR. IMO, Everything at log level 0 / printed by default should be non-expert human oriented and verbosity levels of that should be written for developers and experts.

alenkacz and others added 3 commits October 17, 2019 10:50
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@alenkacz
Copy link
Contributor Author

@gerred @kensipe I updated the description with the current output, are we fine with it? I like it 😊

@alenkacz
Copy link
Contributor Author

I am going to merge this. We can always iterate on master

@alenkacz alenkacz merged commit dec64d4 into master Oct 18, 2019
@alenkacz alenkacz deleted the av/init-logging branch October 18, 2019 07:19
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