Skip to content

Add --verbose option to CLI#154

Merged
pcalcado merged 3 commits intomasterfrom
phil/cli-verbose
Jan 17, 2018
Merged

Add --verbose option to CLI#154
pcalcado merged 3 commits intomasterfrom
phil/cli-verbose

Conversation

@pcalcado
Copy link
Contributor

@pcalcado pcalcado commented Jan 16, 2018 •
edited
Loading

While debugging #136 I had to add a lot of printf because we didn't have a consolidated way to print verbose messages.

This PR introduces --verbose as a global flag that enables Debug log level. It also replaces some usage of other logging libraries in dashboard in favor of the one we use elsewhere.

Example output:

[email protected]:~/src/github.com/runconduit/conduit $ conduit version
Client version: v0.1.1
Server version: v0.1.1
[email protected]:~/src/github.com/runconduit/conduit $ conduit version --log-level debug
DEBU[0000] Making gRPC-over-HTTP call to [https://35.184.231.31/api/v1/namespaces/conduit/services/http:api:http/proxy/api/v1/Version]
DEBU[0000] gRPC-over-HTTP call returned status [200 OK] and content length [43]
Client version: v0.1.1
Server version: v0.1.1
[email protected]:~/src/github.com/runconduit/conduit $ conduit version help
Error: unknown command "help" for "conduit version"
Usage:
  conduit version [flags]

Flags:
      --api-addr string     Override kubeconfig and communicate directly with the control plane at host:port (mostly for testing)
  -h, --help                help for version
      --kubeconfig string   Path to the kubeconfig file to use for CLI requests

Global Flags:
  -n, --conduit-namespace string   namespace in which Conduit is installed (default "conduit")
      --log-level string           log level, must be one of: panic, fatal, error, warn, info, debug (default "fatal")

@pcalcado pcalcado self-assigned this Jan 16, 2018
@pcalcado pcalcado added the review/ready Issue has a reviewable PR label Jan 16, 2018
Use stdout as writer for tap command
ed15800 
fixes #136

Signed-off-by: Phil Calcado <[email protected]>
@siggy siggy added this to the 0.1.2 milestone Jan 16, 2018
siggy
siggy reviewed Jan 16, 2018
View reviewed changes
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

can we standardize on logLevel?
https://github.com/runconduit/conduit/blob/master/web/main.go#L37

also, i think a commit from another PR leaked into this one? i see tap fixes...

@pcalcado pcalcado changed the title Add --verbose option to CLI (blocked on #136) Add --verbose option to CLI Jan 16, 2018
@pcalcado pcalcado changed the title (blocked on #136) Add --verbose option to CLI Add --verbose option to CLI (blocked on #136) Jan 16, 2018
@pcalcado
Copy link
Contributor Author

pcalcado commented Jan 16, 2018

@siggy marked this as blocked on #136 to fix the other commit being carried over

but do you mean expose the log level instead of a generic --verbose option? I guess we could but I would prefer to keep the CLI UX decoupled from the way we happen to implement it. To the user it shouldn't matter what --verbose does, what log level it sets the underlying library to and/or if it does other things under the hood. I believe that leaking log levels would add options to the UX that users don't care about

@siggy
Copy link
Member

siggy commented Jan 16, 2018

@pcalcado i've found it useful to have more fine-grained logging control than binary verbose on/off. particularly during development, i want extremely verbose logging that would rarely be useful for an end-user. without this granularity, the --verbose flag will be too noisy for some, and not noisy enough for others.

coupling a --logLevel param to our logging library is not strictly required, but it does make the code quite easy to reason about, and the implementation we have in web would not preclude us from swapping out our logging library, since it's just strings passed at the command line.

dadjeibaah
dadjeibaah approved these changes Jan 16, 2018
View reviewed changes
Copy link
Contributor

@dadjeibaah dadjeibaah left a comment

Choose a reason for hiding this comment

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

@pcalcado PR looks good. Tried it locally and works fine. Are other verbose messages going to be added in to other commands like get.go, install.go etc? Also out of curiosity, what is the criteria for a message that is deemed verbose?

@pcalcado
Copy link
Contributor Author

pcalcado commented Jan 16, 2018 •
edited
Loading

@siggy made it log-level in a5f9df8 I am still not sure about the UX, but I guess we can discover more as we see people using it @

Add --log-level to command line
a5f9df8 
Signed-off-by: Phil Calcado <[email protected]>
siggy
siggy approved these changes Jan 16, 2018
View reviewed changes
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

looks good (and works!) 👍

@pcalcado pcalcado changed the title Add --verbose option to CLI (blocked on #136) Add --verbose option to CLI Jan 17, 2018
@pcalcado pcalcado merged commit 612bd0f into master Jan 17, 2018
@pcalcado pcalcado removed the review/ready Issue has a reviewable PR label Jan 17, 2018
@pcalcado pcalcado deleted the phil/cli-verbose branch January 23, 2018 16:27
@siggy siggy mentioned this pull request Jan 31, 2018
Merged
olix0r added a commit that referenced this pull request Dec 5, 2018
@olix0r
proxy: bump version for bug fixes
23b8dd7 
* 0065c13 profiles: Drive profile discovery on a daemon task (#156)
* b9ffbb7 Update h2 to v0.1.14
* 3ac6b72 Add basic tap integration tests (#154)
@olix0r olix0r mentioned this pull request Dec 5, 2018
Merged
olix0r added a commit that referenced this pull request Dec 5, 2018
@olix0r
proxy: bump version for bug fixes (#1935)
f80f389 
* 0065c13 profiles: Drive profile discovery on a daemon task (#156)
* b9ffbb7 Update h2 to v0.1.14
* 3ac6b72 Add basic tap integration tests (#154)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siggy siggy siggy approved these changes

@klingerf klingerf Awaiting requested review from klingerf

+1 more reviewer

@dadjeibaah dadjeibaah dadjeibaah approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pcalcado pcalcado

Labels

area/cli

Projects

None yet

Milestone

0.1.2

Development

Successfully merging this pull request may close these issues.

3 participants

@pcalcado @siggy @dadjeibaah