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

MySQL: Fix problem of starting up mysql 5.7.33 version when user is root #3953

Conversation

seveneves
Copy link
Contributor

@seveneves seveneves commented Mar 31, 2021

Fixes following problem in console of mysql container

java.lang.IllegalStateException: Container did not start correctly.
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:435)
	at org.testcontainers.containers.GenericContainer.lambda$doStart$0(GenericContainer.java:325)
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:81)
	at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:323)
	at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:311)
[ERROR] [Entrypoint]: MYSQL_USER="root", MYSQL_USER and MYSQL_PASSWORD are for configuring a regular user and cannot be used for the root user
    Remove MYSQL_USER="root" and use one of the following to control the root user password:
    - MYSQL_ROOT_PASSWORD
    - MYSQL_ALLOW_EMPTY_PASSWORD
    - MYSQL_RANDOM_ROOT_PASSWORD

Fixes issue #3893

Ivan Stanislavciuc added 2 commits March 31, 2021 17:24
Fixes following problem in console of mysql container

[ERROR] [Entrypoint]: MYSQL_USER="root", MYSQL_USER and MYSQL_PASSWORD are for configuring a regular user and cannot be used for the root user
    Remove MYSQL_USER="root" and use one of the following to control the root user password:
    - MYSQL_ROOT_PASSWORD
    - MYSQL_ALLOW_EMPTY_PASSWORD
    - MYSQL_RANDOM_ROOT_PASSWORD
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Thanks @seveneves
Please could you add some tests which demonstrate the problem and can guard against regressions?

#2689 is a draft PR I created to demonstrate #2627 - this might be something you could use as a basis.

@seveneves
Copy link
Contributor Author

@rnorth
Hi. Please check my test

@seveneves seveneves requested a review from rnorth April 9, 2021 07:15
@rnorth
Copy link
Member

rnorth commented Apr 10, 2021

I've noticed a bad (existing) bug when testing this locally... Firstly, I noticed that your testRootAccountUsageWithEmptyPassword test takes >2mins to run.

It seems that setting the password to null causes a NullPointerException here, which is in turn swallowed here.

The terrible thing about the second piece of code is that the connection check fails continuously until timeout is reached, but upon hitting timeout it just continues.

We're going to need to fix that; I think we'll also need to prevent null passwords, to. I will dig a little more and may add a commit to your PR. Please stand by.

1 similar comment
@rnorth
Copy link
Member

rnorth commented Apr 10, 2021

I've noticed a bad (existing) bug when testing this locally... Firstly, I noticed that your testRootAccountUsageWithEmptyPassword test takes >2mins to run.

It seems that setting the password to null causes a NullPointerException here, which is in turn swallowed here.

The terrible thing about the second piece of code is that the connection check fails continuously until timeout is reached, but upon hitting timeout it just continues.

We're going to need to fix that; I think we'll also need to prevent null passwords, to. I will dig a little more and may add a commit to your PR. Please stand by.

@seveneves
Copy link
Contributor Author

seveneves commented Apr 10, 2021

It seems that setting the password to null causes a NullPointerException

@rnorth I noticed the slow test run but I didn't dig into the reason. Thanks for finding the reason.

I pushed a "fix" and replaced the null with an empty string. This, of course, is not a solution to the NPE but I'd like to have this PR merged separately from NPE problem. which would require a separate issue and PR.

What do you think?

@rnorth
Copy link
Member

rnorth commented Apr 11, 2021

@seveneves yes sure, I was going to suggest that. I'll follow up with a change to get null-checks where they need to be.

If you could please add my suggested change, I think we're good.

Co-authored-by: Richard North <rich.north@gmail.com>
@seveneves seveneves requested a review from rnorth April 11, 2021 10:17
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Approved, thanks for this @seveneves!
Fixes #3893, #2627

@rnorth rnorth merged commit f3ffed6 into testcontainers:master Apr 11, 2021
@bsideup bsideup added this to the next milestone Apr 11, 2021
@rnorth rnorth changed the title Fix problem of starting up mysql 5.7.33 version when user is root MySQL: Fix problem of starting up mysql 5.7.33 version when user is root Apr 13, 2021
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.

None yet

3 participants