Introduce application containers #7162

KyleAure · 2023-06-07T15:06:50Z

This is a draft PR to further explore the development of application platform containers.
Related Issue #7098

eddumelendez

Thanks for the quick draft, @KyleAure ! I'm really excited about this. I've left some comments and suggestions.

modules/liberty/src/main/java/org/testcontainers/containers/LibertyContainer.java

modules/application-platform/build.gradle

...s/application-platform/src/main/java/org/testcontainers/containers/ApplicationContainer.java

eddumelendez

Thanks, @KyleAure ! I have left some comments. Please, also run ./gradlew spotlessApply and let's take into account the Java version baseline for Testcontainers is 8.

eddumelendez · 2023-06-15T13:31:00Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+     * @param archives - An archive created using shrinkwrap and test runtime
+     * @return self
+     */
+    public ApplicationServerContainer withArchvies(@NonNull Archive<?>... archives) {


Suggested change

public ApplicationServerContainer withArchvies(@NonNull Archive<?>... archives) {

public ApplicationServerContainer withArchives(@NonNull Archive<?>... archives) {

Also, we would like to be more open about how the archive is provided. WDYT about receiving File... archives instead?

AFAIK we can do something like this.

WebArchive archive = ShrinkWrap.create(WebArchive.class,"jakarta-tc.war") .addClass(App.class) .addPackages(true, "dev.wittek.tc.jakarta.book", "dev.wittek.tc.jakarta.dice", "dev.wittek.tc.jakarta.time", "dev.wittek.tc.jakarta.turing") .addAsResource("META-INF/persistence.xml") .addAsResource("META-INF/init.sql") .addAsResource("turing.csv") .addAsLibraries(deps); archive.as(ZipExporter.class).exportTo(Paths.get("target/jakarta-tc.war").toFile(), true);

This way we can support those using ShrinkWrap and those who doesn't

Fixed spelling error.

I think having native ShrinkWrap support would really help with ease of use.
If users are looking to replace Arquillian with TestContainers then those users will never have had to export their archives to the host system since Arquillian takes care of that behind the scenes.

Without this kind of support I'd say it isn't even worth creating the ApplicationContainer and users could just use a GenericContainer. ShrinkWrap is the value add here.

This way we can support those using ShrinkWrap and those who do not

This is achievable with the pre-existing method:

public ApplicationServerContainer withArchives(@NonNull MountableFile... archives)

I built some examples for JakartaOne and ShrinkWrap play nice with Testcontainers by using Transferable.of(byte[]).

https://github.com/eddumelendez/jakartaone-livestream-2023/blob/7bdccd7f7ac66bbb5903623f97dbff758b783c10/src/test/java/org/example/jakartaeelivestream2023/BaseTest.java#L13-L25

https://github.com/eddumelendez/jakartaone-livestream-2023/blob/7bdccd7f7ac66bbb5903623f97dbff758b783c10/src/test/java/org/example/jakartaeelivestream2023/PayaraMicroTest.java#L45-L46

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

.github/ISSUE_TEMPLATE/bug_report.yaml

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

eddumelendez · 2023-06-15T16:18:10Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+     * @param httpPort - The HTTP port used by the Application platform
+     * @return self
+     */
+    public ApplicationServerContainer withHttpPort(int httpPort) {


Can you please elaborate more on the use cases for this?

This is the port a user would use to connect to their application.

Similar to the database containers we want to be able to construct a connection URL for testing getApplicationURL().

It is common for application containers to be configured with a different port than the default.

Example:

private static ApplicationServerContainer liberty = new LibertyServerContainer(libertyImage) .withArchives(createDeployment()) .withAppContextRoot("test/app/service/") .withHttpPort(80); @Test public void testURL() { String actual = liberty.getApplicationURL(); String expected = "http://localhost:35286/test/app/service" //where 35286 maps to port 80 assertThat(actual).isEqualTo(expected); }

You could argue that we could just rely on firstMappedPort but for future iterations, I could see us adding in withHttpsPort and getSecureApplicationURL() which wouldn't be able to relay on that.

It is common for application containers to be configured with a different port than the default.

even for testing? Just curious. I mean, it can be done but I was not expecting it in testing, at least not very often.

Yes, if I am an application developer who wants to do integration testing I will have my test application server configured to the same specification as my production application server. Most applications do not run on the default port 9080.

I suggest to move the method body to the configure method. The logic about replacePort, appendPort seems a little bit odd here. I think we don't need to overwrite setExposedPorts neither.

Tests in ApplicationServerContainerTest should call configure before getExposedPorts() in order to get the proper container configuration. Also, I'm not sure order is required for ports. LMK if I'm missing something

Please, also add tests when readinessPort is set.

modules/liberty/src/main/java/org/testcontainers/containers/LibertyServerContainer.java

KyleAure · 2023-06-19T15:28:33Z

@eddumelendez Responded to your review comments and updated where necessary.

As for running the spotless gradle task, I keep getting the following error:

Could not determine the dependencies of task ':application-server-commons:spotlessCheck'.
> Could not create task ':application-server-commons:spotlessJavaCheck'.
   > Could not create task ':application-server-commons:spotlessJava'.
      > Running npm command 'npm install --no-audit --no-package-lock --no-fund --prefer-offline' failed with exit code: 1

I've tried uninstalling and reinstalling node multiple times and I honestly cannot get this gradle task working.
Honestly, I don't like that this task has a dependency on node/npm seems out of place in a java project.

** got this working on another system and pushed the spotless changes **

modules/liberty/src/main/java/org/testcontainers/containers/LibertyServerContainer.java

eddumelendez · 2023-07-26T21:51:43Z

docs/modules/liberty.md

@@ -0,0 +1,56 @@
+# Liberty Containers


In order to display the docs we should add a section Application Servers just like we have it for databases

testcontainers-java/mkdocs.yml

Lines 48 to 49 in 8465795

- Modules:

- Databases:

eddumelendez · 2023-07-26T21:56:46Z

modules/liberty/src/test/java/org/testcontainers/liberty/app/TestResource.java

+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.core.MediaType;


Tests failed because of the Java version. Currently, Testcontainers uses Java 8 but we can perform tests with other version declaring the following in the build.gradle

testcontainers-java/modules/oracle-xe/build.gradle

Lines 21 to 32 in 8465795

test {

javaLauncher = javaToolchains.launcherFor {

languageVersion = JavaLanguageVersion.of(11)

}

}

compileTestJava {

javaCompiler = javaToolchains.compilerFor {

languageVersion = JavaLanguageVersion.of(11)

}

options.release.set(11)

}

eddumelendez

@KyleAure few more comments

eddumelendez · 2023-07-31T22:08:51Z

modules/liberty/src/main/java/org/testcontainers/liberty/LibertyServerContainer.java

+    // Container defaults
+    static final int DEFAULT_HTTP_PORT = 9080;
+
+    static final int DEFAULT_HTTPS_PORT = 9443;


unused for now. So, it can be removed.

eddumelendez · 2023-07-31T22:09:14Z

modules/liberty/src/test/java/org/testcontainers/liberty/app/TestService.java

+
+    @GET
+    public String getAll() {
+        System.out.println("Calling getAll");


I think we can remove the println calls

eddumelendez · 2023-07-31T22:13:44Z

modules/liberty/src/test/java/org/testcontainers/liberty/LibertyContainerTest.java

+        //This is unintuitive and should have a better solution.
+
+        // configureLiberty {
+        liberty.withEnv("DB_URL", mockDatabase.getNetworkAliases().get(0) + ":1080");


use the network alias instead to be explicit. Otherwise, the network alias mockDatabase declared in MockServerContainer is not being used.

Suggested change

liberty.withEnv("DB_URL", mockDatabase.getNetworkAliases().get(0) + ":1080");

liberty.withEnv("DB_URL", "mockDatabase:1080");

eddumelendez · 2023-07-31T22:18:36Z

modules/liberty/src/test/java/org/testcontainers/liberty/LibertyContainerTest.java

+
+        //Note cannot use mockDatabase.getEndpoint() since it will return http://localhost:56254 when instead we need http://mockDatabase:1080
+        //This is unintuitive and should have a better solution.


When using container networks, services can call each other through the network alias and the direct port. That's how it is done in docker-compose. I agree that in order to know the ports right now can be challenging but I think javadocs around it can help. That's something to tackle around all Testcontainers modules.

Suggested change

//Note cannot use mockDatabase.getEndpoint() since it will return http://localhost:56254 when instead we need http://mockDatabase:1080

//This is unintuitive and should have a better solution.

eddumelendez · 2023-07-31T22:21:58Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+    public ApplicationServerContainer(@NonNull final Future<String> image) {
+        super(image);
+    }


This is not being used. It can be removed.

eddumelendez · 2023-07-31T23:25:18Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+    private Duration httpWaitTimeout = Duration.ofSeconds(60);
+
+    // Expected path for Microprofile platforms to query for readiness
+    static final String MP_HEALTH_READINESS_PATH = "/health/ready";


it is not being used.

eddumelendez · 2023-07-31T23:37:27Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+     * @return self
+     */
+    public ApplicationServerContainer withAppContextRoot(@NonNull String appContextRoot) {
+        if (Objects.nonNull(this.appContextRoot) && this.appContextRoot == this.readinessPath) {


Suggested change

if (Objects.nonNull(this.appContextRoot) && this.appContextRoot == this.readinessPath) {

if (Objects.nonNull(this.appContextRoot) && this.appContextRoot.equals( this.readinessPath)) {

eddumelendez · 2023-07-31T23:47:23Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+        if (path.matches(".*\\.jar|.*\\.war|.*\\.ear|.*\\.rar")) {
+            return path.substring(path.lastIndexOf("/"));
+        }
+        throw new IllegalArgumentException("File did not contain an application archive");


Suggested change

throw new IllegalArgumentException("File did not contain an application archive");

throw new IllegalArgumentException("File did not contain a valid application archive");

eddumelendez · 2023-08-01T00:32:48Z

...r-commons/src/main/java/org/testcontainers/applicationserver/ApplicationServerContainer.java

+     * @param httpPort - The HTTP port used by the Application platform
+     * @return self
+     */
+    public ApplicationServerContainer withHttpPort(int httpPort) {


I suggest to move the method body to the configure method. The logic about replacePort, appendPort seems a little bit odd here. I think we don't need to overwrite setExposedPorts neither.

Tests in ApplicationServerContainerTest should call configure before getExposedPorts() in order to get the proper container configuration. Also, I'm not sure order is required for ports. LMK if I'm missing something

Please, also add tests when readinessPort is set.

KyleAure force-pushed the 7098-application-container branch from dd5dd51 to 4f226f9 Compare June 7, 2023 15:07

eddumelendez reviewed Jun 7, 2023

View reviewed changes

KyleAure marked this pull request as ready for review June 13, 2023 14:13

KyleAure requested a review from a team as a code owner June 13, 2023 14:13

KyleAure changed the title ~~WIP blueprint for application containers~~ Introduce application containers Jun 13, 2023

eddumelendez requested changes Jun 15, 2023

View reviewed changes

KyleAure added 6 commits June 19, 2023 10:24

WIP blueprint for application containers

715ff0b

Enhancements

7cdfe5e

feedback and refinement

ec310b1

More improvements

3d48b93

enhancement lazy env vars

94348f1

additional feedback

2a0359c

KyleAure force-pushed the 7098-application-container branch from b957fd8 to 2a0359c Compare June 19, 2023 15:24

spotless

a656654

eddumelendez reviewed Jun 20, 2023

View reviewed changes

modules/liberty/src/main/java/org/testcontainers/containers/LibertyServerContainer.java Outdated Show resolved Hide resolved

eddumelendez added the type/new module label Jun 28, 2023

More feedback

ee4c13f

github-actions bot added the type/docs label Jun 30, 2023

eddumelendez reviewed Jul 26, 2023

View reviewed changes

eddumelendez requested changes Aug 1, 2023

View reviewed changes

Merge branch 'main' into 7098-application-container

e6d20bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce application containers #7162

Introduce application containers #7162

KyleAure commented Jun 7, 2023

eddumelendez left a comment

eddumelendez left a comment

eddumelendez Jun 15, 2023

eddumelendez Jun 15, 2023

KyleAure Jun 19, 2023

eddumelendez Nov 13, 2023

eddumelendez Jun 15, 2023

KyleAure Jun 19, 2023

eddumelendez Jun 20, 2023

KyleAure Jun 30, 2023

eddumelendez Aug 1, 2023

KyleAure commented Jun 19, 2023 •

edited

Loading

eddumelendez Jul 26, 2023

eddumelendez Jul 26, 2023

eddumelendez left a comment

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Jul 31, 2023

eddumelendez Aug 1, 2023

	public ApplicationServerContainer withArchvies(@NonNull Archive<?>... archives) {
	public ApplicationServerContainer withArchives(@NonNull Archive<?>... archives) {

	test {
	javaLauncher = javaToolchains.launcherFor {
	languageVersion = JavaLanguageVersion.of(11)
	}
	}

	compileTestJava {
	javaCompiler = javaToolchains.compilerFor {
	languageVersion = JavaLanguageVersion.of(11)
	}
	options.release.set(11)
	}

	liberty.withEnv("DB_URL", mockDatabase.getNetworkAliases().get(0) + ":1080");
	liberty.withEnv("DB_URL", "mockDatabase:1080");


		//Note cannot use mockDatabase.getEndpoint() since it will return http://localhost:56254 when instead we need http://mockDatabase:1080
		//This is unintuitive and should have a better solution.

	if (Objects.nonNull(this.appContextRoot) && this.appContextRoot == this.readinessPath) {
	if (Objects.nonNull(this.appContextRoot) && this.appContextRoot.equals( this.readinessPath)) {

	throw new IllegalArgumentException("File did not contain an application archive");
	throw new IllegalArgumentException("File did not contain a valid application archive");

Introduce application containers #7162

Are you sure you want to change the base?

Introduce application containers #7162

Conversation

KyleAure commented Jun 7, 2023

eddumelendez left a comment

Choose a reason for hiding this comment

eddumelendez 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

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

KyleAure commented Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddumelendez 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

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

KyleAure commented Jun 19, 2023 •

edited

Loading