You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/06/08 02:12:33 UTC
[GitHub] [james-project] vttranlina opened a new pull request #483: [FIX] Remove the invoke LDAP testContainer when unnecessary
vttranlina opened a new pull request #483:
URL: https://github.com/apache/james-project/pull/483
When I run the test `ReadOnlyUsersLDAPRepositoryTest,` I realized two containers about OpenLDAP had been started.
One in it is unnecessary.
The reason is `ldapContainer.start()` was called twice.
The use `static called` make docker container start whenever `DockerLdapSingleton` class be called. (Some of them don't even use it)
**Solution**
Remove the start called in static -> Let start manual.
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #483:
URL: https://github.com/apache/james-project/pull/483#discussion_r647094134
##########
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:
No worries :)
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by 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 mistake manually the start twice, then you have the same problem as initially with your solution
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by 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 you changed 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 mistake manually the start twice, then you have the same problem as initially with your solution
--
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
[GitHub] [james-project] vttranlina commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
vttranlina commented on a change in pull request #483:
URL: https://github.com/apache/james-project/pull/483#discussion_r647086591
##########
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:
Your way is better (y)
Honestly, I had a worry about "you don't know if that static call is used in some other tests".
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by 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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by 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 you changed 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.
While with this you are more sure you have only one container running. If you call by mistake manually the start twice, then you have the same problem as initially with your solution
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #483:
URL: https://github.com/apache/james-project/pull/483#discussion_r647094134
##########
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:
No worries :)
But very good catch though^^
--
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
[GitHub] [james-project] chibenwa merged pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #483:
URL: https://github.com/apache/james-project/pull/483
--
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
[GitHub] [james-project] Arsnael commented on pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #483:
URL: https://github.com/apache/james-project/pull/483#issuecomment-856409148
> Though I think we get the same limitations that with Cassandra container: we likely leak the images...
I disagree with this change. I would see it more the other way around, like we do for Cassandra for example. We do check if the container is running before starting it. I would imagine the same, to be sure. Sure we can see if the tests are passing but you don't know if that static call is used in some other tests where we don;t start it manually.
--
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
[GitHub] [james-project] chibenwa commented on pull request #483: [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #483:
URL: https://github.com/apache/james-project/pull/483#issuecomment-856398069
Can we use `JAMES-2597` as a ticket number?
--
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
[GitHub] [james-project] Arsnael commented on a change in pull request #483: JAMES-2597 [FIX] Remove the invoke LDAP testContainer when unnecessary
Posted by 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 mistake 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