Removed cartridge-java from dependencies #71

iDneprov · 2023-05-05T15:11:42Z

Update executeScript and executeCommand methods to execute code viva execInContainer.
Add executeScriptDecoded and executeCommandDecoded methods to return parsed yaml not String.
Remove cartridge-java client from TarantoolContainerClientHelper and tests.
Rewrite tests and add new cases to support new API.
Remove io.tarantool.cartridge-driver dependency.
Update org.yaml.snakeyaml to 2.0 version.

I haven't forgotten about:

Related issues:

Closes #69

iDneprov · 2023-05-19T14:57:22Z

I also need to think about a container builder for SSL | mTLS tests.

ArtDu

Just two small comments

ArtDu · 2023-05-19T15:34:33Z

CHANGELOG.md

@@ -1,5 +1,15 @@
 # Changelog

+## [0.6.0] - 2023-05-17
+BREAKING CHANGES


Not all of them are breaking changes. Look how we did it cartridge-java

ArtDu · 2023-05-19T15:35:36Z

pom.xml

@@ -49,7 +49,7 @@
        <connection>scm:git:[email protected]:tarantool/cartridge-java-testcontainers.git</connection>
        <developerConnection>scm:git:[email protected]:tarantool/cartridge-java-testcontainers.git</developerConnection>
        <url>http://github.com/tarantool/cartridge-java-testcontainers/tree/master</url>
-        <tag>v0.5.4</tag>
+        <tag>v0.6.0</tag>


Don't change it. Maven will change in release phase

akudiyar

LGTM overall, I have few nitpicks though

akudiyar · 2023-05-29T15:38:57Z

src/test/java/org/testcontainers/containers/CartridgeContainerTestUtils.java

@@ -1,12 +1,15 @@
 package org.testcontainers.containers;

-import java.util.Arrays;
+import org.jetbrains.annotations.NotNull;


Please do not use unnecessary imports for such third-party types

akudiyar · 2023-05-29T15:43:26Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

