Generate protos #4
Conversation
| [submodule "opamp-spec"] | ||
| path = opamp-spec | ||
| url = [email protected]:open-telemetry/opamp-spec.git | ||
| branch = v0.6.0 |
There was a problem hiding this comment.
Not familiar with .gitmodules files, but the info in here points to the wrong repository. We don't have these files in opentelemetry-java or opentelemetry-java-instrumentation. Care to summarize their purpose?
There was a problem hiding this comment.
the opamp-spec repo is used to get the proto definitions, as suggested in the issue. Here I set up a submodule and pinned it to the latest release.
settings.gradle.kts
Outdated
|
|
||
| pluginManagement { | ||
| plugins { | ||
| id("com.google.protobuf") version "0.8.17" |
There was a problem hiding this comment.
Current latest version of this plugin is 0.8.19.
| sourceSets { | ||
| main { | ||
| proto { | ||
| srcDir("../opamp-spec/proto") |
There was a problem hiding this comment.
What's the advantage of doing the submodule thing referencing the opamp-spec repo versus defining a task to download the protos from the opamp-spec into the build dir?
There was a problem hiding this comment.
this is a good question. This is what is suggested in the repo, but I can change it do behave like opentelemetry-java-protos. Perhaps @tigrannajaryan has an opinion here.
There was a problem hiding this comment.
I like git submodule, unless there are any other advantages w.r.t. task to download the protos.
There was a problem hiding this comment.
this is a good question. This is what is suggested in the repo, but I can change it do behave like opentelemetry-java-protos. Perhaps @tigrannajaryan has an opinion here.
@jlegoff I used the submodule in opamp-go since it was the simplest way I knew to bring it as a dependency pinned to a specific commit. If using a gradle download task has any advantages feel free to do that instead. What do you gain by doing that?
There was a problem hiding this comment.
Honestly, I don't know what we gain by using a download tasks instead of a submodule. Seems like a matter of taste. @jack-berg do you have a strong opinion against using submodules?
There was a problem hiding this comment.
Nope, no strong opinion. Agree it feels like a matter of taste.
proto/build.gradle.kts
Outdated
| repositories { | ||
| mavenCentral() | ||
| mavenLocal() | ||
| } |
There was a problem hiding this comment.
These should come from otel.java-conventions.
There was a problem hiding this comment.
thanks, I removed this section
|
Hey folks, great to see progress on this but I want to make sure we set the expectations right. Unless we find the second long-term maintainer we will not be able to move forward with this repo. It is not healthy to have a repo with only one long-term maintainer. @jack-berg since you are reviewing this are you interested in being the second maintainer? |
| sourceSets { | ||
| main { | ||
| proto { | ||
| srcDir("../opamp-spec/proto") |
There was a problem hiding this comment.
I like git submodule, unless there are any other advantages w.r.t. task to download the protos.
|
Hey @tigrannajaryan - I'm happy to act in an approver capacity for this repository, but not able to commit to be a maintainer. If we need a second maintainer (which I think is a very reasonable requirement), someone else will have to volunteer. |
|
Thanks @jack-berg, your reviews/approvals are appreciated. We now also have @AvadheshKaria who I believe will work on becoming the long term maintainer of this repo. |
|
@jlegoff we now have approvals. I can merge this if you don't plan any other changes to this PR. |
|
@tigrannajaryan great, let's merge it and I'll work on the next issue later this week |
This is a first attempt at setting up the build files and generating the protos.
Since the generated protos are part of the repo, the generation itself is not part of the build pipeline and is set up as an standalone project in the
generateProtosdirectory. Regenerating them can be done manually as explained in the README.While the issue mentions a makefile for proto generation, I took inspiration from opentelemetry-proto-java and used a gradle script instead. I can change that if needed.
Note also that the generated code is not published as a library. I suggest doing this in a separate PR.
Resolves #1