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

Prefixes for bad wdl's for validate, run or both #59

Closed
sckott opened this issue Jan 9, 2025 · 6 comments · Fixed by #51
Closed

Prefixes for bad wdl's for validate, run or both #59

sckott opened this issue Jan 9, 2025 · 6 comments · Fixed by #51
Labels
infrastructure Infrastructure fix to execute WDL GitHub Actions question Further information is requested

Comments

@sckott
Copy link
Collaborator

sckott commented Jan 9, 2025

via @tefirman in #53 (comment)

My proposal is thus for prefixes:

  • should fail womtool, but not cromwell run: badVal
  • should not fail womtool, but should fail cromwell run: badRun
  • should fail womtool, and should fail cromwell run: badValRun
  • should not fail womtool or cromwell run: no prefix

Another different take on this is to nest similar types of wdls within the same dir, and then we know the expectation from the parent dir the wdl's are in. e.g.,

  • good
    • helloHostname
    • helloDockerHostname
    • etc
  • badVal
    • testWdl
  • badRun
    • someWdl
  • badValRun
    • badFile

A 3rd option that I kinda like is maintaining the metadata about the various WDLs in a single file, json or yaml, whatever. e.g.,

wdls:
  badFile: 
    - womtool: fail
    - cromwell_run: succeed
  helloHostname: 
    - womtool: succeed
    - cromwell_run: succeed
  parseBatchfile: 
    - womtool: succeed
    - cromwell_run: fail
@sckott
Copy link
Collaborator Author

sckott commented Jan 9, 2025

Thoughts? @seankross @tefirman

@sckott sckott added the question Further information is requested label Jan 9, 2025
@tefirman
Copy link
Collaborator

I think I like the first approach best, followed by the second approach, not a huge fan of the third method, feels like it would be easy to forget to add a workflow to the metadata file.

I also "badVal" and "badValRun" are actually the same thing, because if a workflow doesn't validate, there's no way it's going to run successfully. So I think we would just need "badVal" and "badRun".

@sckott
Copy link
Collaborator Author

sckott commented Jan 13, 2025

Thanks @tefirman Sounds good to do 1st one, but I'll wait to see what @seankross says

I also "badVal" and "badValRun" are actually the same thing, because if a workflow doesn't validate, there's no way it's going to run successfully. So I think we would just need "badVal" and "badRun".

Okay, so we'd just have

  • should fail womtool, but not cromwell run: badVal
  • should not fail womtool, but should fail cromwell run: badRun
  • should fail womtool, and should fail cromwell run: badVal
  • should not fail womtool or cromwell run: no prefix

And for tests for cromwell run we'd know to assert that a workflow prefixed with badVal should fail on run (in addition to validate)

@seankross
Copy link
Collaborator

@sckott I think you meant to say: (@tefirman please double check this for me)

womtool pass womtool fail
cromwell pass [no prefix] [not possible]
cromwell fail fail-cromwell fail-all

@tefirman
Copy link
Collaborator

tefirman commented Jan 14, 2025

Exactly, WOMtool fail + Cromwell pass is not possible to my knowledge, so maybe it would just be:

  • [no prefix]: WOMtool pass, Cromwell pass
  • badRun: WOMtool pass, Cromwell fail
  • badVal: WOMtool fail, Cromwell fail

@tefirman tefirman added the infrastructure Infrastructure fix to execute WDL GitHub Actions label Jan 14, 2025
@sckott
Copy link
Collaborator Author

sckott commented Jan 14, 2025

Okay, great, so badRun and badVal it is. I'll proceed with this and document it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure fix to execute WDL GitHub Actions question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants