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

Feature/solace example #6432

Closed
wants to merge 8 commits into from
Closed

Feature/solace example #6432

wants to merge 8 commits into from

Conversation

infoShare
Copy link
Contributor

Example for Solace Pub Sub (solace/solace-pubsub-standard)

@infoShare infoShare requested a review from a team as a code owner January 24, 2023 10:51
Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @infoShare ! I think would have been much better trying to give an overview about the implementation instead of investing time in the example itself. IMHO, the example should provide different use-cases instead of the most complex one. I wonder if all of this setup is needed just to send a message... probably, yes.

One thing to consider, testcontainers-java is built with Java 8 and there are some features such as Collections factories that will not be available. As a consequence, the build will fail.

Comment on lines 79 to 80
MountableFile.forHostPath(getResourceFile("solace.pem")),
MountableFile.forHostPath(getResourceFile("rootCA.crt"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MountableFile.forHostPath(getResourceFile("solace.pem")),
MountableFile.forHostPath(getResourceFile("rootCA.crt"))
MountableFile.forClasspathResource("solace.pem"),
MountableFile.forClasspathResource("rootCA.crt")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

SolaceContainer solace = new SolaceContainer(DEFAULT_IMAGE_NAME.withTag(DEFAULT_TAG))
.withCredentials("user", "pass")
.withTopic(TOPIC_NAME, Protocol.SMF)
.withVpn("test_vpn")
Copy link
Member

Choose a reason for hiding this comment

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

is vpn required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one VPN configured (but it has a lot of more additional configuration) - and it's not the best one for certificates usage.


@Override
protected void configure() {
addEnv("system_scaling_maxconnectioncount", String.valueOf(maxConnections));
Copy link
Member

Choose a reason for hiding this comment

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

it can be added using withEnv(key, value). So, I would remove it from here.

}

private MountableFile createConfigurationScript() throws IOException {
Path scriptFile = Files.createTempFile("script", ".cli");
Copy link
Member

Choose a reason for hiding this comment

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

instead of create a tmp file and writing into it, create the script in the string and the use Transferable.of(string). updateConfigScript can be deleted.

Comment on lines 372 to 397
private void waitOnCommandResult(List<String> command, String waitingFor) {
waitUntilConditionIsMet(
() -> {
try {
return execInContainer(command.toArray(new String[0])).getStdout().contains(waitingFor);
} catch (IOException | InterruptedException e) {
logger().error("Could not execute command {}: {}", command, e.getMessage());
return true;
}
},
30
);
}

private void waitUntilConditionIsMet(BooleanSupplier awaitedCondition, int timeoutInSec) {
boolean done;
long startTime = System.currentTimeMillis();
do {
done = awaitedCondition.getAsBoolean();
try {
Thread.sleep(500);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
} while (!done && System.currentTimeMillis() - startTime < timeoutInSec * 1000);
}
Copy link
Member

Choose a reason for hiding this comment

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

you may want to use awaitility instead

Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

Hi @infoShare ! Thanks again for updating the PR. I'm submitting more comments and also want to let you know that we are glad to accept this as a new module instead of only an example. As a more knowledgeable about solace, would you be able to perform those changes? :)

Comment on lines 207 to 226
public SolaceContainer(final DockerImageName dockerImageName) {
super(dockerImageName.toString());
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
this.waitStrategy = Wait.forLogMessage(SOLACE_READY_MESSAGE, 1).withStartupTimeout(Duration.ofSeconds(60));
}

@Override
protected void configure() {
withCreateContainerCmdModifier(cmd -> {
cmd
.getHostConfig()
.withShmSize(SHM_SIZE)
.withUlimits(new Ulimit[] { new Ulimit("nofile", 2448L, 6592L) });
});
configureSolace();
}

private void configureSolace() {
withCopyToContainer(createConfigurationScript(), TMP_SCRIPT_LOCATION);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public SolaceContainer(final DockerImageName dockerImageName) {
super(dockerImageName.toString());
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
this.waitStrategy = Wait.forLogMessage(SOLACE_READY_MESSAGE, 1).withStartupTimeout(Duration.ofSeconds(60));
}
@Override
protected void configure() {
withCreateContainerCmdModifier(cmd -> {
cmd
.getHostConfig()
.withShmSize(SHM_SIZE)
.withUlimits(new Ulimit[] { new Ulimit("nofile", 2448L, 6592L) });
});
configureSolace();
}
private void configureSolace() {
withCopyToContainer(createConfigurationScript(), TMP_SCRIPT_LOCATION);
}
public SolaceContainer(final DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
withCreateContainerCmdModifier(cmd -> {
cmd
.getHostConfig()
.withShmSize(SHM_SIZE)
.withUlimits(new Ulimit[] { new Ulimit("nofile", 2448L, 6592L) });
});
this.waitStrategy = Wait.forLogMessage(SOLACE_READY_MESSAGE, 1).withStartupTimeout(Duration.ofSeconds(60));
}
@Override
protected void configure() {
withCopyToContainer(createConfigurationScript(), TMP_SCRIPT_LOCATION);
}

Comment on lines +408 to +413
AMQP("amqp", 5672, "amqp", false),
MQTT("mqtt", 1883, "tcp", false),
REST("rest", 9000, "http", false),
SEMP("semp", 8080, "http", false),
SMF("smf", 55555, "tcp", true),
SMF_SSL("smf", 55443, "tcps", true);
Copy link
Member

Choose a reason for hiding this comment

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

we should cover those protocols with tests

}

public String getUsername() {
return username;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return username;
return this.username;

}

public String getPassword() {
return password;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return password;
return password;
Suggested change
return password;
return this.password;

}

public Integer getPort() {
return port;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return port;
return this.port;

}

private String getProtocol() {
return protocol;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return protocol;
return this.protocol;

}

public String getService() {
return service;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return service;
return this.service;

}

public boolean isSupportSSL() {
return supportSSL;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return supportSSL;
return this.supportSSL;

}

public String getVpn() {
return vpn;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return vpn;
return this.vpn;

"grep",
"-R",
SOLACE_ACTIVE_MESSAGE,
"/usr/sw/jail/logs/system.log"
Copy link
Member

Choose a reason for hiding this comment

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

wonder if system.log is displayed as part of the container log. If so, checking against the container log would be easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly - it is not displayed. There is no information about that in the logs.

@infoShare
Copy link
Contributor Author

I'll do that in the next week !

@infoShare
Copy link
Contributor Author

infoShare commented Feb 6, 2023

@eddumelendez
Changes pushed to the clean branch with new PR #6583

@eddumelendez
Copy link
Member

Closing in favor of PR #6583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants