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

Upgrade protoc, grpc, go, alpine and gogo versions #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bogdandrutu
Copy link

No description provided.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Author

@pavolloffay @jpkrohling can you please help me with this PR, and consider to do a release? I want to update generated model file in the jaeger in the end :)

@jpkrohling
Copy link

We have an issue on rebasing this repo with the origin (#17). Would you be interested in working on that instead? This would be the proper solution.

@bogdandrutu
Copy link
Author

@jpkrohling I can help with that after, but I need this change soon if possible.

@jpkrohling
Copy link

As long as CI is happy, I'm also happy with this change, but @pavolloffay should also take a look, as he has also dealt with this in the past few days.

@bogdandrutu
Copy link
Author

The problem is the newer gogoproto+protoc generate a new MarshalToSizedBuffer and it is impossible to embed the model.proto in another proto that is generated with a newer gogoproto+protoc.

@jpkrohling
Copy link

Yeah, I think that's one of the issues we faced, but not the only one, IIRC.

@bogdandrutu
Copy link
Author

@jpkrohling @pavolloffay you may consider to use https://github.com/open-telemetry/build-tools/blob/master/protobuf/README.md, which is a bit newer (includes this change at least). And we may solve the issue in one place

@pavolloffay
Copy link
Member

I am +1 on unifying the OTEL proto generation with Jaeger. Is https://github.com/open-telemetry/build-tools/blob/master/protobuf/README.md used in the OTEL collector? I could not find a reference to it there.

@jpkrohling
Copy link

What's the current agreement on this? Will we first rebase this to the latest version from the fork, and then push the changes downstream to OpenTelemetry's, or will this repository here be deprecated in favor of OpenTelemetry's right away? If we'll deprecate this, we should first rebase OpenTelemetry's then, IMO.

@yurishkuro
Copy link
Member

It is not clear from #20 (comment) that OTEL version will work. If OTEL docker image is ready and we can successfully run in in jaeger repo, then +1 to switch.

@bogdandrutu
Copy link
Author

@yurishkuro we made a release of the first version. I can give a try to use it in one of the go repositories.

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.

4 participants