-
Notifications
You must be signed in to change notification settings - Fork 879
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
detect service.version based on jar file name #10514
Conversation
String jarName = jarPath.getFileName().toString(); | ||
int dotIndex = jarName.lastIndexOf("."); | ||
return dotIndex == -1 ? jarName : jarName.substring(0, dotIndex); | ||
if (dotIndex == -1 || ANY_DIGIT.matcher(jarName.substring(dotIndex)).find()) { |
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.
A user could add a number to the application name. Could you please add a test for this case?
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.
A user could also potentially add a dot to the application name. For example: my-service2.3-1.0.0.jar.
It's odd but feasible. So, I am not sure about the version extraction from the JAR name.
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.
it actually works 😄 - see the tests
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.
It's a bit strange for me to deduce a version from the Spring Boot JAR file name. With the Spring Boot Maven/Gradle plugin, it's possible to override the default JAR file name. Let's imagine the user configures a JAR file name application-2.jar
. How could we know that the application name is application-2
, or application
with a 2
version number?
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.
yes, you can override the default name - but this detector is only a fallback in case the user didn't specify the name and version manually - or is using build-info.properties
(where the version is in a dedicated property).
So I'd say it's a good best guess.
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 would suggest to remove the Spring detector based on the JAR file name, and fallback as a last resort to the build-info.properties
file or the manifest file. See #10515 (comment).
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.
are you saying that unknown_service:java
would be better than a name based on the jar file?
@@ -121,10 +149,27 @@ private Path pathIfExists(String programArguments) { | |||
return fileExists.test(candidate) ? candidate : null; | |||
} | |||
|
|||
private static String getServiceName(Path jarPath) { | |||
private static NameAndVersion getServiceNameAndVersion(Path jarPath) { |
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.
Do we want to fail if the code of this method raises an exception?
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 don't understand - can you make an example?
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.
Perhaps after having solved #10514 (comment)? To be sure that this PR makes sense.
c6edb13
to
6346da7
Compare
SIG meeting outcome: we'll wait until someone asks for this feature. |
meeting decision: not worth the complexity |
Add
JarServiceVersionDetector
Note: This PR already has a bug - see #10540. Fixed now.