Fix Yaml Validation for when testParameterFiles is empty or non-existent #233
Conversation
…scriptor field is not provided, or is empty, an error will be thrown by readAsDockstoreYaml12
One where the testParameterFiles filed is empty and one where the testParameterFiles field DOES NOT EXIST
- empty - non-existent
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #233 +/- ##
=============================================
- Coverage 69.48% 67.33% -2.15%
+ Complexity 1042 1005 -37
=============================================
Files 47 47
Lines 6072 6077 +5
Branches 801 802 +1
=============================================
- Hits 4219 4092 -127
- Misses 1514 1646 +132
Partials 339 339
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. 📢 Do you have feedback about the report comment? Let us know in this issue. |
dockstore-client/src/main/java/io/dockstore/client/cli/YamlVerifyUtility.java
Outdated
Show resolved
Hide resolved
| description: testing testing testing | ||
| data: | ||
| test-2: | ||
| targetDirectory: ~/test |
There was a problem hiding this comment.
How are these ~ interpreted? Were these examples from somewhere else?
There was a problem hiding this comment.
This would've just been me filling out the template. I just copied a .dockstore.yml file I had already created for this file.
However, after reading through the template, it appears this is not valid, as it requires that the targetDirectory is a relative path.
I have corrected this for all .dockstore.yml test files that I created.
I have also added this comment to this ticket to either correct DockstoreYamlHelper.validateDockstoreYamlProperties() such that it catches this in the future, or to update the template to indicate that absolute paths are acceptable.
| filePathsToCheck.addAll(testParameterFiles); | ||
| } | ||
|
|
||
| filePathsToCheck.add(tool.getPrimaryDescriptorPath()); |
There was a problem hiding this comment.
It was like this, but this whole code block duplicates code block starting at line 95. You could optionally refactor a couple of ways to avoid the duplication.
There was a problem hiding this comment.
Refactored it a bit.
Thanks for the feedback.
| } | ||
|
|
||
| missingFiles.addAll(filesExist(filePathsToCheck, basePath)); |
There was a problem hiding this comment.
Comment is for line 122/139, and it was like this, but INVALID_FILE_STRUCTURE, which translates to Your file structure has the following errors: -- I would say something like Your .dockstore.yml has the following errors: -- INVALID_FILE_STRUCTURE is slightly misleading variable name too (the structure of the file is fine in this case, it's just values that aren't good).
There was a problem hiding this comment.
Changed INVALID_FILE_STRUCTURE to MISSING_FILE_ERROR. I also changed the wording to "Your .dockstore.yml has the following errors:".
Thanks!
|
Kudos, SonarCloud Quality Gate passed! |
Description
This PR makes it such that you can have a
.dockstore.ymlfile that has either an empty or non-existenttestParameterFilesfield. This is valid and accepted by dockstore.org, but it wasn't accepted by the dockstore yaml validator (until now).Whilst completing this PR I also realized
DockstoreYamlHelper.validateDockstoreYamlProperties()did not throw an error when.dockstore.ymlfile contained noprimaryDescriptorPathThe former was already fixed, but for the ladder was not, hence why I added this code. I have created this ticket to improve
DockstoreYamlHelper.validateDockstoreYamlProperties()and left instructions to remove the fixes in the CLI for the above two errors when the issue is resolved (as they should be caught byDockstoreYamlHelper.validateDockstoreYamlProperties()).Review Instructions
Ensure that a
.dockstore.ymlfile with either an empty or non-existenttestParameterFilesfield (but with everything else correct) passes the dockstore yaml validator with no errors. As it is valid to have a.dockstore.ymlfile with either an empty or non-existenttestParameterFilesfield.Issue
dockstore/dockstore#5401
DOCK-2338
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