[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary

[GitBox <gi...@apache.org>]

Arsnael commented on a change in pull request #483:
URL: https://github.com/apache/james-project/pull/483#discussion_r647083602



##########
File path: server/data/data-ldap/src/test/java/org/apache/james/user/ldap/DockerLdapSingleton.java
##########
@@ -34,7 +34,4 @@
         .password(ADMIN_PASSWORD)
         .build();
 
-    static {
-        ldapContainer.start();

Review comment:
       Instead of removing this, I would more likely go to `LdapGenericContainer` class and change the start method to:
   
   ```
   public void start() {
           if (!container.isRunning()) {
               container.start();
           }
       }
   ```
   All methods starting the container point to this one. If you do what is above if you have some tests where you don't start it manually, you gonna spend quite some time debugging with the CI to spot and fix them. 
   
   And also with this you are more sure you have only one container running. If you call by mistaking manually the start twice, then you have the same problem as initially




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org