Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Womtool: Flag to list workflow dependencies (Approach 2) [BA-3501] #5098

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

salonishah11
Copy link
Contributor

@salonishah11 salonishah11 commented Jul 31, 2019

This PR adds an optional flag -l or --list-dependencies for command validate to list the imported files in the workflow and their subworkflows.

JIRA ticket: here

Copy link
Collaborator

@aednichols aednichols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

This feature is an excellent example of where investing extra effort up front led to a very nice solution!

Copy link
Contributor

@cjllanwarne cjllanwarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments optional. LGTM

wom/src/main/scala/wom/package.scala Outdated Show resolved Hide resolved
womtool/src/main/scala/womtool/input/WomGraphMaker.scala Outdated Show resolved Hide resolved
case Right(_) => SuccessfulTermination("Success!")
case Left(errors) => UnsuccessfulTermination(errors.toList.mkString(System.lineSeparator))

def validate(main: Path, inputs: Option[Path], listDependencies: Boolean): Termination = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOL: womtool validate seems like a slightly strange command to add this flag to. Ideally I guess it would be womtool describe, if such a command existed. I guess leave it as-is for now, but I might start complaining louder if more "augmentations" get added to this command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the use case mentioned in the ticket, I figured it was supposed to be added to validate command. But we can definitely change this to womtool describe at a later point.

versionDirectory.isEmpty should be(false)
}

Option(versionDirectory.list).toList.flatten.filterNot(s => s.pathAsString.contains(".DS")) foreach { validCase =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the listAndFilter function here?

@salonishah11 salonishah11 merged commit 87f1be9 into develop Aug 7, 2019
@salonishah11 salonishah11 deleted the ss_womtool_imports_2 branch August 7, 2019 14:27
myazinn pushed a commit to EpamLifeSciencesTeam/cromwell that referenced this pull request Aug 22, 2019
TimurKustov pushed a commit to TimurKustov/cromwell that referenced this pull request Sep 2, 2019
TimurKustov pushed a commit to TimurKustov/cromwell that referenced this pull request Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants