-
Notifications
You must be signed in to change notification settings - Fork 852
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 engine_getClientVersionV1 #7512
add engine_getClientVersionV1 #7512
Conversation
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
private static final String CLIENT_VERSION = "TestClientVersion/0.1.0"; | ||
private static final String CLIENT_NODE_NAME = "TestClientVersion/0.1.0"; |
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.
Renaming this to better match its source in BesuInfo
…gain Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
|
||
static { | ||
if (VERSION == null) { | ||
COMMIT = null; |
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.
instead of NULL, would it be better to return empty String ""
?
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.
Also, is there a test that covers NULL commit? How would it be reported?
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.
Unfortunately, it's difficult to test the value of COMMIT because BesuInfo pulls data from the class' package which isn't typically set outside of a proper build.
As for null vs empty String, philosophically I'd rather it be null. The api doesn't really specify what should be supplied if a value isn't available.
Signed-off-by: Matilda Clerke <[email protected]>
COMMIT = null; | ||
} else { | ||
Pattern pattern = Pattern.compile("v?\\d*\\.\\d*\\.\\d*-\\w+-(?<commit>[0-9a-fA-F]{8})"); | ||
Matcher matcher = pattern.matcher(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.
Only the develop builds include the commit hash in the version at the moment, so this won't work for github release versions.
I tested locally using ./gradlew -Prelease.releaseVersion=99.9.0 -Pversion=99.9.0 assemble installDist
and the commit field in the response was null.
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 not sure how else to get the commit hash. The purpose of the field is to give a specific commit at which the application was run, and for github releases there seems to be an accompanying tag, so the version itself can be used to find the exact commit if desired.
Do you think it would be worthwhile finding some other way to get the commit field populated?
… CHANGELOG.md Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
…V1' into 6471-add-engine-getClientVersionV1
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.
LGTM
PR description
Add the engine_getClientVersionV1 to the engine RPC API
Fixed Issue(s)
#6471