Skip to content

Import split-repo boms into google-cloud-bom #6554

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
Merged
chingor13 merged 35 commits into from
Oct 30, 2019
Merged

Import split-repo boms into google-cloud-bom #6554

chingor13 merged 35 commits into from
Oct 30, 2019

Conversation

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Oct 23, 2019 •
edited
Loading

Rather that specifying all the artifacts from the split repos, we will import the bom artifacts that those repos provide:

  • asset
  • automl
  • core
  • datacatalog
  • datalabeling
  • dlp
  • gameservices
  • iamcredentials
  • iot
  • phishingprotection
  • speech
  • talent
  • tasks
  • texttospeech
  • trace
  • translate
  • vision
  • websecurityscanner

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 23, 2019
@chingor13 chingor13 changed the title Import boms Import split-repo boms Oct 23, 2019
@chingor13 chingor13 changed the title Import split-repo boms Import split-repo boms into google-cloud-bom Oct 23, 2019
@codecov
Copy link

codecov bot commented Oct 23, 2019 •
edited
Loading

Codecov Report

Merging #6554 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6554   +/-   ##
=========================================
  Coverage     46.35%   46.35%           
+ Complexity    28018    28002   -16     
=========================================
  Files          2613     2613           
  Lines        288075   288075           
  Branches      33774    33779    +5     
=========================================
  Hits         133528   133528           
  Misses       144308   144308           
  Partials      10239    10239

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 19d77e0...2e2d0f3. Read the comment docs.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2019
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2019
@suztomo suztomo mentioned this pull request Oct 28, 2019
Closed
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2019
@suztomo suztomo mentioned this pull request Oct 30, 2019
Closed
@chingor13 chingor13 marked this pull request as ready for review October 30, 2019 15:26
@chingor13 chingor13 requested a review from a team October 30, 2019 15:26
@chingor13 chingor13 merged commit f23d93c into googleapis:master Oct 30, 2019
@chingor13 chingor13 deleted the import-boms branch October 30, 2019 16:14
@chingor13 chingor13 mentioned this pull request Oct 30, 2019
Merged
@anuraaga anuraaga mentioned this pull request Nov 3, 2019
Closed
@anuraaga
Copy link

anuraaga commented Nov 5, 2019

Hello - as we found in gradle/gradle#11208 this PR changed the behavior of google-cloud-bom significantly. It used to be the one BOM to manage them all but now if a user wanted to use, e.g., trace, they have to add an unversioned dependency on the trace-bom too. Just want to confirm, is this the expected new behavior? I can't find any issue to provide more reasoning on the reason to use imported BOMs - if it's to reduce copy-paste, it's probably better for releasetool to inline BOMs instead of importing them to make it easier for users to use the super-bom.

@elharo
Copy link
Contributor

elharo commented Nov 5, 2019

I'm looking into this. I am not certain that Maven behavior is as gradle/gradle#11208 thinks it is.

@elharo
Copy link
Contributor

elharo commented Nov 5, 2019

Update: the comments on the gradle bug are incorrect. For Maven there is no behavior change from 0.116.0 to 0.117.0. Gradle I'm not as familiar with.

@jjohannes
Copy link

jjohannes commented Nov 5, 2019

This is working in Gradle as well. The confusion was created because grpc-google-cloud-trace-v2 is not listed in google-cloud-trace-bom-0.109.0-beta.pom. And because the individual entry was removed in this PR, there is no entry for grpc-google-cloud-trace-v2 anymore (only for grpc-google-cloud-trace-v1).

@anuraaga
Copy link

anuraaga commented Nov 5, 2019

Thanks a lot for investigating this - sorry for not confirming the contents of google-cloud-trace-bom first. Looks like it will be a relatively simple fix to just add to that bom :)

@codyoss codyoss mentioned this pull request Nov 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@BenWhitehead BenWhitehead BenWhitehead approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@chingor13 @anuraaga @elharo @jjohannes @BenWhitehead @googlebot @yoshi-kokoro