Skip to content
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

Various Java cleanups #114

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Various Java cleanups #114

merged 1 commit into from
Nov 18, 2024

Conversation

mimaison
Copy link
Contributor

@mimaison mimaison commented Nov 14, 2024

  • Removed usage of deprecated APIs
  • Minor refactorings and code simplifications

Signed-off-by: Mickael Maison <[email protected]>
@@ -197,7 +197,7 @@ public String toString() {
*/
public KafkaVersion latestRelease() {
// at least one release in the json schema is needed
if (this.logicalKafkaVersionEntities == null || this.logicalKafkaVersionEntities.size() < 1) {
if (this.logicalKafkaVersionEntities.isEmpty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field is final and initialized when declared so it should never be null.

* @param ready lambda predicate
*/
static long waitFor(String description, long pollIntervalMs, long timeoutMs, BooleanSupplier ready) {
static long waitFor(String description, Duration pollInterval, Duration timeout, BooleanSupplier ready) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is not public so I assume it's only used by this project and we can change its declaration.

@@ -72,7 +72,7 @@ void testStartContainerWithEmptyConfiguration(final String imageName) {
systemUnderTest.start();

assertThat(systemUnderTest.getBootstrapServers(), is("PLAINTEXT://"
+ systemUnderTest.getContainerIpAddress() + ":" + systemUnderTest.getMappedPort(9092)));
+ systemUnderTest.getHost() + ":" + systemUnderTest.getMappedPort(9092)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getContainerIpAddress() is deprecated, so switching to getHost() which is the preferred method now.

@@ -18,8 +18,8 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertTrue;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the junit 4 dependency brought via testcontainers. We should instead use org.junit.jupiter.api.Assertions which is junit 5 (the declared dependency in pom.xml).

Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, please update the description of this PR.

@see-quick see-quick added this to the 0.109.0 milestone Nov 15, 2024
@mimaison
Copy link
Contributor Author

Thanks, I updated the description.

Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming that everything works :) thanks

@see-quick see-quick merged commit 3091d96 into strimzi:main Nov 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants