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

[BEAM-551] Add utility to handle JSON option manipulation #1105

Closed
wants to merge 3 commits into from
Closed

[BEAM-551] Add utility to handle JSON option manipulation #1105

wants to merge 3 commits into from

Conversation

sammcveety
Copy link
Contributor

Provide a utility to be used with RuntimeValueProviders that allows runners to manipulate PipelineOptions by interleaving runtime parameters.

Green travis: https://travis-ci.org/sammcveety/incubator-beam/jobs/167702097

R: @dhalperi

dhalperi
dhalperi requested changes Oct 24, 2016
View reviewed changes
Copy link
Contributor

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

LGTM, modulo minor test improvements.

sdks/java/core/src/test/java/org/apache/beam/sdk/options/ValueProviderUtilsTest.java
TestOptions runtime = mapper.readValue(updatedOptions, PipelineOptions.class)
.as(TestOptions.class);
assertEquals("bar", runtime.getString());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you test with a value that gets replaced?

Copy link
Contributor

@dhalperi dhalperi Oct 24, 2016 •
edited

Choose a reason for hiding this comment

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

Additionally, with an options with multiple values, one gets preserved and another gets replaced.

And replacing with an empty map and ensuring it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dhalperi
Copy link
Contributor

dhalperi commented Oct 24, 2016 •
edited

Minor Checkstyle error.

[ERROR] /home/jenkins/jenkins-slave/workspace/beam_PreCommit_MavenVerify@2/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ValueProviderUtilsTest.java:33:10: Redundant 'static' modifier. [RedundantModifier]
Audit done.
[INFO] There is 1 error reported by Checkstyle 6.19 with beam/checkstyle.xml ruleset.
[ERROR] src/test/java/org/apache/beam/sdk/options/ValueProviderUtilsTest.java:[33,10] (modifier) RedundantModifier: Redundant 'static' modifier.

Checkstyle has been updated since the commit at which you originally based the PR, perhaps?

@sammcveety
Copy link
Contributor Author

I'd believe that. Fixed.

@dhalperi
Copy link
Contributor

Merged, thanks for fixes.

@asfgit asfgit closed this in c94b8ea Oct 26, 2016
@dhalperi
Copy link
Contributor

(Manually re-ran the jenkins command as this PR was sent when Jenkins was busted)

@sammcveety sammcveety mentioned this pull request Dec 13, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalperi dhalperi dhalperi requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sammcveety @dhalperi