Skip to content

Conversation

@elharo
Copy link
Contributor

@elharo elharo commented Sep 26, 2018

It's not immediately obvious why these two pom.xml files separately declare versions.

@elharo elharo requested a review from pongad as a code owner September 26, 2018 18:33
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2018
Copy link
Contributor

@pongad pongad left a comment

Choose a reason for hiding this comment

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

Wait sorry. I think we need to update GAX's gRPC as well right...?

@elharo
Copy link
Contributor Author

elharo commented Sep 26, 2018

Possibly. This is an experiment to see if it breaks. Not quite ready to commit to this yet. One thing I'm curious about is why we're setting these versions in multiple places.

@pongad
Copy link
Contributor

pongad commented Sep 26, 2018

@elharo I think it's because the api-grpc and cloud-clients don't share a parent. This might just be someting carried over from when they were in different repositories. @vam-google Can you comment?

@elharo
Copy link
Contributor Author

elharo commented Sep 26, 2018

Yes, that seems right. Should they share a parent? or perhaps each import a BOM that manages the dependencies for both?

@elharo
Copy link
Contributor Author

elharo commented Sep 26, 2018

Theoretically we shouldn't need to upgrade GAX at the same time since it's on 1.13.1 and 1.15.0 (minor version bump) is supposed to be backwards compatible with that. But I wouldn't bet heavily on it.

@elharo elharo requested a review from a team as a code owner October 15, 2018 22:10
@vam-google
Copy link
Contributor

vam-google commented Oct 15, 2018

@elharo, @pongad google-api-grpc and google-cloud-clients have different group ids (com.google.api.grpc and com.google.cloud respectively). Putting those under same parent is against maven's naming conventions (i'm not even sure that it will compile/publish, or at least not output severe warnings in the console on build). This separation is historical (happened much earlier than grpc packages migration to google-cloud-java). It partially makes sense (since all the grpc packages are not necessarily about cloud). To adhere to maven naming convention the parent must be in group com.google but that is too generic (like defining a parent pom for the whole google under google-cloud-java).

So the current separation is by design. It is annoying to have to repeat same dependency multiple times, but unfortunately it is not the only place (there are many dependencies duplications, not only in this repo), so fixing it here will not fix the actual problem. How to fix that dependency duplication problem is actually a very complicated problem (out of scope here).

(one of the first googled stackoverflow links regarding parent-child groupId relation)

@elharo
Copy link
Contributor Author

elharo commented Oct 15, 2018

@vam-google Do you have a reference for not putting different group IDs under the same parent? Happy not to do it here, but it may be relevant for some PRs in other repos I'm looking at.

@vam-google
Copy link
Contributor

When I was doing it I remember that putting those under same parent didn't really work (I'm not saying it did not compile, but it caused more problems than solved). As for the reference, the one, with pretty weak wording (describing it only as "A good way", not a "must") is in the maven documentation:

"A good way to determine the granularity of the groupId is to use the project structure. That is, if the current project is a multiple module project, it should append a new identifier to the parent's groupId. For example, org.apache.maven, org.apache.maven.plugins, org.apache.maven.reporting.

Notice, that children must share prefix, the only way for our case to share prefix is to have com.google group as a parent, which is too generic. Aslo there were other reasons to keep those separate (having common parent solves some problems, but also makes the whole project structure more monolithic).

@elharo
Copy link
Contributor Author

elharo commented Oct 16, 2018

Looks like the CI hung. Anyone happen to know how to force it to restart?

@garrettjonesgoogle
Copy link
Member

@chingor13 how do we trigger Kokoro to run?

@chingor13
Copy link
Contributor

chingor13 commented Oct 16, 2018

We add the kokoro:force-run label and Kokoro will pick it up.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 16, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 16, 2018
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

LGTM, @garrettjonesgoogle can merge when gax is updated as well.

@garrettjonesgoogle
Copy link
Member

It's fine to merge this before GAX

@elharo elharo merged commit a45fe30 into master Oct 16, 2018
@elharo elharo deleted the grpc branch October 16, 2018 21:32
@JesseLovelace JesseLovelace mentioned this pull request Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants