[SERV-830] get version from manifest #53
Conversation
| } | ||
| } catch (final IOException details) { | ||
| // Either the app wasn't deployed as a JAR, or Vert.x Maven Plugin isn't creating the manifest file | ||
| LOGGER.warn(MessageCodes.AUTH_024, manifestPath, details.getMessage()); |
There was a problem hiding this comment.
Is the use of version in Hauth important enough to warrant an error?
There was a problem hiding this comment.
Per the README, HAUTH_VERSION is a required value, and Config.HAUTH_VERSION is referenced in AccessTokenHandler, AccessCookieHandler, AccessCookieServiceImpl, and SinaiAccessTokenHandler.
There was a problem hiding this comment.
So, should it be more than a warning? Seems, to me, that missing something that's required should be an error, perhaps also bubbling up a failed future?
There was a problem hiding this comment.
I was thinking that it would be nice (as an enhancement) for HTTP responses to include the Server header, which I'd see looking something like Hauth/x.y.z. Here's how prl-harvester deals with this (MDN says User-Agent and Server are usually in a similar format). However, simply the product name is also acceptable, so it's up to us on how strict we want to be with the value.
There was a problem hiding this comment.
Can the function passed to ConfigRetriever.setConfigurationProcessor (here setAppVersion) return a Future? And if so, does that require changing how setConfigurationProcessor is handled?
There was a problem hiding this comment.
Yes, setAppVersion could return a future, but you're right you'd have to handle that instead of just passing the result to setConfigurationProcessor since that takes a function that returns a JsonObject: setConfigurationProcessor(Function<JsonObject, JsonObject> processor);.
|
Also, regarding "Remove HAUTH_VERSION from pom" -- it doesn't look like the POM was updated in this PR(?) |
The value was never literally removed. I commented out the entries in the pom, then eventually realized they were needed for the tests (which don't run from the deployed JAR and so can't get the version from the manifest file) and uncommented them. |
|
HAUTH_VERSION should be removed from the run configuration of the Hauth container, since it's able to pull it from the JAR. But yes, if we want to keep the equality assertions as-is (comparing entire JsonObjects that have HAUTH_VERSION keys), it must still be explicitly provided to Failsafe (although a comment in the POM explaining "why" might be a good idea). |
|
Also, it should be removed from the list of env vars in the README. |
|
Closing this old pull request because we're deprecating all the Java work and won't add new features to them any more. We'll only be doing bug fixes. |
prl) to retrieve version from JAR manifestMainVerticleMessageBundle