-
Notifications
You must be signed in to change notification settings - Fork 404
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
Issue 4209: Bump versions of dependencies #4210
Conversation
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
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.
The PR CI Build is failing with compilation error.
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Codecov Report
@@ Coverage Diff @@
## master #4210 +/- ##
============================================
+ Coverage 81.67% 81.74% +0.07%
- Complexity 10103 10114 +11
============================================
Files 672 672
Lines 37815 37813 -2
Branches 3667 3667
============================================
+ Hits 30884 30910 +26
+ Misses 4845 4830 -15
+ Partials 2086 2073 -13
Continue to review full report at Codecov.
|
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.
|
||
// Same as ISO8601DateFormat but serializing milliseconds. | ||
@Override | ||
public StringBuffer format(Date date, StringBuffer toAppendTo, FieldPosition fieldPosition) { | ||
String value = ISO8601Utils.format(date, true); | ||
String value = com.fasterxml.jackson.databind.util.ISO8601Utils.format(date, true); |
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.
why are we updating generated file?
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.
Otherwise the build fails with a compilation error.
There is no functional change here, this just suppresses the warnings from importing a deprecated class. Which would normally be a warning, but our build is set to treat warnings as errors and fail the build. Originally I just changed that behaviour but based on feedback, it seemed better to not remove that safety net for the sake of preserving one generated file in it's unmodified state.
Once there is an update to the code generator to use non-deprecated code, we should be able to re-generate the file and leave it unmodified.
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.
Given the constraints in front of us, I'd say that we modify the file and create a separate issue explaining the problem and pointing to the jackson issue. The new issue should also say that once the jackson issue is resolved, we need to upgrade and regenerate the file.
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.
@tkaitchuck can you please open an issue for the same.
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.
Created #4224
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.
Blocking because we are modifying a generated file. Please revert that file and rest all looks good.
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Coverage is at 0% because changes are in the gradle file, and the service starter which is only used by integration tests. (Which don't count towards the coverage metric) |
Change log description
Purpose of the change
Fixes #4209
What the code does
Increases the versions of all of our current dependencies without breaking any code/tests.
How to verify it
We should run all tests against this branch.