Fix Incorrect WDL validation #225
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 69.73% // Head: 69.51% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #225 +/- ##
=============================================
- Coverage 69.73% 69.51% -0.23%
+ Complexity 1076 1058 -18
=============================================
Files 47 47
Lines 6050 6062 +12
Branches 796 801 +5
=============================================
- Hits 4219 4214 -5
- Misses 1510 1511 +1
- Partials 321 337 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Contributor
Author
|
What the output looks like when a valid workflow is given (that is missing |
coverbeck
approved these changes
Feb 15, 2023
Contributor
coverbeck
left a comment
There was a problem hiding this comment.
This is good for the existing way we handle this. But I think in the future we should do one of these 2 things (a separate ticket, if agreed):
- If we want to do this check before launching something, we should use Cromwell instead of regular expressions.
- Without knowing the whole history behind this, it's not clear to me that we need this check at all -- why not let the runtime (Cromwell) display an error instead if there is an issue with the WDL?
| return true; //this is a valid WDL file | ||
| // check all the required fields and give error message if it's missing | ||
| if (wfFound && callFound) { | ||
| String missingPotentiallyRequiredFields = ""; |
Contributor
There was a problem hiding this comment.
You might find it easier to just have a List<String>, then use String.join().
| } else { | ||
| //WDL file but some required fields are missing | ||
| // WDL file but some required fields are missing | ||
| String missingPotentiallyRequiredFields = ""; |
Contributor
There was a problem hiding this comment.
Could use List as well here and next line.
|
Kudos, SonarCloud Quality Gate passed! |
denis-yuen
approved these changes
Feb 16, 2023
Contributor
Author
Ticket created: SEAB-5265 |
kathy-t
approved these changes
Feb 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, the
CLIdoes not accept WDL workflows that do not containtask,command, oroutputin the--entryfile. However, it is possible to have a valid WDL workflow that does not contain these fields, through the use of sub-workflows, an example given here.To fix this, I have modified the method
WDLClient.check(file)such that it only sends an error message if eitherworkfloworcallare not found in the--entryfile and it gives a warning to the user if the--entryis missingtask,command, oroutput.I am giving the warning as if the WDL is invalid, when it is launched, it will give the user a lot of error messages, but many of them are confusing, particularly if it is for a simple mistake such as forgetting to specify a
task.The warning message looks like this,
WARNING: these fields are missing from WDL file, this could be causing errors: 'task' 'command' 'output''.*The method
WDLClient.check(file)is not necessarily required, as if the files are invalid, error messages will be given when the tool is launched, but these messages can be very confusing.Review Instructions
Ensure that workflows such as the one specified in this discourse discussion are able to be run on the
dockstore-cli.This feature can be checked with
dockstore tool launch --entry [FILE PATH] --json [JSON PATH].Issue
DOCK-2245
dockstore/dockstore#5133
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
./mvnw clean install