Skip to content
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

job-info: Allow reading more than eventlog #2114

Merged
merged 5 commits into from Apr 5, 2019

Conversation

@chu11
Copy link
Contributor

commented Apr 4, 2019

part 2 peeled off from #2107. This supports reading more than just the eventlog via the job-info module. The bulk read isn't in this PR. This adds the flux job info command. There some extra cleanup in this PR at the end.

chu11 added 5 commits Mar 28, 2019
In main lookup cb, pre-check that the owner of the instance sent
the RPC, which allows us to remove a guest access check for future
job info retrieval.
Support ability to get jobspec, J, and R from a job.
Remove flux_job_eventlog_lookup() and flux_job_eventlog_lookup_get()
which had only one caller in flux-job.  Adjust flux-job appropriately.
@codecov-io

This comment has been minimized.

Copy link

commented Apr 5, 2019

Codecov Report

Merging #2114 into master will increase coverage by 0.03%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #2114      +/-   ##
==========================================
+ Coverage   80.23%   80.27%   +0.03%     
==========================================
  Files         201      201              
  Lines       31618    31663      +45     
==========================================
+ Hits        25369    25417      +48     
+ Misses       6249     6246       -3
Impacted Files Coverage Δ
src/common/libjob/job.c 76.42% <ø> (-1.72%) ⬇️
src/cmd/flux-job.c 84.63% <79.41%> (-0.34%) ⬇️
src/modules/job-info/lookup.c 68.31% <84.37%> (+3.84%) ⬆️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/cmd/flux-module.c 83.96% <0%> (-0.24%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
src/common/libflux/reactor.c 92.08% <0%> (+0.22%) ⬆️
src/common/libflux/message.c 81.51% <0%> (+0.36%) ⬆️
src/connectors/local/local.c 89.28% <0%> (+0.71%) ⬆️
src/common/libflux/msg_handler.c 90.23% <0%> (+1.17%) ⬆️
... and 2 more
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

LGTM, and I think all stuff we discussed in #2107 so merging.

It's not part of this PR but one thing offhand I noticed just now when merging is the job tests are setting errno = EINVAL before testing that a failing case sets errno to EINVAL. I think it must have been meant to set errno = 0? O/w it's not really testing that the function did what it's supposed to on failure. Might be good to address cleanup in the next PR in this series.

@garlick garlick merged commit 566f39b into flux-framework:master Apr 5, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 81.81% of diff hit (target 80.23%)
Details
codecov/project 80.27% (+0.03%) compared to 18a16ac
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

It's not part of this PR but one thing offhand I noticed just now when merging is the job tests are setting errno = EINVAL before testing that a failing case sets errno to EINVAL. I think it must have been meant to set errno = 0? O/w it's not really testing that the function did what it's supposed to on failure. Might be good to address cleanup in the next PR in this series.

Doh! Good catch, will fix in next PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.