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

deps: grpc-alts is only used for tests #757

Merged
merged 1 commit into from Dec 21, 2020
Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Dec 16, 2020

Adding grpc-alts as a runtime dependency causes the maven-flatten-plugin to add the compile time dependencies in the flattened pom. As far as I can tell, grpc-alts is (currently) only used for testing, so adding it as a test scoped dependency seems to make more sense to me.

Fixes the build error in googleapis/java-spanner-jdbc#293

@olavloite olavloite requested review from as code owners Dec 16, 2020
@google-cla google-cla bot added the cla: yes label Dec 16, 2020
@product-auto-label product-auto-label bot added the api: spanner label Dec 16, 2020
@olavloite olavloite requested a review from thiagotnunes Dec 16, 2020
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Dec 16, 2020

@mohanli-ml Is there a specific reason that grpc-alts was added as a runtime dependency instead of a test dependency?

@codecov
Copy link

@codecov codecov bot commented Dec 16, 2020

Codecov Report

Merging #757 (543148b) into master (aa701f5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #757   +/-   ##
=========================================
  Coverage     85.15%   85.15%           
  Complexity     2562     2562           
=========================================
  Files           142      142           
  Lines         13960    13960           
  Branches       1331     1331           
=========================================
  Hits          11887    11887           
- Misses         1513     1514    +1     
+ Partials        560      559    -1     
Impacted Files Coverage Δ Complexity Δ
.../google/cloud/spanner/SpannerExceptionFactory.java 82.47% <0.00%> (-2.07%) 46.00% <0.00%> (-1.00%)
...ud/spanner/SessionPoolAsyncTransactionManager.java 87.30% <0.00%> (+1.58%) 13.00% <0.00%> (+2.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa701f5...543148b. Read the comment docs.

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

@mohanli-ml Won't we need alts for the usage of direct path in production? Or is this only necessary for the tests?

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Dec 17, 2020

@mohanli-ml Won't we need alts for the usage of direct path in production? Or is this only necessary for the tests?

If it is needed for production: Why does it need to explicitly be added as a runtime dependency, as it is already a compile dependency (it's already included transitively by a couple of other dependencies)?

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Dec 17, 2020

@olavloite I wonder why didn't we add it in the test scope instead.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Dec 18, 2020

@olavloite I wonder why didn't we add it in the test scope instead.

@thiagotnunes I'm not exactly sure what you mean in this case. This PR puts it in the test scope. The current situation is that it is explicitly added as a runtime dependency, and that is what is causing some trouble downstream with the JDBC driver.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Dec 20, 2020

Yes, sorry, what I meant was why didn't we use the test scope in the first place instead of the runtime scope.

@mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Dec 21, 2020

@olavloite @thiagotnunes DirectPath use ALTS for authentication, but in this repo we only use it for tests, so I think use test scope should be good. Thanks!

@thiagotnunes thiagotnunes merged commit c8ef46f into master Dec 21, 2020
21 checks passed
@thiagotnunes thiagotnunes deleted the grpc-alts-as-test-dep branch Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants