TSPS-303 validate method config and update if needed #121
Conversation
| // returns true if submission is in a running state | ||
| public static boolean submissionIsRunning(Submission submission) { | ||
| return !FINAL_RUN_STATES.contains(submission.getStatus()); | ||
| } | ||
|
|
||
| /** | ||
| * validates a method config against the expected version, inputs, and outputs |
There was a problem hiding this comment.
could use a little more description here - i like that you validate not only that the expected inputs and outputs are there, but also that they're pointed to the corresponding fields in the data table. if you could note that here that'd be great.
| wdlMethodVersion); | ||
|
|
||
| // if not a valid method config, set the method config to what we think it should be. This | ||
| // shouldn't happen |
There was a problem hiding this comment.
shouldn't happen often, but it will happen once every time we change the wdlMethodVersion... unless we update it in Terra as well as in our service, which actually we could do. 🤷 it's nice to have the warning log here. wonder if we should set up a slack channel to capture warnings.
There was a problem hiding this comment.
is that something we can do -- is that a sentry thingy?
There was a problem hiding this comment.
yeah probably sentry
|
|
||
| // validate and extract parameters from working map | ||
| FlightMap workingMap = flightContext.getWorkingMap(); | ||
|
|
||
| // grab current method config and validate it | ||
| MethodConfiguration methodConfiguration = | ||
| rawlsService.getCurrentMethodConfigForMethod( |
There was a problem hiding this comment.
do we want to wrap this in try-catch retry logic like the later rawls call?
There was a problem hiding this comment.
oh good point, definitely missed this.
jsotobroad
force-pushed
the
js_TSPS-303
branch
from
1a68e89
to
6e30645
Compare
| // returns true if submission is in a running state | ||
| public static boolean submissionIsRunning(Submission submission) { | ||
| return !FINAL_RUN_STATES.contains(submission.getStatus()); | ||
| } | ||
|
|
||
| /** | ||
| * validates a method config against the expected version, inputs, and outputs it does this by |
There was a problem hiding this comment.
nit: missing period after "outputs"
| // returns true if submission is in a running state | ||
| public static boolean submissionIsRunning(Submission submission) { | ||
| return !FINAL_RUN_STATES.contains(submission.getStatus()); | ||
| } | ||
|
|
||
| /** | ||
| * validates a method config against the expected version, inputs, and outputs it does this by | ||
| * checking that the methodRepoMethod method version matches we expect and that all expected |
There was a problem hiding this comment.
- "methodRepoMethod method version" can just be "wdlMethodVersion"
- "matches what we expect"
- (next line) "inputs outputs both exist" -> "inputs/outputs exist"
| wdlMethodVersion); | ||
|
|
||
| // if not a valid method config, set the method config to what we think it should be. This | ||
| // shouldn't happen |
There was a problem hiding this comment.
yeah probably sentry
| // shouldn't happen | ||
| if (!validMethodConfig) { | ||
| logger.warn( | ||
| "found method config that was not valid for billing project: {}, workspace: {}, method name: {}, methodConfigVersion: {}", |
There was a problem hiding this comment.
nit: it's not necessarily "not valid" but it's not matching our expectations. super minor but to me "not valid" suggests there's an actual error in the setup
There was a problem hiding this comment.
but to us there is an error in the setup no?
| * @param wdlWorkflowName - name of the wdl workflow, used to construct the full wdl variable name | ||
| * @param pipelineInputDefinitions - list of input definitions for a pipeline | ||
| * @param pipelineOutputDefinitions - list of output definitions for a pipeline | ||
| * @param wdlMethodVersion - version of wdl we should be submitting |
There was a problem hiding this comment.
also validate rootEntity (or whatever it's called) that defines which data table it should read from
| .methodNamespace("namespace") | ||
| .methodVersion("1.2.3") | ||
| .methodUri("this/is/a/uri/with/a/version/1.2.3")); | ||
| // test wrong reference |
There was a problem hiding this comment.
really like how you did this!
stop using any() for rawls service tests
|
Description
We want to make sure we are using the correct wdl and inputs when submitting our pipelines. This PR adds the logic to the submission step in the flight make this possible.
Do we think this is too much to do in one step?
Jira Ticket
https://broadworkbench.atlassian.net/browse/TSPS-303