-
Notifications
You must be signed in to change notification settings - Fork 207
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
report sdk version in user-agent #810
Conversation
public final class ActorUtils { | ||
|
||
private static String version = 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.
Rename to sdkVersion
InputStream in = new ByteArrayInputStream(new byte[0]); | ||
try { | ||
Properties properties = new Properties(); | ||
in = ActorUtils.class.getResourceAsStream("/app.properties"); |
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.
Make it a try with resources block so that input stream is properly closed even on 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.
Oh cool, wasn't aware of this feature.
InputStream in = new ByteArrayInputStream(new byte[0]); | ||
try { | ||
Properties properties = new Properties(); | ||
in = Version.class.getResourceAsStream("/app.properties"); |
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.
Same as above
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.
Change app.properties to sdk_version.properties?
@addjuarez can you check why the build seems to be failing? |
public final class ActorUtils { | ||
|
||
private static String sdkVersion = 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.
Actor sdk import sdk module.
So can't we just use Version.getSdkVersion() command from there?
* | ||
* @return String version of sdk. | ||
*/ | ||
public static String getVersion() { |
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.
public static String getVersion() { | |
public static String getSdkVersion() { |
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.
@addjuarez just one small change. Thanks
@@ -0,0 +1 @@ | |||
version=${project.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.
version=${project.version} | |
sdk_version=${project.version} |
try (InputStream input = Version.class.getResourceAsStream("/sdk_version.properties");) { | ||
Properties properties = new Properties(); | ||
properties.load(input); | ||
sdkVersion = "dapr-sdk-java/v" + properties.getProperty("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.
Will exception be thrown if some other property is there and not version? Won't it return a null instead?
@addjuarez build passes. Please fix dco |
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
Signed-off-by: addjuarez <addiajuarez@gmail.com>
29716b2
to
479c86f
Compare
Codecov Report
@@ Coverage Diff @@
## master #810 +/- ##
============================================
+ Coverage 77.56% 77.62% +0.06%
- Complexity 1157 1161 +4
============================================
Files 104 105 +1
Lines 3628 3647 +19
Branches 419 419
============================================
+ Hits 2814 2831 +17
- Misses 600 603 +3
+ Partials 214 213 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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
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.
Please, follow up with an integration test that validates the user agent.
Signed-off-by: addjuarez addiajuarez@gmail.com
Description
Report the sdk version inside user-agent header.
HTTP: useragent="dapr-sdk-java/v1.7.1"
gRPC: useragent="dapr-sdk-java/v1.8.0-SNAPSHOT grpc-java-netty/1.42.1"
Please explain the changes you've made
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: