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
Collaborator

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 a team as code owners December 16, 2020 11:33
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 16, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Dec 16, 2020
@olavloite
Copy link
Collaborator Author

@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 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

Choose a reason for hiding this comment

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

@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
Collaborator Author

@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

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

@olavloite
Copy link
Collaborator Author

@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

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

@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
@thiagotnunes thiagotnunes deleted the grpc-alts-as-test-dep branch December 21, 2020 01:07
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
This is an auto-generated regeneration of the .pb.go files by
cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will
update the corresponding PR to depend on the newer version of go-genproto, and
assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot
will not create any more regeneration PRs. If all regen PRs are closed,
gapicgen will create a new set of regeneration PRs once per night.

If you have been assigned to review this PR, please:

- Ensure that CI is passing. If it's failing, it requires your manual attention.
- Approve and submit this PR if you believe it's ready to ship. That will prompt
genbot to assign reviewers to the google-cloud-go PR.

Corresponding google-cloud-go PR: googleapis/google-cloud-go#5501

Changes:

feat(redis): add secondary_ip_range field
  PiperOrigin-RevId: 427771855
  Source-Link: googleapis/googleapis@fed73d4

docs(accessapproval): added resource annotations feat: added flag for implicit dismissal for `DismissDecision`
  PiperOrigin-RevId: 427766265
  Source-Link: googleapis/googleapis@5556c91

docs(billing/budgets): Formatting change from HTML to markdown; Additional clarifications
  PiperOrigin-RevId: 427761663
  Source-Link: googleapis/googleapis@46c5d65

chore: regenerate API index

  Source-Link: googleapis/googleapis@14fe023

feat(osconfig): Update v1beta protos with recently added features. PatchRollout proto, mig_instances_allowed field to PatchConfig, UpdatePatchDeployment RPC,PausePatchDeployment and ResumePatchDeployment pair of RPCs
  PiperOrigin-RevId: 427579992
  Source-Link: googleapis/googleapis@250797d

chore: regenerate API index

  Source-Link: googleapis/googleapis@d450beb

feat(googleads): Protos and build files for Google Ads API v10
  Committer: @aohren
  PiperOrigin-RevId: 427535047
  Source-Link: googleapis/googleapis@553cb54

chore: regenerate API index

  Source-Link: googleapis/googleapis@978e373

feat: Initial release of Video Stitcher API v1 Public Preview
  PiperOrigin-RevId: 427509057
  Source-Link: googleapis/googleapis@b409abd

chore: regenerate API index

  Source-Link: googleapis/googleapis@95b6277

feat(binaryauthorization): Updates the summary of Binary Authorization to include more recently supported platforms, Anthos clusters on VMware and Cloud Run
  PiperOrigin-RevId: 427481635
  Source-Link: googleapis/googleapis@1bc2eca

chore: regenerate API index

  Source-Link: googleapis/googleapis@48b6d01

feat: Adding Certificate Manager v1 API See public documentation at https://cloud.google.com/certificate-manager/docs
  PiperOrigin-RevId: 427451561
  Source-Link: googleapis/googleapis@d5d129f

chore: regenerate API index

  Source-Link: googleapis/googleapis@7a961f3

feat(datacatalog): Add methods and messages related to starring feature feat: Add methods and messages related to business context feature docs: Updates copyright message
  PiperOrigin-RevId: 427140868
  Source-Link: googleapis/googleapis@0f56b69

chore: regenerate API index

  Source-Link: googleapis/googleapis@c507c8d

feat(osconfig): Update osconfig v1 protos
  PiperOrigin-RevId: 427050266
  Source-Link: googleapis/googleapis@010716c

chore: regenerate API index

  Source-Link: googleapis/googleapis@6d0be82

fix!: Remove deprecated v1beta1 API that is no longer available BREAKING CHANGE:
  PiperOrigin-RevId: 426957789
  Source-Link: googleapis/googleapis@42d9ed3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants