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

Introduce DockerOpenPaasExtension #1535

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Feb 13, 2025

Closes #1527

TODO

  • Wait for OpenPaas server to warmup.

- OPENPASS_BASIC_AUTH=YWRtaW5Ab3Blbi1wYWFzLm9yZzpzZWNyZXQ=
networks:
- emaily
-
Copy link
Member

Choose a reason for hiding this comment

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

We need more healthcheck if not we will get more unstable tests in future

users.add(new OpenPaasUser("user" + i + "@open-paas.org", "secret"));
}

return users;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we need a api for "add test users"
if don't want to all user in Tmail is "[email protected]" for compatible

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been searching for a way to do this for about a day now but haven’t found anything. Our best option is to inject an environment variable to indicate how many test users to create and then recompile the OpenPaas Docker image.

Or we need to interact with MongoDB directly and reproduce the below script. We need an extra dependency for that.

https://github.com/linagora/openpaas-esn/blob/611e83067b410e5688b5a13d1e2700f410d7506e/fixtures/populate/index.js#L117

Copy link
Member

Choose a reason for hiding this comment

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

Let's create ourselves into the mongo container the users we need just like this script does?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Otherwize it looks wonderful!

…agora/tmail/DockerOpenPaasExtensionTest.java

Co-authored-by: Benoit TELLIER <[email protected]>

import reactor.core.publisher.Mono;

public class DockerOpenPaasPopulateService {
Copy link
Member

Choose a reason for hiding this comment

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

The name looks bad. I see nothing connected to docker (we directly connect to mongo).

How about OpenPaaSProvisioningService ?

hostname: esn
image: linagora/esn:branch-1.11
ports:
- "8080:8080"
Copy link
Member

Choose a reason for hiding this comment

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

Binding fixed ports is an anti pattern as

  • it prevents parallel run
  • might conflict with other services I run locally when testing

Ideally docker-compose services shall speak on their private network,
And the Java code shall speak to private containers IPs and not to a local port.

@Test
void allServersShouldStartSuccessfully() {
assertTrue(dockerOpenPaasExtension.getDockerOpenPaasSetupSingleton().getAllContainers()
.stream().allMatch(this::checkContainerStarted));
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
.stream().allMatch(this::checkContainerStarted));
.stream().allMatch(ContainerState::isRunning));

Maybe

Comment on lines +51 to +52
private final MongoCollection<Document> usersCollection = database.getCollection("users");
private final MongoCollection<Document> domainsCollection = database.getCollection("domains");
Copy link
Member

Choose a reason for hiding this comment

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

Those do not needs to be fields IMO

Comment on lines +60 to +79
private Mono<Document> joinDomain(Document user, Document domain) {
return Mono.from(usersCollection
.findOneAndUpdate(
eq("_id", user.get("_id")),
push("domains", new Document("domain_id", domain.get("_id")))));
}

public Mono<Document> createUser() {
UUID randomUUID = UUID.randomUUID();
Document userToSave = new Document()
.append("firstname", "User_" + randomUUID)
.append("lastname", "User_" + randomUUID)
.append("password", "secret")
.append("accounts", List.of(new Document()
.append("type", "email")
.append("emails", List.of("user_" + randomUUID + "@open-paas.org"))));

return Mono.from(usersCollection.insertOne(userToSave))
.then(joinDomain(userToSave, getOpenPaasDomain()));
}
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
private Mono<Document> joinDomain(Document user, Document domain) {
return Mono.from(usersCollection
.findOneAndUpdate(
eq("_id", user.get("_id")),
push("domains", new Document("domain_id", domain.get("_id")))));
}
public Mono<Document> createUser() {
UUID randomUUID = UUID.randomUUID();
Document userToSave = new Document()
.append("firstname", "User_" + randomUUID)
.append("lastname", "User_" + randomUUID)
.append("password", "secret")
.append("accounts", List.of(new Document()
.append("type", "email")
.append("emails", List.of("user_" + randomUUID + "@open-paas.org"))));
return Mono.from(usersCollection.insertOne(userToSave))
.then(joinDomain(userToSave, getOpenPaasDomain()));
}
public Mono<Document> createUser(Document domain) {
UUID randomUUID = UUID.randomUUID();
Document userToSave = new Document()
.append("firstname", "User_" + randomUUID)
.append("lastname", "User_" + randomUUID)
.append("password", "secret")
.append("domains", List.of(new Document("domain_id", domain.get("_id"))))
.append("accounts", List.of(new Document()
.append("type", "email")
.append("emails", List.of("user_" + randomUUID + "@open-paas.org"))));
return Mono.from(usersCollection.insertOne(userToSave))
.then(joinDomain(userToSave, getOpenPaasDomain()));
}

Create then update do seems to be accidental. Why not create upfront the right document ?

Comment on lines +50 to +51
new File(
DockerOpenPaasSetup.class.getResource("/docker-openpaas-setup.yml").toURI()))
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
new File(
DockerOpenPaasSetup.class.getResource("/docker-openpaas-setup.yml").toURI()))
new File(DockerOpenPaasSetup.class.getResource("/docker-openpaas-setup.yml").toURI()))

Comment on lines +93 to +94
return List.of(
getOpenPaasContainer(),
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 List.of(
getOpenPaasContainer(),
return List.of(getOpenPaasContainer(),

Comment on lines +47 to +50
if (testsPlayedCount > MAX_TEST_PLAYED) {
testsPlayedCount = 0;
restart();
}
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
if (testsPlayedCount > MAX_TEST_PLAYED) {
testsPlayedCount = 0;
restart();
}
if (testsPlayedCount > MAX_TEST_PLAYED) {
testsPlayedCount = 0;
restart();
}

This looks copied and pasted from the Cassandra code where we have good reasons to regularly restart (or do we?)
Here this sounds like early optimisation to me.
Unless provided with a good reason I'd just never restart it for the time being...

Comment on lines +48 to +53
@Override
public void beforeAll(ExtensionContext context) {
DockerOpenPaasSetupSingleton.incrementTestsPlayed();
DockerOpenPaasSetupSingleton.restartIfMaxTestsPlayed();
}

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
@Override
public void beforeAll(ExtensionContext context) {
DockerOpenPaasSetupSingleton.incrementTestsPlayed();
DockerOpenPaasSetupSingleton.restartIfMaxTestsPlayed();
}

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.

Create DockerOpenPaasExtension
3 participants