-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Consul module #4683
Add Consul module #4683
Conversation
Hi @julb, thanks a lot for contributing this module. Looking at the logic around However, we are currently lacking the capacity to review and merge PRs for new modules, so I can't promise when we will be able to get to it. This does not mean, that we don't appreciate your contribution. We are currently in the process to rework our current approach around modules, to make it easier for the community to contribute them. In case you are interested in getting it out to users as early as possible, you can always consider publishing it in your own repo (this is in no way an indication, that we don't want to add it to our repo). |
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.
Hi @julb, sorry for the late reply and thanks for your contribution! The code looks good but I've added a few comments.
Are you willing to update the PR?
modules/consul/src/test/java/org/testcontainers/consul/ConsulClientTest.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
Hey @eddumelendez |
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.
just a couple of comments and LGTM. Once again, thanks!
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
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.
few more comments from @kiview and me :)
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
@eddumelendez @kiview all remarks addressed! Thanks a lot. |
modules/consul/src/test/java/org/testcontainers/consul/ConsulContainerTest.java
Show resolved
Hide resolved
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
modules/consul/src/main/java/org/testcontainers/consul/ConsulContainer.java
Outdated
Show resolved
Hide resolved
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.
Thanks a lot for the PR @julb, LGTM 👍
There is one open question regarding the package name, but this is something we as maintainer should align on first 🙂
@@ -0,0 +1,103 @@ | |||
package org.testcontainers.consul; |
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.
We normally use the org.testcontainers.containers
package for Testcontainers container implementation, but AFAIK this makes Testcontainers suffer from the Split-Package problem when using the modulepath (as described here: https://stackoverflow.com/questions/53245628/jdk9-automatic-modules-and-split-packages-dependencies).
We also have other modules, that follow a package convention of org.testcontainers.{module-name}
, the same as here. So I am inclined to say, we should leave it as provided in the PR.
WDYT @bsideup?
modules/consul/src/test/java/org/testcontainers/consul/ConsulContainerTest.java
Show resolved
Hide resolved
Although we have some AUTHORS files in different modules, this is generally not what we do and we prefer using the Git history to identify authors.
@julb can you please add consul to
Index: .github/ISSUE_TEMPLATE/bug_report.yaml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/ISSUE_TEMPLATE/bug_report.yaml b/.github/ISSUE_TEMPLATE/bug_report.yaml
--- a/.github/ISSUE_TEMPLATE/bug_report.yaml (revision c9fc9afff3c13bdf7c6f7762ab8815ed91550399)
+++ b/.github/ISSUE_TEMPLATE/bug_report.yaml (revision 95124487415f8ffad3a06df54d6081f4dd6e269d)
@@ -17,6 +17,7 @@
- Azure
- Cassandra
- Clickhouse
+ - Consul
- Couchbase
- DB2
- Dynalite
Index: .github/ISSUE_TEMPLATE/enhancement.yaml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/ISSUE_TEMPLATE/enhancement.yaml b/.github/ISSUE_TEMPLATE/enhancement.yaml
--- a/.github/ISSUE_TEMPLATE/enhancement.yaml (revision c9fc9afff3c13bdf7c6f7762ab8815ed91550399)
+++ b/.github/ISSUE_TEMPLATE/enhancement.yaml (revision 95124487415f8ffad3a06df54d6081f4dd6e269d)
@@ -17,6 +17,7 @@
- Azure
- Cassandra
- Clickhouse
+ - Consul
- Couchbase
- DB2
- Dynalite
Index: .github/ISSUE_TEMPLATE/feature.yaml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/ISSUE_TEMPLATE/feature.yaml b/.github/ISSUE_TEMPLATE/feature.yaml
--- a/.github/ISSUE_TEMPLATE/feature.yaml (revision c9fc9afff3c13bdf7c6f7762ab8815ed91550399)
+++ b/.github/ISSUE_TEMPLATE/feature.yaml (revision 95124487415f8ffad3a06df54d6081f4dd6e269d)
@@ -17,6 +17,7 @@
- Azure
- Cassandra
- Clickhouse
+ - Consul
- Couchbase
- DB2
- Dynalite
Index: .github/dependabot.yml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
--- a/.github/dependabot.yml (revision c9fc9afff3c13bdf7c6f7762ab8815ed91550399)
+++ b/.github/dependabot.yml (revision 95124487415f8ffad3a06df54d6081f4dd6e269d)
@@ -43,6 +43,11 @@
interval: "monthly"
open-pull-requests-limit: 10
- package-ecosystem: "gradle"
+ directory: "/modules/consul"
+ schedule:
+ interval: "monthly"
+ open-pull-requests-limit: 10
+ - package-ecosystem: "gradle"
directory: "/modules/couchbase"
schedule:
interval: "monthly"
Index: .github/labeler.yml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/labeler.yml b/.github/labeler.yml
--- a/.github/labeler.yml (revision c9fc9afff3c13bdf7c6f7762ab8815ed91550399)
+++ b/.github/labeler.yml (revision 95124487415f8ffad3a06df54d6081f4dd6e269d)
@@ -15,6 +15,8 @@
- modules/clickhouse/*
"modules/cockroachdb":
- modules/cockroachdb/*
+"modules/consul":
+ - modules/consul/*
"modules/couchbase":
- modules/couchbase/*
"modules/db2":
|
@eddumelendez @kiview done! |
Thanks @julb, we will merge once we internally decided on how to proceed with package names for container modules 🙂 |
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.
one last thing @julb ! We are not able to push to your branch so here we are again asking you for some changes 😬 about moving to assertj
Thank you so much!
// Write operation | ||
properties.forEach((key, value) -> { | ||
Response<Boolean> writeResponse = consulClient.setKVValue(key, value); | ||
Assertions.assertThat(writeResponse.getValue()).isTrue(); |
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.
import static
public void readFirstPropertyPathWithCli() throws IOException, InterruptedException { | ||
GenericContainer.ExecResult result = consulContainer.execInContainer("consul", "kv", "get", "config/testing1"); | ||
final String output = result.getStdout().replaceAll("\\r?\\n", ""); | ||
MatcherAssert.assertThat(output, CoreMatchers.containsString("value123")); |
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.
move to assertThat from assertj, please
// Read operation | ||
properties.forEach((key, value) -> { | ||
Response<GetValue> readResponse = consulClient.getKVValue(key); | ||
Assertions.assertThat(readResponse.getValue().getDecodedValue()).isEqualTo(value); |
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.
import static
RestAssured | ||
.given() | ||
.when() | ||
.get("http://" + getHostAndPort() + "/v1/kv/config/testing1") | ||
.then() | ||
.assertThat() | ||
.body( | ||
"[0].Value", | ||
CoreMatchers.equalTo(Base64.getEncoder().encodeToString("value123".getBytes(StandardCharsets.UTF_8))) | ||
); |
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.
can we do something like
RestAssured | |
.given() | |
.when() | |
.get("http://" + getHostAndPort() + "/v1/kv/config/testing1") | |
.then() | |
.assertThat() | |
.body( | |
"[0].Value", | |
CoreMatchers.equalTo(Base64.getEncoder().encodeToString("value123".getBytes(StandardCharsets.UTF_8))) | |
); | |
io.restassured.response.Response response = RestAssured | |
.given() | |
.when() | |
.get("http://" + getHostAndPort() + "/v1/kv/config/testing1") | |
.andReturn(); | |
assertThat(response.body().jsonPath().getString("[0].Value")) | |
.isEqualTo(Base64.getEncoder().encodeToString("value123".getBytes(StandardCharsets.UTF_8))); |
Thanks @julb ! PR has been merged and now we have a consul module! |
OMG! |
No description provided.