-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
support empty password for mysql #899
support empty password for mysql #899
Conversation
added MYSQL_ALLOW_EMPTY_PASSWORD=yes option when the passwrod is empty.
Looks good, can you please add a test case for using an empty password? |
@kiview: sure! |
modules/jdbc-test/src/test/java/org/testcontainers/junit/EmptyPasswordMysqlTest.java
Show resolved
Hide resolved
Add unit test for empty password with non root user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test for the exception condition? This needs to be done without @Rule
, but instead by starting the container manually.
Please also make the sure the file is formatted correctly (see the .editorconfig
file in the repository root).
addEnv("MYSQL_PASSWORD", password); | ||
addEnv("MYSQL_ROOT_PASSWORD", password); | ||
} else { | ||
if("root".equalsIgnoreCase(username)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use username
as the object for the method invocation.
if(password != null && !password.isEmpty()){ | ||
addEnv("MYSQL_PASSWORD", password); | ||
addEnv("MYSQL_ROOT_PASSWORD", password); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use an else if
here and lose one level of nesting.
.withPassword("") | ||
.withEnv("MYSQL_ROOT_HOST", "%"); | ||
|
||
container.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do a try-catch here, catching ContainerException
.
.withDatabaseName(DB_NAME) | ||
.withUsername(USER) | ||
.withPassword("") | ||
.withEnv("MYSQL_ROOT_HOST", "%"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, if we'd always need to add this and so we could put it into configure()
as well?
} else if (MYSQL_ROOT_USER.equalsIgnoreCase(username)) { | ||
addEnv("MYSQL_ALLOW_EMPTY_PASSWORD", "yes"); | ||
} else { | ||
throw new ContainerLaunchException("Empty password can be used only with the root user"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found out, that we catch all exceptions and throw ContainerLaunchException
in GenericContainer
's doStart()
method, so it's really not that important which exception we throw here, but ContainerLaunchException
makes sense of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiview: should we check the cause of the exception to make sure that the ContainerLaunchException is thrown by this method instead of others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking something like this at first also. But in the end, we just want to check the public API and the container behaves as expected if we receive this exception, so I'd be good with it.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me - thanks for the contribution @chungngoops !
Merged, thanks a lot @chungngoops. |
Unfortunately, this doesn't work if you launch the container via JDBC URL scheme, for example, with The problem is that Can you provide a fix for this case? |
Never mind, just noticed this got fixed in #2207. :) |
@OverRide addEnv("MYSQL_ALLOW_EMPTY_PASSWORD", "yes"); will be done for such configuration: but with this configuration container does not start with error message: |
related issue: #896
added MYSQL_ALLOW_EMPTY_PASSWORD=yes option when the passwrod is empty.