src: remove flux_account_shares.cpp in favor of just using -t option with view-bank
#471
Conversation
There was a problem hiding this comment.
Looks good, two small comments.
Also, depending on what your use-case is, have you considered an option to output the hierarchy info as JSON or to allow users to specify what information they want printed out and in what format? Like flux jobs does:
flux jobs ƒ2WFJyd8B -o"{id}|{username}"
JOBID|USER
ƒ2WFJyd8B|corbett8
| @@ -1,7 +1,6 @@ | |||
| # This list is included in both TESTS and dist_check_SCRIPTS. | |||
| TESTSCRIPTS = \ | |||
| t0000-sharness.t \ | |||
| t1000-print-hierarchy.t \ | |||
There was a problem hiding this comment.
It looks like this file isn't deleted, even though it's removed from the testsuite?
There was a problem hiding this comment.
Oh yeah, I forgot to delete this file. Thanks for catching that
| + str(user[2]) | ||
| + "|" | ||
| + str(user[3]) | ||
| + "\n" |
There was a problem hiding this comment.
did you consider f"{indent} {bank}|{user[0]}|{user[1]}|{user[2]}|{user[3]}\n"?
There was a problem hiding this comment.
I didn't even consider f-strings 🤦 thank you for pointing that out! I'm gonna switch to using these instead; much cleaner
Problem: The "view-bank" command does not have an option to print out
the database hierarchy in a parsable format.
Add an option to print out a pipe-delimited ('|') hierarchy of the
flux-accounting database, starting with the bank passed in.
Problem: flux-accounting has two identical ways to print out the hierarchy of its accounting database: the flux account-shares command (a .cpp file) and an optional argument passed into the "view-bank" command. It would be less confusing to have two different options that do the same thing, especially since the .cpp file requires a dash '-' in between "account" and "shares", which doesn't match the rest of the flux-accounting commands. Convert the sharness tests in t1000-print-hierarchy.t into multiple, smaller sets of tests. Use the "-t" option from the "view-bank" command instead of calling "flux account-shares". Since the "-t" optional argument in the "view-bank" command prints job usage values with more precision, edit the expected files to match this precision.
Problem: There are a couple of instances throughout the testsuite that call "flux account-shares", but there is a cleaner option to just use the "-t" optional argument from the "view-bank" command. Convert the calls from "flux account-shares" to just use "view-bank -t". Edit the expected files in the testsuite to account for the precision that "view-bank -t" includes in its output of the database hierarchy.
Problem: flux_account_shares.cpp is no longer needed by flux-accounting since its function is replicated by the "-t" option in the "view-bank" command. Remove flux_account_shares.cpp from flux-accounting as well as any references to it throughout documentation.
cmoussa1
force-pushed
the
rm.flux-account-shares
branch
from
d6e72e9 to
3cf6282
Compare
|
Thanks for reviewing @jameshcorbett! I've force-pushed up some fixes/changes based on your suggestions.
This is a good suggestion. Setting MWP here shortly |
Problem: flux_account_shares.cpp was removed in flux-framework#471, but there is still a target definition for it in the top-level Makefile.am that is left over. Remove the left over targets.
Problem: flux_account_shares.cpp was removed in flux-framework#471, but there is still a compile definition for it in the top-level Makefile.am that is left over. Remove the left over compilation instructions.
Problem
flux-accounting has two identical ways to print out the hierarchy of its accounting database: the
flux account-shares command(a.cppfile) and an optional argument passed into theview-bankcommand. It would be less confusing to have two different options that do the same thing, especially since the.cppfile requires a dash-in between "account" and "shares", which doesn't match the rest of the flux-accounting commands.This PR removes
flux_account_shares.cppfrom flux-accounting and converts the various calls in its testsuite to just use the-toption of theview-bankcommand. It also converts the expected files used for these tests to match the higher precision output from theview-bankcommand, particularly the job-usage values. I've added a-P/--parsableoption toview-bankto print out hierarchies in a pipe-delimited (|) format.I've also broken up
t1000-print-hierarchy.tinto multiple, smaller tests.