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

Add logging #1212

Merged
merged 2 commits into from
Apr 18, 2018
Merged

Add logging #1212

merged 2 commits into from
Apr 18, 2018

Conversation

bamarni
Copy link
Contributor

@bamarni bamarni commented Apr 17, 2018

  • Add logrus as a dependency
  • Add flags for verbose outputs

@bamarni bamarni requested a review from armandgrillet April 17, 2018 19:17
pkg/cmd/dcos.go Outdated
}

cmd.PersistentFlags().CountVarP(&verbose, "", "v", "verbose (-v, or -vv)")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/verbose (-v, or -vv)/verbosity (-v or -vv). Also, the current output -v, -- count verbose (-v, or -vv) is not very meaningful (what does -- count means?), is there any way we can customize it?

revision = "7b2c5ac9fc04fc5efafb60700713d4fa609b777b"
version = "v0.0.1"
revision = "a1f051bc3eba734da4772d60e2d677f47cf93ef4"
version = "v0.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing the update in this commit? Is it necessary for logrus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this old cobra version the usage output is a bit worse : spf13/pflag#141

Copy link
Contributor

@armandgrillet armandgrillet left a comment

Choose a reason for hiding this comment

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

The usage of this library seems good but we're apparently gonna need our own type of flag to have a meaningful output.

@bamarni
Copy link
Contributor Author

bamarni commented Apr 18, 2018

Updated it, I agree the usage help output should be improved for this flag. I couldn't find a simple way to customize it yet. We can revisit this later as the flag still behaves as expected and it's only about the help output (which we are probably going to customize anyway).

armandgrillet
armandgrillet previously approved these changes Apr 18, 2018
bamarni added 2 commits April 18, 2018 11:30
This package will be used for CLI logging.

Also update existing dependencies.
A logger is introduced, the -v and -vv global flags can control its
level. The -v flag sets the level to info while the -vv flag sets the
level to debug.
@bamarni bamarni merged commit 64ca872 into dcos:master Apr 18, 2018
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.

2 participants