egalpin commented on pull request #16690: URL: https://github.com/apache/beam/pull/16690#issuecomment-1030114682
I apologize for the slowness on my review, trying to carve out time. I've taken a scan so far and the changes look good generally. With respect to tests, I agree that this is a tricky one to draw the lines on. We don't want to be testing every external module we might use (Ex. we don't directly test the `RestClient` methods on their own). Keeping that in mind I think we should be sure to test that the code surrounding SSLContext produces the desired behaviour in the pipeline and place no effort on testing SSLContext directly. I think it would be sufficient to show that any singular code path making use of a non-null SslConfiguration can be used to run a pipeline to read or write data. My other questions are around backward compatibility/the update story for users with existing pipelines that may need to update to this version. I haven't yet satisfied myself with an answer to the question of whether or not there are compatibility issues. A few things you could do in the mean time @clandry94 would be ensuring that the tests and `spotless` linter both pass: - You can run `./gradlew spotlessApply` from the top level of the beam project directory on your machine to apply linting changes automatically - You can run `./gradlew :sdks:java:io:elasticsearch-tests:elasticsearch-tests-7:test` to run unit tests locally, as it appears as though the new test is failing: https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/20847/testReport/org.apache.beam.sdk.io.elasticsearch/ElasticsearchIOTest/testSslConfiguration/ - Feel free to ignore the `Java_Examples_Dataflow_Java17` failure as this is expected and unrelated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
