7000 mpconfig version #8824
7000 mpconfig version #8824
Conversation
poikilotherm
added
Component: Code Infrastructure
| // which contains in the source a Maven property reference to ${project.version}. | ||
| // When packaging the app to deploy it, Maven will replace this, rendering it a static entry. | ||
| // NOTE: MicroProfile Config will cache the entry for us in internal maps. | ||
| String appVersion = JvmSettings.VERSION.lookup(); |
There was a problem hiding this comment.
As the version info is accessed very frequently (every page view), it's debatable if we should cache the app version ourselves as done before and not rely on MPCONFIG for that (the spec does not guarantee caching).
poikilotherm
force-pushed
the
7000-mpconfig-version
branch
from
79d6fe2
to
0213597
Compare
Instead of trying to read a built time file from Maven, use MicroProfile Config to retrieve the version and build number. The version is by default set via microprofile-config.properties (or overridden by an env var in a container). The build number is still read from either BuildNumber.properties or, if not present, from MicroProfile Config, defaulting to empty. This also avoids copying extra files into containers to retrieve the version string.
poikilotherm
force-pushed
the
7000-mpconfig-version
branch
from
0213597
to
5f925ed
Compare
There was a problem hiding this comment.
Looks fine to me. I raised a couple minor questions for @poikilotherm where he might want to make changes. I'm approving since I don't think either thing would require re-review if he does make changes.
| assertTrue(result.startsWith("100.100"), "'" + result + "' not starting with 100.100"); | ||
| assertTrue(result.contains("build")); | ||
|
|
||
| // Cannot test this here - there might be the bundle file present which is not under test control |
There was a problem hiding this comment.
A bundle entry would affect the real app the same way, right? So should this test be here to assure the default app is not shipping a bundle file that's going to override this setting? Or should the code prioritize the mpconfig value over any hardcoded bundle entry (that would allow the test to work, but is it also a better choice for the app - should you have to delete a bundle entry to be able to control build number? I'm asking and not recommending - you/others probably have a better sense of whether a bundle entry should have precedence or just be a default.)
There was a problem hiding this comment.
I gave this a bit of though today.
I don't really have an opinion on what should be preferred, because I do have the flexibility to have it configured with this PR merged.
Excluding the file via an <exclude> in the resources means it would not be picked up for testing deployments, too. But a unit test is kind of strange place to verify a file is not present under certain conditions.
We could also switch from using a bundle to the same trick used for the version - make it a Maven property, defaulting to empty, getting filtered in microprofile-config.properties at build time. Make it usable in any CI context or custom builds, not using hacks. Could also refer other vars, e.g. by the git-commit Maven plugin if people wish to do so.
Let me know what y'all think, given the fact that the Bundle file was an occasional problem before.
There was a problem hiding this comment.
@poikilotherm my first thought is that this shouldn't be at the top of the community backlog if there's still stuff to figure out! 😄
There was a problem hiding this comment.
😲
Why is that? It's the first time this code is getting a review and raised a question from a core member. I was fine with the code as is, it works as designed and tested. Maybe I'm getting the scope of "review" wrong?
There was a problem hiding this comment.
@poikilotherm ok, it helps to know that you're still fine with the PR, as-is. Thanks!
There was a problem hiding this comment.
When I run this test I get this as the result:
100.100 build pdurbin
This is because I have a file here:
$ cat src/main/java/BuildNumber.properties
build.number=pdurbin
The test passes just fine, which is good! As a developer I should be free to specify a custom build number.
So should this test be here to assure the default app is not shipping a bundle file that's going to override this setting?
No. In fact, we do ship a BuildNumber.properties in each war file. I'm not sure why we would assert that we don't do this but I'm probably misunderstanding what's being discussed above.
In short, I think we should get this into a sprint so core team members can focus on a proper review while it's in a sprint. Again, it sounds like @poikilotherm is ok with the PR as-is. Great.
|
added to sprint Dec 15, 2022 |
What this PR does / why we need it:
By simplifying the way to set the version and build number, the container setup is already simplified. There is no need to include some special Maven files anymore, as Maven takes care of providing the version number via the META-INF/properties file.
Relates to #7000
Which issue(s) this PR closes:
None.
Special notes for your reviewer:
It is a good and simple start to see how the pieces laid out in #8823 are coming together in a simple first usage.
Suggestions on how to test this:
Play around with the settings described in the little dev docs section and watch it work. Or take a look at the nice test example, also using the new shiny
@JvmSettingtest extension!Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Not really.
Is there a release notes update needed for this change?:
Maybe? Dunno. Please leave comment. IMHO this is very much dev-only.
Additional documentation:
Included.