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

M2 NIO wdl -- updated for Firecloud pet service accounts #4710

Merged
merged 8 commits into from
Apr 26, 2018

Conversation

davidbenjamin
Copy link
Contributor

@LeeTL1220 Ready for review. I will concurrently test in Firecloud.

Copy link
Contributor

@LeeTL1220 LeeTL1220 left a comment

Choose a reason for hiding this comment

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

Looks okay. I made a comment, but it does not necessarily require action.

I compared this to the non-NIO version of the M2 WDL (in this branch/master) for my review.

File? pon
Int scatter_count
File? gnomad
File? variants_for_contamination
Copy link
Contributor

Choose a reason for hiding this comment

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

Why eliminate all the index files? Seems like that will make it harder to keep the files in sync. Your call.

@LeeTL1220 LeeTL1220 assigned davidbenjamin and unassigned LeeTL1220 Apr 26, 2018
@LeeTL1220
Copy link
Contributor

If it works in FC, then feel free to merge. Addressing my comment is your decision.

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #4710 into master will increase coverage by 0.006%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             master     #4710       +/-   ##
==============================================
+ Coverage     79.85%   79.856%   +0.006%     
- Complexity    17339     17341        +2     
==============================================
  Files          1074      1074               
  Lines         62932     62932               
  Branches      10183     10183               
==============================================
+ Hits          50251     50255        +4     
+ Misses         8703      8699        -4     
  Partials       3978      3978
Impacted Files Coverage Δ Complexity Δ
...utils/smithwaterman/SmithWatermanIntelAligner.java 90% <0%> (+40%) 3% <0%> (+2%) ⬆️

@davidbenjamin
Copy link
Contributor Author

Working on Firecloud after one tweak: if [ -f ${normal_bam} ] doesn't work when normal_bam is a String because nothing is localized, so I replaced it with if [ ! -z ${normal_bam} ]. I will merge around 6 am after Travis tests hopefully pass.

@davidbenjamin davidbenjamin merged commit 3e41aa5 into master Apr 26, 2018
@davidbenjamin davidbenjamin deleted the db_m2_nio_wdl branch April 26, 2018 10:51
cwhelan pushed a commit to cwhelan/gatk-linked-reads that referenced this pull request May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants