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

API in release-1.11 not usable (protobuf dependency) #806

Open
Tanemahuta opened this issue Jan 4, 2022 · 6 comments
Open

API in release-1.11 not usable (protobuf dependency) #806

Tanemahuta opened this issue Jan 4, 2022 · 6 comments

Comments

@Tanemahuta
Copy link
Contributor

Describe the bug
The protobuf marshaller referenced by the API module is not compatible.

Steps to reproduce the issue:

  1. Create a project and add require github.com/banzaicloud/istio-operator/api/v2 v2.0.0-20211217152025-f1b654fb609f to your go.mod.
  2. Use IstioMeshGateway struct in your code.
  3. Compiling will ultimately fail.

Expected behavior
Compiling should succeed.

Additional context
Line in question is

CommonMarshaler = &github_com_gogo_protobuf_jsonpb.Marshaler{Int64Uint64asIntegers: true}
.

Maybe it would be a good idea to exclude the gogo/protobuf in all your go.mod and require the waynz0r/protobuf as a dependency in the API go.mod.

Workaround
add a replace github.com/gogo/protobuf => github.com/waynz0r/protobuf v1.3.3-0.20210811122234-64636cae0910 to your project's go.mod.

@Laci21
Copy link
Member

Laci21 commented Jan 10, 2022

Hi @Tanemahuta,

Thanks for raising this issue!

I think you have a completely valid point that it would be nice if the API was usable without any replace directives.

The thing is though that I think we cannot easily require waynz0r/protobuf instead of gogo/protobuf in the API go.mod, because gogo/protobuf is used in a lot of generated GO files in the api/v1alpha1 directory. So the code generation should be changed accordingly to generate GO files with waynz0r/protobuf usage, which does not seem trivial to me, we might need to fork GO file generator lib as well or do some other hackish stuff, but I don't think that's the best way to go.

I'd rather consult with @waynz0r about this fork whether we need to keep it or is there a better workaround to handle this dependency. @waynz0r could you please take a look at this once you are back and whenever have time?

@Tanemahuta
Copy link
Contributor Author

All fine. In the meantime adding that replace directive to the go.mod of the API would be nice 🙌 thank you

@Laci21
Copy link
Member

Laci21 commented Jan 11, 2022

All fine. In the meantime adding that replace directive to the go.mod of the API would be nice 🙌 thank you

Unfortunately, it doesn't work like that. The other dependencies used in the API go.mod use the gogo/protobuf lib and because of that even when replace github.com/gogo/protobuf => github.com/waynz0r/protobuf v1.3.3-0.20210811122234-64636cae0910 is added to the API go.mod, those other dependencies require gogo/protobuf in their own go.mod and since waynz0r/protobuf has the same go module name as gogo/protobuf at the end of the day it won't be replaced in your project unless you explicitly replace it.

➜  go mod graph | grep "github.com/gogo/[email protected]"
...
istio.io/[email protected] github.com/gogo/[email protected]
istio.io/[email protected] github.com/gogo/[email protected]

Let us know if you can make it work somehow, but I'm afraid the reality is that you need to replace it in your own project for now.

We either fork those other repos and make sure that the forks are used everywhere from the API (which is a way I don't want to go) or find a solution to omit our fork altogether.

@Tanemahuta
Copy link
Contributor Author

Tanemahuta commented Jan 11, 2022

True, I forgot about replace not being transient. Meh. A pity. At least there's a solution and it can be found when searching through the issues.

@Laci21
Copy link
Member

Laci21 commented Sep 2, 2022

@Tanemahuta, this was fixed here for the release-1.15 branch.

@Laci21 Laci21 closed this as completed Sep 2, 2022
@Laci21
Copy link
Member

Laci21 commented Sep 16, 2022

Reopening because of this PR: #868

@Laci21 Laci21 reopened this Sep 16, 2022
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

No branches or pull requests

2 participants