+        Container.ExecResult result = executeScript(scriptResourcePath, sslIsActive, keyFile, certFile);
+
+        if (result.getExitCode() != 0) {
+            throw new IllegalStateException(String.format("Executed script %s with exit code %d," +


I suggest making these command string templates constant variables

akudiyar · 2023-05-29T15:46:36Z

src/test/java/org/testcontainers/containers/CartridgeContainerTestUtils.java


-        List<?> result = container.executeCommand("return profile_get(...)", 1).get();
+        ArrayList result = container.executeCommandDecoded("return profile_get(1)");


The linter and compiler most likely show warnings for this place. ArrayList<?> or ArrayList<Object> are more approriate. ArrayList is an old form from Java 4 that is long prohibited but exists for compatibility

iDneprov · 2023-05-29T17:15:05Z

Now TarantoolCartridgeBootstrapFromLuaWithFixedPortsTestdoes not start testcontainer

akudiyar · 2023-05-30T23:22:36Z

src/main/java/org/testcontainers/containers/TarantoolCartridgeContainer.java

-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.CompletableFuture;
+import java.util.*;


Please do not use star imports (set up your IDE to not change this for, say, 100 import lines)

akudiyar · 2023-05-30T23:25:01Z

src/main/java/org/testcontainers/containers/TarantoolContainer.java

-                              TarantoolClientBuilder clientBuilder) {
-        super(TarantoolContainerImageHelper.getImage(tarantoolImageParams));
-        clientHelper = new TarantoolContainerClientHelper(this, clientBuilder);
+        clientHelper = new TarantoolContainerClientHelper(this, "/sdk/tarantoolctl");


This should be a constant at least, but better that should be either an env variable or the executable should be in the classpath so no explicit path is specified. DO not leave such hardcode, that will not work for all environments

akudiyar · 2023-05-30T23:25:58Z

src/main/java/org/testcontainers/containers/TarantoolContainer.java

@@ -39,6 +37,9 @@ public class TarantoolContainer extends GenericContainer<TarantoolContainer>
    private String password = API_PASSWORD;
    private String host = DEFAULT_HOST;
    private Integer port = DEFAULT_PORT;
+    private Boolean sslIsActive = false;
+    private String keyFile = "";


I think this should be a default path or name, shouldn't it?

akudiyar · 2023-05-30T23:27:10Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

-                .withRetryingByNumberOfAttempts(15,
-                        TarantoolRequestRetryPolicies.retryNetworkErrors(),
-                        b -> b.withDelay(100));
+        this.tarantoolctlPath = "tarantoolctl";


I suggest using an environment variable with default value here

akudiyar · 2023-05-30T23:28:32Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

+
+        String bashCommand;
+        if (keyFile != "" && certFile != "") {
+            bashCommand = String.format("echo \"print(require('yaml').encode(require('net.box').connect({ uri='%s:%d', " +


This should be a constant., not an inline string. Also, we may use more proper script formatting for easier reading with backslashes or plus signs.

The same for the subsequent lines

akudiyar · 2023-05-30T23:32:30Z

src/test/java/org/testcontainers/containers/CartridgeContainerTestUtils.java


-        List<?> result = container.executeCommand("return profile_get(...)", 1).get();
+        ArrayList<?> result = container.executeCommandDecoded("return profile_get(1)");


I don't get why we need a more specific type here. Do we use any ArrayList methods or properties? Is ArrayList always guaranteed as a result (as I see it is a generic parameter so it seems not)?

iDneprov · 2023-06-02T15:50:31Z

SSL and mTLS support will be added in #75

iDneprov · 2023-06-05T11:34:02Z

@ArtDu, @akudiyar, guys, this RP is ready for review.

iDneprov · 2023-06-07T09:39:45Z

@akudiyar, hi! We are planning to merge this PR this week. Will you have time to watch it again?

akudiyar

It seems that we are putting too many details to the changelog. These details can be read from the git history. Leaving internal changes that do not affect customers does not make sense, and it's better for readability to shorten several small changes related to the same feature to one line that describes the meaning of the changes.

CHANGELOG.md

akudiyar · 2023-06-08T21:24:57Z

pom.xml

+            <groupId>org.projectlombok</groupId>
+            <artifactId>lombok</artifactId>
+            <version>1.18.28</version>
+            <scope>provided</scope>


Please limit this import to the test scope

I use it in SslContext class, not in the test scope.
Why you do not recommend using lombok in other scopes?

Using it for a single class with 2 fields and in a library code needs additional justification. We've just removed the driver dependency (which is a good move), why are we adding to the library level a new dependency that is normally used on application level?

src/main/java/org/testcontainers/containers/TarantoolCartridgeContainer.java

akudiyar · 2023-06-08T21:26:25Z

src/main/java/org/testcontainers/containers/TarantoolCartridgeContainer.java

@@ -189,6 +191,22 @@ public TarantoolCartridgeContainer(String dockerFile, String buildImageName, Str
        this(buildImage(dockerFile, buildImageName, buildArgs), instancesFile, topologyConfigurationFile, buildArgs);
    }

+// todo add SSL and mTLS cartridge test


Let's not leave any commented code in the repo. We have git and branches for not-ready changes.

akudiyar · 2023-06-08T21:27:01Z

src/main/java/org/testcontainers/containers/TarantoolCartridgeContainer.java

@@ -251,6 +269,20 @@ private static ImageFromDockerfile buildImage(String dockerFile, String buildIma
                                                           "cartridge" : buildArgs.get("CARTRIDGE_SRC_DIR"));
    }

+// todo add SSL and mTLS cartridge test


Let's not leave any commented code in the repo. We have git and branches for not-ready changes.

src/main/java/org/testcontainers/containers/TarantoolContainer.java

akudiyar · 2023-06-09T08:57:51Z

A side note: commits named like "Fix someone's comments" or "Update filename" are not useful for the git history. They need either to be squashed or re-formulated in a way that the message describes what exact changes are made to the code (examples: "rename variable", "pull code literals to constants", "remove/add dependency XXX" etc.)

dkasimovskiy

Fix changelog, please.

CHANGELOG.md

iDneprov · 2023-06-12T12:09:58Z

A side note: commits named like "Fix someone's comments" or "Update filename" are not useful for the git history.

All commits will be squashed into one before the changes are merged into master. I try not to merge them during reviews to make it easier to keep track of requested changes.

If everything looks good for you, just approve changes, and I'll use "Squash and merge" button.

ArtDu · 2023-06-14T15:15:14Z

src/test/java/org/testcontainers/containers/Enterprise/TarantoolSslContainerTestEnterprise.java

@@ -0,0 +1,62 @@
+package org.testcontainers.containers.Enterprise;


Suggested change

package org.testcontainers.containers.Enterprise;

package org.testcontainers.containers.enterprise;

src/main/java/org/testcontainers/containers/TarantoolContainerOperations.java

src/main/java/org/testcontainers/containers/TarantoolCartridgeContainer.java

ArtDu · 2023-06-14T15:26:28Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

+        // No SSL
+        if (sslContext == null) {


I think this type of comment code style is more appropriate in this case

Suggested change

// No SSL

if (sslContext == null) {

if (sslContext == null) { // No SSL

ArtDu · 2023-06-14T15:27:01Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

+        // mTLS
+        } else if (sslContext.getKeyFile() != null && sslContext.getCertFile() != null) {


Suggested change

// mTLS

} else if (sslContext.getKeyFile() != null && sslContext.getCertFile() != null) {

} else if (sslContext.getKeyFile() != null && sslContext.getCertFile() != null) { // mTLS

ArtDu · 2023-06-14T15:27:23Z

src/main/java/org/testcontainers/containers/TarantoolContainerClientHelper.java

+        // SSL
+        } else {


Suggested change

// SSL

} else {

} else { // SSL

dkasimovskiy

Есть небольшие замечания.
В остальном, можно мержить.

dkasimovskiy · 2023-06-15T10:51:04Z

src/main/java/org/testcontainers/containers/TarantoolContainer.java

@@ -213,6 +167,20 @@ public TarantoolContainer withPassword(String password) {
        return this;
    }

+
+    /**
+     * Specify SSL as connection transport. And path to key and cert files inside your container for mTLS connection


Suggested change

* Specify SSL as connection transport. And path to key and cert files inside your container for mTLS connection

* Specify SSL as connection transport. Add path to key and cert files inside your container for mTLS connection

?

I meant, specify transport and path.

dkasimovskiy · 2023-06-15T10:57:47Z

...test/java/org/testcontainers/containers/enterprise/TarantoolMTlsContainerTestEnterprise.java

+        );
+        final Map<String, String> buildArgs = new HashMap<>();
+        buildArgs.put("DOWNLOAD_SDK_URI", System.getenv("DOWNLOAD_SDK_URI"));
+        buildArgs.put("SDK_VERSION", "tarantool-enterprise-sdk-nogc64-2.10.6-0-r557.linux.x86_64");


Я бы вынес версию SDK в константу или settings.

- [breaking change] Remove io.tarantool.cartridge-driver dependency - [breaking change] Update executeScript and executeCommand methods to execute code viva execInContainer (now it returns yaml string in Container.ExecResult not CompletableFuture) - Add executeScriptDecoded and executeCommandDecoded methods to return parsed yaml not string. - Add SslContext class - Add withSslContext method to TarantoolContainer and TarantoolCartridgeContainer. - Update org.yaml.snakeyaml to 2.0 version. Closes #69

iDneprov force-pushed the gh-69 branch from ecbbaf5 to a9f4314 Compare May 5, 2023 15:12

iDneprov force-pushed the gh-69 branch from a9f4314 to fa8e02e Compare May 17, 2023 09:38

iDneprov changed the title ~~Gh 69~~ Removed cartridge-java from dependencies May 17, 2023

iDneprov closed this May 17, 2023

iDneprov force-pushed the gh-69 branch from fa8e02e to dda7e9c Compare May 17, 2023 13:22

iDneprov reopened this May 17, 2023

iDneprov requested a review from ArtDu May 17, 2023 17:11

dkasimovskiy requested a review from akudiyar May 18, 2023 09:21

ArtDu requested changes May 19, 2023

View reviewed changes

iDneprov force-pushed the gh-69 branch from 9cead42 to 76b8cc8 Compare May 19, 2023 17:30

iDneprov force-pushed the gh-69 branch 4 times, most recently from 71ec0bc to cbdf777 Compare May 29, 2023 15:42

akudiyar suggested changes May 29, 2023

View reviewed changes

iDneprov force-pushed the gh-69 branch 6 times, most recently from f7ebd35 to a4b81d9 Compare May 30, 2023 17:04

akudiyar suggested changes May 30, 2023

View reviewed changes

iDneprov force-pushed the gh-69 branch 2 times, most recently from 6e052f8 to 67b4481 Compare June 2, 2023 15:48

iDneprov requested review from akudiyar and ArtDu June 2, 2023 15:49

iDneprov force-pushed the gh-69 branch from c2afbc7 to 362b438 Compare June 2, 2023 16:23

iDneprov force-pushed the gh-69 branch 2 times, most recently from 63b2277 to 13f838b Compare June 5, 2023 11:27

iDneprov force-pushed the gh-69 branch from 13f838b to 36db7a8 Compare June 5, 2023 13:59

akudiyar suggested changes Jun 8, 2023

View reviewed changes

iDneprov force-pushed the gh-69 branch from 2dd904f to 5621fae Compare June 9, 2023 10:08

iDneprov requested a review from akudiyar June 9, 2023 10:40

dkasimovskiy reviewed Jun 9, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

iDneprov requested a review from dkasimovskiy June 9, 2023 15:03

dkasimovskiy previously approved these changes Jun 9, 2023

View reviewed changes

ArtDu reviewed Jun 14, 2023

View reviewed changes

iDneprov dismissed dkasimovskiy’s stale review via dbeb003 June 14, 2023 15:43

iDneprov force-pushed the gh-69 branch from 7839126 to 8e83549 Compare June 14, 2023 15:46

iDneprov requested a review from ArtDu June 14, 2023 15:46

iDneprov force-pushed the gh-69 branch from 8e83549 to 4e9d764 Compare June 14, 2023 15:47

ArtDu previously approved these changes Jun 14, 2023

View reviewed changes

dkasimovskiy reviewed Jun 15, 2023

View reviewed changes

iDneprov dismissed ArtDu’s stale review via 0bfe33e June 15, 2023 11:45

iDneprov force-pushed the gh-69 branch 5 times, most recently from 0947346 to 7e6d389 Compare June 16, 2023 10:21

iDneprov force-pushed the gh-69 branch from 7e6d389 to 0701bf9 Compare June 16, 2023 10:28

ArtDu merged commit 211db68 into master Jun 16, 2023

ArtDu deleted the gh-69 branch June 16, 2023 10:37


		List<?> result = container.executeCommand("return profile_get(...)", 1).get();
		ArrayList result = container.executeCommandDecoded("return profile_get(1)");


		List<?> result = container.executeCommand("return profile_get(...)", 1).get();
		ArrayList<?> result = container.executeCommandDecoded("return profile_get(1)");

		@@ -0,0 +1,62 @@
		package org.testcontainers.containers.Enterprise;

	// No SSL
	if (sslContext == null) {
	if (sslContext == null) { // No SSL

		// mTLS
		} else if (sslContext.getKeyFile() != null && sslContext.getCertFile() != null) {

	* Specify SSL as connection transport. And path to key and cert files inside your container for mTLS connection
	* Specify SSL as connection transport. Add path to key and cert files inside your container for mTLS connection

Removed cartridge-java from dependencies #71

Removed cartridge-java from dependencies #71

Conversation

iDneprov commented May 5, 2023 • edited Loading

iDneprov commented May 19, 2023

ArtDu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akudiyar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iDneprov commented May 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iDneprov commented Jun 2, 2023

iDneprov commented Jun 5, 2023

iDneprov commented Jun 7, 2023

akudiyar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akudiyar commented Jun 9, 2023 • edited Loading

dkasimovskiy left a comment

Choose a reason for hiding this comment

iDneprov commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkasimovskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iDneprov commented May 5, 2023 •

edited

Loading

akudiyar left a comment •

edited

Loading

akudiyar commented Jun 9, 2023 •

edited

Loading