Skip to content

Correct naming of com_github_lyft_protoc_gen_star go_repository#31169

Merged
htuch merged 2 commits intoenvoyproxy:mainfrom
alexbozhenko:upd_protoc_gen_star_name
Dec 11, 2023
Merged

Correct naming of com_github_lyft_protoc_gen_star go_repository#31169
htuch merged 2 commits intoenvoyproxy:mainfrom
alexbozhenko:upd_protoc_gen_star_name

Conversation

@alexbozhenko
Copy link
Contributor

@alexbozhenko alexbozhenko commented Dec 4, 2023 •
edited
Loading

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 alexbozhenko@gmail.com

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Dec 4, 2023

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
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Dec 4, 2023

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.

update
22dd6c7 
Signed-off-by: Alex Bozhenko <abozhenko@pinterest.com>
@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

alexbozhenko commented Dec 5, 2023

@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

alexbozhenko commented Dec 6, 2023

@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

htuch
htuch approved these changes Dec 7, 2023
View reviewed changes
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

alexbozhenko commented Dec 11, 2023

/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexbozhenko @htuch @phlax