-
Notifications
You must be signed in to change notification settings - Fork 932
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
shared/cmd: Allow a cmd asker to be created with a logger instance #13859
shared/cmd: Allow a cmd asker to be created with a logger instance #13859
Conversation
5a463f2
to
e588b88
Compare
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.
Sounds reasonable to me, though one nit I have is that verbose
is kind of ambiguous for a feature like this which is intended for debugging. Not necessarily a blocker.
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.
As discussed the other day, lets change this to allow passing in a logger for verbose logging rather than outputting to stdout.
If a command asker is set with a logger instance, the question and answer (even if it is invalid) are systematically logged. This will greatly help debugability in integration test suites like in MicroCloud which have a lot of interactions in the command line. Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
e588b88
to
5c1fcef
Compare
@tomponline updated |
@gabrielmougard linting issues need fixing |
40d7144
to
80e2400
Compare
…govet)` lint error Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
80e2400
to
128f9de
Compare
If a command asker is with a logger, the question and answer (even if it is invalid) are systematically logged to the logger destination. This will greatly help debugability in integration test suites like in MicroCloud which have a lot of interactions in the command line.