Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create OtelVersion class at build time. #5365
Create OtelVersion class at build time. #5365
Changes from 3 commits
800c143
b274538
804022f
4abd4ea
1905d57
bb81bb3
8e01e0d
3a09de5
e622d7f
840df35
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with this approach: will devs have to run a gradle build in order to be able to compile in IDEA? Or is IDEA smart enough to run what needs to be run and get this file into the generated source when the project is imported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm sure a gradle build step has to happen before
Resource
can compile. Isn't that the same with autovalue tho?Another option was to do string substitution on the actual source tree, but that leaves uncommitted changes. Open to alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is one idea of how to compromise: https://gist.github.com/breedx-splk/01278cc3eb94415353cac1f9cdce89c7
The idea is instead of referencing a static field constant, try to do the same with reflection, and if that fails, fall back to the previous behavior. I don't love it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this for
:sdk:common
, should we do it everywhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only wanted to do this in the one place where it's used. If there are other places that ever need to reference the class, then those modules can apply the plugin. I don't think we need it floating around every module. Feels bloaty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable. Here are some other places we read the version:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And those will still continue work, I didn't remove the properties behavior. Are you suggesting that we should tackle those all in this same PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have this pattern, it offers a pretty clean way of accessing the version. Adding a plugin to the gradle build is cleaner than copy / pasting the code to read from the resource.
Doesn't have to be in this PR, but if we like this pattern I think we should standardize on it.