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

Update jobspec command to be list instead of list or string #549

Merged
merged 3 commits into from Dec 6, 2019

Conversation

@chu11
Copy link
Contributor

chu11 commented Dec 2, 2019

Per RFC PR flux-framework/rfc#215, and follow up in flux-core of flux-framework/flux-core#2564, the command in a jobspec shall be a list and not a list OR string.

The work in this PR is "done", but I've labeled it as "WIP" until the RFC change is approved. This was initially done mostly to see "how much do we have to change". Ends up not too much. Some parsing in libjobspec had to be tweaked, but that was it code wise. Update jobspecs throughout the tests and added an extra test for a bad jobspec with command as a string.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #549 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
- Coverage   75.83%   75.82%   -0.01%     
==========================================
  Files          70       70              
  Lines        7001     6999       -2     
==========================================
- Hits         5309     5307       -2     
  Misses       1692     1692
Impacted Files Coverage Δ
resource/libjobspec/jobspec.cpp 85.66% <100%> (+0.26%) ⬆️
resource/utilities/command.cpp 72.38% <0%> (-0.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e1c65e...da36396. Read the comment docs.

@chu11 chu11 changed the title [WIP] Update jobspec command to be list instead of list or string Update jobspec command to be list instead of list or string Dec 2, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 2, 2019

Removed WIP as flux-framework/rfc#215 has been merged.

Should wait for flux-framework/flux-core#2564 to be merged first.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 2, 2019

We may want to wait to merge this PR until after v0.8 is tagged, just because we're not also tagging a version of flux-core with this change. (Though likelihood of a problem is low)

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Dec 4, 2019

@chu11: Just FYI -- PR #548 added one additional jobspec.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Dec 5, 2019

PR #548 has been merged.

chu11 added 3 commits Nov 27, 2019
Update per RFC14 changes, where commands are no longer allowed to be strings.
Commands must be arrays.
Per update to RFC14, update all jobspecs to have commands as an array and
not a string.
Add test to ensure if command is a string, it is considered an
invalid jobspec.
@chu11 chu11 force-pushed the chu11:rfcissue213 branch from 774464f to da36396 Dec 5, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 5, 2019

re-based & re-pushed with updates to the new jobspecs recently merged in (update and granule jobspecs)

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Dec 5, 2019

On a second thought, it is okay to merge this in before 0.8 tag. The scheduler doesn't make use of the task section at all so this is harmless. Thoughts?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Dec 6, 2019

OK, I reviewed this PR and this looks great to me. I will merge this in and then rebase PR #546.

@dongahn dongahn merged commit d4a2a75 into flux-framework:master Dec 6, 2019
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 75.83%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +24.16% compared to 0e1c65e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 6, 2019

On a second thought, it is okay to merge this in before 0.8 tag. The scheduler doesn't make use of the task section at all so this is harmless. Thoughts?

I was more worried that the change to libjobspec would reject valid jobspec possibly generated by flux-core v0.13 (before the change to command key) when flux-sched-v0.8 was used with this version.

I do think the probability of that is quite low and it will be obvious what happened, so I'm not worried.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Dec 6, 2019

Sorry I jumped the gun. Yeah hopefully this isn't an issue in practice.

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