-
Notifications
You must be signed in to change notification settings - Fork 202
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
feat: Verbose logging with flag #350
Conversation
Sample output from stderr after running AVP on a template pulling secrets from IBM Cloud SM:
Some of the output is out of order, probably b/c of the goroutines being used for this particular backend. It's still mostly traceable though especially with the original template. |
@@ -59,6 +61,7 @@ func NewGenerateCommand() *cobra.Command { | |||
} | |||
|
|||
v := viper.New() | |||
viper.Set("verboseOutput", verboseOutput) |
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.
Using the global singleton Viper here since the verbose output setting isn't specific to any backend or even any part of the code (it's used when doing auth, replacements in the template, and when retrieving from backends)
Codecov Report
@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 76.68% 77.53% +0.84%
==========================================
Files 22 22
Lines 948 1006 +58
==========================================
+ Hits 727 780 +53
- Misses 137 142 +5
Partials 84 84
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@jkayani does it make sense to add tests for the verbose logging? |
@werne2j I don't know, what do you think? On one hand I don't like taking the hit on code coverage, but on the other, writing tests for this seems a bit silly, since it's really just supposed to be useful debug information. I wonder if I can refactor so that the calls to the log function aren't wrapped in the conditional and instead the conditional is moved to the function definition - that'd probably reduce the code coverage hit (since the tests we have now would exercise those lines) and make the code a bit cleaner |
2d3952e
to
f7fccdb
Compare
@jkayani I think moving the logic to the utility function makes sense. |
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.
Looks good!
Description
Adds a verbose output flag so that users can troubleshoot AVP usage easier. Goal is to be able to trace:
Assuming this looks worthwhile, I think it'd be good for bug reports to include this info to help us debug what's happening. This PR amends the issue template to say that.
Fixes: #307
Checklist
Please make sure that your PR fulfills the following requirements:
go mod tidy -compat=1.17
to ensure only the minimum is pulled in.Type of Change
Other information