-
-
Notifications
You must be signed in to change notification settings - Fork 97
Restructure vast status #995
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
Conversation
41f154a
to
e362055
Compare
bd48107
to
710245d
Compare
69f2da4
to
a5a0928
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.
First review round; this time code only. Still compiling on the side to give feedback to the output itself.
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.
From a user POV, I've noticed a few things:
Things are no longer grouped by components. The output is much easier to parse if the info, verbose and debug objects are merged like this:
vast status -v debug | jq '.info * .debug * .verbose'
I think we should just do this by default. Having .info.archive
and .debug.archive
separated from each other makes no sense as a user.
For the components, the names are doubled as keys. In a future PR, I'd like to see a components: [...]
array containing all the components, with components[<index>].name
containing the name. Right now, <name>.name
is always "<name>"
.
The helptext of vast status
should show what unit the memory usage values are in.
a5a0928
to
1233c7e
Compare
e335396
to
0c241d8
Compare
21a8d40
to
1872693
Compare
1872693
to
f98f91e
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.
Can you clean up the includes and the guarding #if
s in libvast/src/detail/process.cpp
? There are some seemingly unnecessary includes, and they should be moved to the top.
We also shouldn't use both __linux__
and VAST_LINUX
, only the latter should be used for guarding includes.
03cea48
to
3fdc413
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.
Thanks! 🙏
I verified locally that this looks as we discussed.
No description provided.