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

Correct naming of com_github_lyft_protoc_gen_star go_repository #31169

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

alexbozhenko
Copy link
Contributor

@alexbozhenko alexbozhenko commented Dec 4, 2023

Commit Message:Correct naming of com_github_lyft_protoc_gen_star go_repository
Additional Description:
Similar to bufbuild/protoc-gen-validate#900
correct naming should include the major version.

https://go.dev/ref/mod#module-path

If the module is released at major version 2 or higher, the module path must end with a major version suffix like /v2. This may or may not be part of the subdirectory name. For example, the module with path golang.org/x/repo/sub/v2 could be in the /sub or /sub/v2 subdirectory of the repository golang.org/x/repo.

So go_repository stanza also should include the major version, to distinguish between the different major versions of the module, that can coexist.

After the change, the naming is consistent:

$ rg 'com_github_lyft_protoc_gen_star'
bazel/dependency_imports.bzl
128:        name = "com_github_lyft_protoc_gen_star_v2",

api/bazel/repository_locations.bzl
27:            "com_github_lyft_protoc_gen_star_v2",

Risk Level: None
Testing: CI passes...?
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Alex Bozhenko [email protected]

Copy link

Hi @alexbozhenko, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #31169 was opened by alexbozhenko.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Dec 4, 2023
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #31169 was opened by alexbozhenko.

see: more, trace.

Signed-off-by: Alex Bozhenko <[email protected]>
@alexbozhenko alexbozhenko force-pushed the upd_protoc_gen_star_name branch from e765253 to 22dd6c7 Compare December 4, 2023 20:21
@alexbozhenko
Copy link
Contributor Author

@fishcakez @JuniorHsu

@htuch
Copy link
Member

htuch commented Dec 6, 2023

Seems fine, but maybe I'm a bit dense - how does the Bazel internal name matter?

@alexbozhenko
Copy link
Contributor Author

@htuch: we have a setup where we import this repo and use envoy_dependency_imports from this repo. On top of that, we have some extra go code, that also has a dependency on protoc_gen_star, but on v1 of it. Because the names of those two go_repositroes are the same, we have a conflict, similar to what I described at bufbuild/protoc-gen-validate#900

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

OK, seems harmless and technically right.

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Dec 7, 2023
@htuch htuch enabled auto-merge (squash) December 7, 2023 03:51
@alexbozhenko
Copy link
Contributor Author

/retest

@phlax
Copy link
Member

phlax commented Dec 11, 2023

@alexbozhenko unfortunately we have just updated our ci framework so /retest wont work to resolve - you will need to merge main

@htuch htuch merged commit 8c5720f into envoyproxy:main Dec 11, 2023
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants