feat: Implement the task chunking RFC 0001 #168
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mwiebe
force-pushed
the
rfc-0001-task-chunking
branch
2 times, most recently
from
fffedf4 to
a2b335c
Compare
Signed-off-by: Mark Wiebe <[email protected]>
mwiebe
force-pushed
the
rfc-0001-task-chunking
branch
from
a2b335c to
edfc0e3
Compare
|
jusiskin
left a comment
jusiskin
left a comment
There was a problem hiding this comment.
Great job. Just a few nitpicks, but I don't consider them blocking. Feel free to address them or merge as-is.
| # If the string value has no expressions, can validate the value now. | ||
| # Otherwise will validate when |
There was a problem hiding this comment.
nit: this comment is unfinished
| pytest.param( | ||
| { | ||
| "name": "foo", | ||
| "type": "CHUNK[INT]", | ||
| "range": "1-100", | ||
| "chunks": { | ||
| "defaultTaskCount": 10, | ||
| "targetRuntimeSeconds": "{{Param.TargetChunkRuntime}}", | ||
| "rangeConstraint": "NONCONTIGUOUS", | ||
| }, | ||
| }, | ||
| id="targetRuntimeSeconds is str expression", | ||
| ), |
There was a problem hiding this comment.
nit: Should we also have a case where defaultTaskCount is a format string? I see it is already covered in test/openjd/model/v2023_09/test_create.py, but maybe for completeness it would be good to have here as well.
| }, | ||
| "Value must be an integer or integer string.", | ||
| 1, | ||
| id="disallow floats", |
There was a problem hiding this comment.
nit: should this mention "range"? same applies to test cases below this
| # Reject the string if it contains any expressions. | ||
| if len(item.expressions) == 0: | ||
| try: | ||
| int(item) | ||
| except ValueError: | ||
| errors.append( | ||
| InitErrorDetails( | ||
| type="value_error", | ||
| loc=(i,), | ||
| ctx={ | ||
| "error": ValueError( | ||
| "String literal must contain an integer." | ||
| ) | ||
| }, | ||
| input=item, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
nit: this code comment doesn't match the implementation. It seems we are only rejecting strings if they have no expressions and the string cannot be parsed as an integer.
Thanks! I'll merge this and then open a smaller PR to fix up what you found. |
4 participants
What was the problem/requirement? (What/Why)
https://github.com/OpenJobDescription/openjd-specifications/blob/mainline/rfcs/0001-task-chunking.md
What was the solution? (How)
Implement the task chunking additions as the TASK_CHUNKING extensions, using the context-sensitive parsing introduced by the Pydantic V2 conversion and extensions RFC implementation.
What is the impact of this change?
Job templates that use the task chunking feature can be parsed successfully.
How was this change tested?
Added tests for the new functionality.
Yes
Was this change documented?
Yes
Is this a breaking change?
Not a breaking change, it adds support for the TASK_CHUNKING extension.
Does this change impact security?
This does not impact the threat model, and the new extensions field and parameter follow the same level of input validation relevant for existing threats.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.