-
Notifications
You must be signed in to change notification settings - Fork 69
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
Updates dependency to recent versions #1584
Conversation
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.
Reviewable status: 0 of 20 files reviewed, 2 unresolved discussions
.github/workflows/agent_test.yml
line 29 at r1 (raw file):
- 'ibmcom/ibmjava:8-sdk' - 'eclipse-temurin:8' - 'eclipse-temurin:11'
I included the following information in the corresponding commit comment. That ensures that reasons for this decision are not completely lost in a distant future.
Why did I change those images?
- The images of sgrio are at least two years old.
See https://hub.docker.com/r/sgrio/java - Images from openjdk are deprecated.
See https://hub.docker.com/_/openjdk
eclipse-temurin are the official openjdk builds. Since they are based on
ubuntu (as far as the comitter understands it) there is no reason for
ubuntu specific images in the build matrix anymore.
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/ExecutorContextPropagationTest.java
line 65 at r1 (raw file):
taskFuture.get(); assertThat(refTags.get()).toIterable().hasSize(1)
That change was necessary to be able to use a current version of assertj-core
.
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/special/ScheduledExecutorContextPropagationSensorTest.java
line 31 at r1 (raw file):
@BeforeEach private void beforeEach() {
That change was necessary to be able to use current versions of JUnit.
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.
Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion
gradle.properties
line 59 at r1 (raw file):
piccoloVersion=1.0.3 # TODO Should we simply update that version? Was 3.24.1-GA
At least all tests are running. So I do not see any reason to not update to that newer 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.
Reviewed 19 of 20 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @quandor)
gradle.properties
line 59 at r1 (raw file):
Previously, quandor (Jochen Just) wrote…
At least all tests are running. So I do not see any reason to not update to that newer version.
Yeah, I think we can upgrade.
However, there is a typo in javassist
, i.e., the second s is missing (it should be javassist
gradle.properties
line 75 at r2 (raw file):
# Can we get safely newer? 3.15.7 protobufJavaUtilVersion=3.21.12
I have no idea if an upgrade would smoothly work. What do you think? Have you tried it out?
inspectit-ocelot-core/build.gradle
line 49 at r2 (raw file):
implementation( platform("com.fasterxml.jackson:jackson-bom:${jacksonBomVersion}"),
how is jackson now managed?
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/metrics/jmx/JmxMetricsRecorderTest.java
line 123 at r2 (raw file):
TagContext tagContext = tagContextBuilder.build(); assertThat(InternalUtils.getTags(tagContext)).toIterable().isEmpty();
the calls to toIterable()
are all needed due to an upgrade of assertJ
, right?
Code quote:
InternalUt
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @heiko-holz)
gradle.properties
line 59 at r1 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
Yeah, I think we can upgrade.
However, there is a typo in
javassist
, i.e., the second s is missing (it should be javassist
Deleted the TODO and changed to javassist.
gradle.properties
line 75 at r2 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
I have no idea if an upgrade would smoothly work. What do you think? Have you tried it out?
I just forgot to remove that comment. Since it is dependency we use for tests only and all tests are running, there is no real risk.
inspectit-ocelot-core/build.gradle
line 49 at r2 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
how is jackson now managed?
Jackson is managed via the spring boot bom and the spring boot plugin.
inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/metrics/jmx/JmxMetricsRecorderTest.java
line 123 at r2 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
the calls to
toIterable()
are all needed due to an upgrade ofassertJ
, right?
Yes, that is correct.
During the update process we did two things: * Switched to Spring-Boot-Plugin for dependency management. This allows easier and more robust spring updates in future. * All other versions are in the file gradle.properties.
Why did we change those images? * The images of sgrio are at least two years old. See https://hub.docker.com/r/sgrio/java * Images from openjdk are deprecated. See https://hub.docker.com/_/openjdk eclipse-temurin are the official openjdk builds. Since they are based on ubuntu (as far as the comitter understands it) there is no reason for ubuntu specific images in the build matrix anymore.
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.
Reviewed 19 of 20 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @heiko-holz)
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @quandor)
Most of the dependencies are now managed via spring-boot dependency management. That means an upgrade of spring-boot version updates automatically dependencies managed by spring-boot and guarantees compatibility.
All other versions are managed in a single file (gradle.properties). Usually those versions are the most current ones. If that is not the case a comment should explain why not.
This change is