feat: adds merge_job_parameter_definitions() #32
Conversation
ddneilson
force-pushed
the
ddneilson/16563
branch
3 times, most recently
from
d82c8a6
to
f5dc085
Compare
ddneilson
force-pushed
the
ddneilson/16563
branch
from
f5dc085
to
d18dd61
Compare
ddneilson
force-pushed
the
ddneilson/16563
branch
from
d18dd61
to
e031499
Compare
ddneilson
force-pushed
the
ddneilson/16563
branch
from
e031499
to
118e968
Compare
| @@ -127,34 +125,49 @@ def preprocess_job_parameters( | |||
| """ | |||
| if job_template.version not in (SchemaVersion.v2023_09,): | |||
| raise NotImplementedError(f"Not implemented for schema version {job_template.version}") | |||
| if environment_templates and any( | |||
| env.version not in (SchemaVersion.v2023_09,) for env in environment_templates | |||
There was a problem hiding this comment.
It looks like SchemaVersion.v2023_09 is jobtemplate-2023-09. This should be comparing with SchemaVersion.ENVIRONMENT_v2023_09?
There was a problem hiding this comment.
Yeah, this is a bigger change. I need to split the current SchemaVersion -- it's trying to do too much. I need a TemplateVersion that'll be the 'jobtemplate-2023-09' type values and a PublicationVersion that'll be the "spec publication" (i.e. v2023-09). The .version field of the models will become PublicationVersion
It was already on the list; just not in this PR. :-)
There was a problem hiding this comment.
I could actually change this to
| env.version not in (SchemaVersion.v2023_09,) for env in environment_templates | |
| env.specificationVersion not in (SchemaVersion.ENVIRONMENT_v2023_09,) for env in environment_templates |
If that would be clearer. Otherwise, this'll change with a refactor that's coming up anyways.
There was a problem hiding this comment.
I see, yeah. I think the change does make it more clear, the difference between "version" and "specificationVersion" is pretty subtle.
ddneilson
force-pushed
the
ddneilson/16563
branch
from
118e968
to
388e858
Compare
When creating/running a Job, that Job is defined with a Job Template but can also include one or more externally-defined Environment Templates. This adds support for including these Environment Templates when creating a job via create_job(), checking job parameters via preprocess_job_parameters(), and adds a new function that can be used just for checking compatibility (merge_job_parameter_definitions()). Signed-off-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>
ddneilson
force-pushed
the
ddneilson/16563
branch
from
388e858
to
52c2053
Compare
What was the problem/requirement? (What/Why)
When creating/running a Job, that Job is defined with a Job Template but can also include one or more externally-defined Environment Templates. The library does not currently support that.
What was the solution? (How)
This adds support for including these Environment Templates when creating a job via create_job(), checking job parameters via preprocess_job_parameters(), and adds a new function that can be used just for checking compatibility (merge_job_parameter_definitions()).
What is the impact of this change?
More complete featureset
How was this change tested?
I added unit test coverage for the new code paths. I also noticed that
create_job()didn't actually have any dedicated testing so I added tests for it.Was this change documented?
N/A
Is this a breaking change?
No; not intentionally.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.