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.

Sign up for GitHub

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

Jump to bottom

Config file no longer overrides system properties #4445

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Config file no longer overrides system properties #4445

merged 4 commits into from
Feb 27, 2018

Conversation

jonn-smith
Copy link
Collaborator

Fixes #4435
Fixes #4436

@codecov-io
Copy link

codecov-io commented Feb 23, 2018 •
edited
Loading

Codecov Report

Merging #4445 into master will increase coverage by 0.319%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #4445       +/-   ##
===============================================
+ Coverage     79.156%   79.475%   +0.319%     
- Complexity     16583     17182      +599     
===============================================
  Files           1049      1050        +1     
  Lines          59510     60950     +1440     
  Branches        9747     10190      +443     
===============================================
+ Hits           47106     48440     +1334     
- Misses          8620      8690       +70     
- Partials        3784      3820       +36
Impacted Files Coverage Δ Complexity Δ
.../main/java/org/broadinstitute/hellbender/Main.java 77.703% <ø> (+5.764%) 62 <0> (+17) ⬆️
...stitute/hellbender/utils/config/ConfigFactory.java 76.398% <100%> (-0.036%) 45 <3> (ø)
...utils/smithwaterman/SmithWatermanIntelAligner.java 50% <0%> (-40%) 1% <0%> (-2%)
...nder/cmdline/PicardCommandLineProgramExecutor.java 60% <0%> (-10%) 4% <0%> (+1%)
...e/hellbender/engine/spark/SparkContextFactory.java 71.233% <0%> (-2.74%) 11% <0%> (ø)
...nder/utils/runtime/StreamingProcessController.java 70.37% <0%> (-2.058%) 50% <0%> (ø)
...ellbender/tools/walkers/annotator/QualByDepth.java 95.745% <0%> (-1.553%) 18% <0%> (+1%)
...oadinstitute/hellbender/utils/gcs/BucketUtils.java 78.71% <0%> (-1.29%) 39% <0%> (ø)
...dinstitute/hellbender/utils/pileup/ReadPileup.java 90.496% <0%> (-0.413%) 104% <0%> (+35%)
... and 30 more

lbergelson
lbergelson approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@jonn-smith I have a few comments about the test, but they're mostly just nitpicky stylistic things. Do what you would like 👍

src/test/java/org/broadinstitute/hellbender/utils/config/ConfigIntegrationTest.java Outdated
@@ -67,54 +109,94 @@ public String getTestedClassName() {
@Test(dataProvider = "provideForTestToolWithConfigValidation")
Copy link
Member

Choose a reason for hiding this comment

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

Add the singleThreaded = true property to the test so it's never executed alongside other tests. We don't currently parallelize our tests, but it's good to mark ones that would definitely break others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

src/test/java/org/broadinstitute/hellbender/utils/config/ConfigIntegrationTest.java Outdated
}

// Run our command:
runCommandLine(args.getArgsArray());
Copy link
Member

Choose a reason for hiding this comment

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

you can pass an Args Builder directly to runCommandLine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

src/test/java/org/broadinstitute/hellbender/utils/config/ConfigIntegrationTest.java Outdated

// Create some arguments for our command:
final ArgumentsBuilder args = new ArgumentsBuilder();
args.add("--" + StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION);
Copy link
Member

Choose a reason for hiding this comment

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

these could be a little bit less verbose if you use args.addArgument(key, value)
and there's a special args.addOutput(file) too

it's fine if you prefer them this way though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I didn't realize that was an option.

src/test/java/org/broadinstitute/hellbender/utils/config/ConfigIntegrationTest.java
System.setProperty(entry.getKey(), entry.getValue().toString());
finally {
// Clear our system properties:
final List<Object> keyList = new ArrayList<>(System.getProperties().keySet());
Copy link
Member

Choose a reason for hiding this comment

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

Does System.getProperties().clear() not work to clear them all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. I'm using System.clearProperty because that's the interface that specifically says it will clear a specific key.

Copy link
Member

Choose a reason for hiding this comment

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

reasonable

build.gradle
@@ -451,6 +452,11 @@ task bundle(type: Zip) {
from("gatk")
from("README.md")
from("build/docs/tabCompletion/gatk-completion.sh")

from("src/main/resources/org/broadinstitute/hellbender/utils/config/GATKConfig.properties") {
rename 'GATKConfig.properties', 'GATKConfig.EXAMPLE.properties'
Copy link
Member

Choose a reason for hiding this comment

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

This is a definitely useful to have in our bundle. I think we need to add something to A) the readme explaining how to use this, and B) probably in the example file itself. Doesn't have to be part of this pr though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I'll add some info in the config file itself and I'll add a blurb in the Readme about it as well.

@lbergelson lbergelson assigned jonn-smith and unassigned droazen Feb 26, 2018
@jonn-smith jonn-smith assigned lbergelson and unassigned jonn-smith Feb 27, 2018
@jonn-smith
Copy link
Collaborator Author

@lbergelson Addressed comments - back for final sign-off.

@jonn-smith
Addressing code review comments.
09c454d 
Fixed a missing line in the README.md
@jonn-smith jonn-smith force-pushed the jts_system_property_config_fix_4436 branch from 15ac150 to 09c454d Compare February 27, 2018 18:01
lbergelson
lbergelson approved these changes Feb 27, 2018
View reviewed changes
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@jonn-smith looks good to me

@jonn-smith jonn-smith merged commit 3e5523b into master Feb 27, 2018
@jonn-smith jonn-smith deleted the jts_system_property_config_fix_4436 branch February 27, 2018 18:53
@jonn-smith jonn-smith mentioned this pull request Mar 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbergelson lbergelson lbergelson approved these changes

@droazen droazen Awaiting requested review from droazen

Assignees

@lbergelson lbergelson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonn-smith @codecov-io @lbergelson @droazen