-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
"vault operator usage" CLI for client count reporting #10365
Conversation
Help output:
|
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 really good!
Should any of this stuff be unit tested?
I don't know if I can get testVaultServer() to return anything sensible for unit testing. |
command/operator_usage.go
Outdated
} | ||
|
||
if resp == nil || resp.Data == nil { | ||
c.UI.Warn("No data is available for the given time range.") |
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.
In cases where a user only provides an end date or a start date (and not both), would it be possible to show the time range itself? E.g. if a user gives an end date of October 31, we say "No data is available for the given time range START to END." It gives some visibility into what's actually being done and may help the user form their search better.
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.
I like this suggestion, but I'm having a hard time implementing it, because all the logic is on the server side, and not returned in the current API (which just does a 204 no content response.)
Retrieving the default_reporting_period would require an extra round trip.
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.
Instead, I added a check whether any queries were available, using the existing API. So we are paying that round trip but I still felt it was the wrong direction to reproduce the range-calculation logic in the client. (But I still agree that would be the clearest; maybe it makes sense to change the API response when no data is available.)
Can you add a changelog file for this please? |
* Working draft of CLI command. * Sort order, robustness checking. * Text edits and check of queries_available. * Added changelog.
Here's the CLI in action: