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

Add support for io.etcd:jetcd-core:0.7.5 #170

Closed
wants to merge 1 commit into from

Conversation

linghengqian
Copy link
Contributor

@linghengqian linghengqian commented Jan 12, 2023

What does this PR do?

Checklist before merging

  • I have properly formatted metadata files (see CONTRIBUTING document)
  • I have added thorough tests. (see this)

@linghengqian
Copy link
Contributor Author

linghengqian commented Jan 17, 2023

  • One test had a too short timeout of 30 seconds. I'll deal with it in another Commit.

  • Update: Done.

@linghengqian
Copy link
Contributor Author

linghengqian commented Jan 21, 2023

  • I need to start to modify this PR tomorrow, because I am not near the work computer on vacation these days.

  • Something unexpected happened after fixing checkstyle's CI.

@linghengqian
Copy link
Contributor Author

Error: Could not find GraalVM dev build for Java 17. It may no longer be available, so please consider upgrading the Java version. If you think this is a mistake, please file an issue at: https://github.com/graalvm/setup-graalvm/issues.

@linghengqian
Copy link
Contributor Author

@dnestoro @vjovanov

@linghengqian
Copy link
Contributor Author

linghengqian commented Feb 4, 2023

  • I also temporarily turned on metadataRepository 0.2.5. If I don't turn it on, Netty prevents the native image from compiling, I don't know why.

import static org.mockito.Mockito.verify;

@SuppressWarnings({"SameParameterValue", "ResultOfMethodCallIgnored"})
// `@org.junit.jupiter.api.Timeout(value = 30)` can't be used in the nativeTest GraalVM CE 22.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this may be interesting for our JUnit testing support, do you by any chance remember why @Timeout didn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private Watch watchClient;
private ExecutorService executor = Executors.newFixedThreadPool(2);
private AtomicReference<StreamObserver<WatchResponse>> responseObserverRef;
@Mock
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you hit any issues with Mockito when writing these tests? I remember there were problems when using it with native-image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// `@org.junit.jupiter.api.Timeout(value = 30)` can't be used in the nativeTest GraalVM CE 22.3
public class AuthClientTest {
@RegisterExtension
public static final EtcdClusterExtension cluster = EtcdClusterExtension.builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this start a Docker image that contains an Etcd cluster in the background for the purposes of testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Yes, io.etcd:jetcd-test:0.7.5 is a friendly utility library for launching Etcd Docker Images.

import static org.assertj.core.api.Assertions.assertThat;

// `@org.junit.jupiter.api.Timeout(value = 30)` can't be used in the nativeTest GraalVM CE 22.3
@Disabled("https://github.com/etcd-io/jetcd/pull/1092")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan to enable these tests in the future? If not, I would remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I think it's perfectly fine to remove these tests, as it seems jetcd-core has never been able to fix this.
  • If a future version of jetcd-core fixes it, that's something another issue should be talking about, not something this PR should be talking about.

.withNodes(1)
.withSsl(true)
.withAdditionalArgs(Arrays.asList("--auth-token",
"jwt,pub-key=/etc/ssl/etcd/server.pem,priv-key=/etc/ssl/etcd/server-key.pem,sign-method=RS256,ttl=1s"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this refer to a path inside of the test container that EtcdClusterExtension starts?

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 path refers specifically to the internal path of the Docker Image. The files corresponding to Docker Volumes are in the folder src/test/resources/ssl/cert/. Those files are manually generated under Docker Image of Linux. I've noticed that GraalVM Native Build Tools finds these files when packaging even though they don't need to be specified in resource-config.json .

Copy link
Contributor Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

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

@linghengqian
Copy link
Contributor Author

  • Late reply as I am busy these months. I will refactor the contents of this PR as required by the master branch. Until the refactoring is complete, the PR will be in a draft state.

@linghengqian linghengqian marked this pull request as draft August 8, 2023 16:19
Copy link
Contributor Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

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

@linghengqian linghengqian deleted the jetcd-core branch January 17, 2024 15:36
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.

Add support for io.etcd:jetcd-core:0.7.5
2 participants