flux job info: drop multiple key support, clean up code, add man page entry #5520
Conversation
Problem: there are several tests for flux job info fetching multiple keys at once, but this feature is to be deprecated. Drop tests.
Problem: the flux job info usage message is not output if the job id is not specified, and is not reached if flux_open() fails. In preparation for deprecating multi-key support, make the following changes to the free argument parsing and usage message processing: - check the arguments before flux_open() - require exactly one key - print the usage message even if the job id is not specified - update usage message to reflect that only one key is accepted - add key descriptions to the usage message Update sharness test that checks usage message content.
Codecov Report
@@ Coverage Diff @@
## master #5520 +/- ##
========================================
Coverage 83.46% 83.47%
========================================
Files 487 487
Lines 82111 82011 -100
========================================
- Hits 68533 68456 -77
+ Misses 13578 13555 -23
|
Problem: flux-job info is overly complex. Do the following to simplify the code: - drop multi-key fetch - do the work synchronously - use job-info.update-lookup for "current" R - break up special cases into (mostly) standalone blocks with docs
garlick
force-pushed
the
flux_job_info_cleanup
branch
from
fc19459 to
dccba32
Compare
|
I woke up this morning remembering some sphinx docs I came across and reworked the man page changes. Just pushed (sphinx refs included in the commit bodies) |
doc/man1/flux-job.rst
Outdated
| RFC 34: Flux Task Map: https://flux-framework.readthedocs.io/projects/flux-rfc/en/latest/spec_34.html | ||
| :doc:`rfc:spec_34` |
There was a problem hiding this comment.
I don't understand this sentence in the commit message:
The man reference is now just the title but man doesn't support hyperlinks and the title can be googled
Oh, does this mean the reference to RFC 34 in the manpage when using man(1) is only a title?
There was a problem hiding this comment.
oops, my comment says it is outdated, so feel free to ignore if I commented on an older version of this PR
There was a problem hiding this comment.
Yes it looks like this
RESOURCES
Flux: http://flux-framework.org
14/Canonical Job Specification
not ok?
There was a problem hiding this comment.
No that is fine, it just wasn't clear (to me) from the commit message (maybe just change to "The reference in the man page is now just the RFC title ..." though not necessary if you'd rather not IMO)
|
Oh, also suggest updating the PR title - the capital "Flux job info" could be confusing, also the main change here seems to be the removal of the ability to request multiple keys, so maybe that fact should be included in the title so it appears in release notes? |
garlick
changed the title
|
Does that man page HTML formatting look like something we should use for all of the pages? I was sort of putting this forward as an example. Maybe this PR shouldn't do that, now that I think of it... |
Problem: flux job info is not documented. Add a new subsection for it.
garlick
force-pushed
the
flux_job_info_cleanup
branch
from
dccba32 to
af91239
Compare
|
Repushed with just the new man section reformatted. I'll follow up with a PR that changes formatting across section 1 and we can discuss what we want RFC references to look like in that one. |
|
I'll just go ahead and set MWP since the code changes were approved and I plan to submit a broader cleanup in man1 soon. |
Problem: In PR flux-framework#5520 "multiple key" "flux job info" tests were removed due to a feature change in "flux job info". This removed some coverage of corner cases in job-info.lookup. Add a new "job-info.lookup" RPC target helper tool and re-add a number of the previously removed "multiple key" tests.
Problem: In PR flux-framework#5520 "multiple key" "flux job info" tests were removed due to a feature change in "flux job info". This removed some coverage of corner cases in job-info.lookup. Add a new "job-info.lookup" RPC target helper tool and re-add a number of the previously removed "multiple key" tests.
Problem: In PR flux-framework#5520 "multiple key" "flux job info" tests were removed due to a feature change in "flux job info". This removed some coverage of corner cases in job-info.lookup. Add a new "job-info.lookup" RPC target helper tool and re-add a number of the previously removed "multiple key" tests.
Following up on #5464, this tries to simplify
flux job info. For a start, it usesjob-info.update-lookupto fetch the current R instead of replaying the eventlog on the original R. It also cleans up theflux-job(1)man page and adds an entry forflux job infowith descriptions of each available key.