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

Add git submodule for opentelemetry-proto #987

Conversation

pcwiese
Copy link
Contributor

@pcwiese pcwiese commented Aug 3, 2020

This change replaces the proto files that were previously copied from the
repo with a direct reference using a git submodule.

People with existing repos will need to initialize the git submodule
before the build will succeed.

git submodule init
git submodule update

If you are cloning a fresh repo, add the --recursive flag to your clone command.

Fixes #652

This change replaces the proto files that were previously copied from the
repo with a direct reference using a git submodule.

People with existing repos will need to initialize the git submodule
before the build will succeed.

```
git submodule init
git submodule update
```

If you are cloning a fresh repo, add the --recursive flag to your clone command.
@pcwiese pcwiese requested a review from a team August 3, 2020 22:35
@reyang
Copy link
Member

reyang commented Aug 3, 2020

Instead of having a submodule dependency, I wonder if we could use a package dependency as it makes things a lot simpler.

FYI - this is how the same problem got solved in OpenCensus: https://github.com/census-instrumentation/opencensus-proto/tree/master/gen-python.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

I am concerned about submodule due to the management issue, I think we probably should go via a different route - having a NuGet package dependency.

@reyang reyang added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Aug 3, 2020
@pcwiese
Copy link
Contributor Author

pcwiese commented Aug 3, 2020

I'm not a huge fan of git submodules either but it does seem to be the way others are doing it (java for e.g.). If we went with a NuGet package, ideally that would actually get published by the proto repo itself and not here.

@reyang
Copy link
Member

reyang commented Aug 3, 2020

I'm not a huge fan of git submodules either but it does seem to be the way others are doing it (java for e.g.). If we went with a NuGet package, ideally that would actually get published by the proto repo itself and not here.

Yep, that's what OpenCensus did (and you can see the link in my previous comment).
We probably need to see what's the feedback from the proto folks, and worst case, having a separate repo that generates the nuget package.

@pcwiese
Copy link
Contributor Author

pcwiese commented Aug 3, 2020

From the looks of things, I don't think they will be checking in or generating any build assets from the proto repo itself. Somebody just recently removed the gen-go directory (presumably an artifact from opencensus).

open-telemetry/opentelemetry-proto@6f6985e

There is a long thread where people are discussing code gen (mostly go specific but still applicable) and why it was removed from the repo above:
open-telemetry/opentelemetry-proto#79

So I think we are left with this git submodule or setting up another repo to build this NuGet package (which would use a submodule anyway). Frankly, I think that will be more overhead than this solution. Once populated, a git clean won't remove the submodule files. Developers would only have to update them via 'git submodule update' when they changed. I also think it is reasonable for people to have a rudimentary understanding of how to consume a git submodule during development.

@reyang If you still feel strongly about this I can attempt to set up another repo called opentelemetry-proto-dotnet for it.

@pcwiese
Copy link
Contributor Author

pcwiese commented Aug 6, 2020

Chatted with @reyang offline a bit. For now, we can proceed with our current approach where we simply make a copy of the proto files as necessary. If this process doesn't suffice for some reason, we can create a dedicated pipeline or another single purpose repo with a nightly build that pulls in the latest proto files and publishes them as a NuGet package when they change.

@pcwiese pcwiese closed this Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handling of OTLP proto sources
3 participants