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

Add support for MySQL socket connection #171

Merged
merged 34 commits into from
Dec 21, 2023

Conversation

wojtekn
Copy link
Contributor

@wojtekn wojtekn commented Nov 10, 2023

Related wp-cli/wp-cli#5859

In this diff, I propose to add support for MySQL socket connection to the wp config create command.

I used the approach proposed by @aonsyed under #169

Testing steps:

  1. Start MySQL/MariaDB, download WordPress and unpack it
  2. Test MySQL socket connection e.g.
mysql -S /tmp/mysql.sock
  1. Create a database user
  2. Run command
vendor/bin/wp config create --dbname=dbname --dbuser=dbuser --dbpass=somevalue --dbprefix=wp_ --url=site.com --dbhost=localhost:/tmp/mysql.sock --allow-root --skip-themes --skip-packages --skip-plugins --path=/Sites/wordpress-root
  1. Confirm the success message:

Success: Generated 'wp-config.php' file.

  1. Check if the wp-config.php file was created and confirm that it has a socket configured in place of dbhost

@wojtekn wojtekn requested a review from a team as a code owner November 10, 2023 14:12
@wojtekn
Copy link
Contributor Author

wojtekn commented Nov 10, 2023

I'm unsure how to test the --dbhost=localhost:/var/run/mysqld/mysqld.sock format as it looks specific to Linux. On MacOS, I need to provide the socket file name, e.g. --dbhost=/tmp/mysql.sock.

However, as soon as the format passes the file_exists() check and mysqli_real_connect() can use it as a valid socket, it will be written to the generated wp-config.php in that form.

@danielbachhuber
Copy link
Member

Can we add a functional test for this?

@aonsyed
Copy link

aonsyed commented Nov 10, 2023

Different OSes use different paths but file check should succeed for 'unix://path/mysql.sock' or 'localhost://path/mysql.sock' too or you can use realpath() function to get the actual path to the file which would be the same in this case, on mac localhost one should work since that's supposed to point to device itself

@wojtekn
Copy link
Contributor Author

wojtekn commented Nov 10, 2023

Can we add a functional test for this?

@danielbachhuber I've added one, but I couldn't make it work on GH environments, so I added --skip-check so now it only checks if the value is written to the wp-config.php file.

Do you think it's enough? I couldn't quickly figure out how we could use MySQL with a socket connection during GH action.

@schlessera
Copy link
Member

@wojtekn At the very least, you can try to run the command without the --skip-check but with --debug, expect it to fail and assert that the debugging output shows the right MySQL command (the --debug output will dump the exact command being used, IIRC).

@danielbachhuber
Copy link
Member

@wp-cli/committers 976b7c6 passes for me locally, but then fails in the container.

What do you think we should do? I think I'd be fine adding a @broken flag for the test, and landing the PR.

@aonsyed
Copy link

aonsyed commented Nov 10, 2023

@danielbachhuber why not try the proper sock path /var/run/mysqld/mysqld.sock instead of fetching it from socket which seems to be sending both sock and port

@danielbachhuber
Copy link
Member

why not try the proper sock path /var/run/mysqld/mysqld.sock instead of fetching it from socket which seems to be sending both sock and port

@aonsyed I'm not sure I follow. Can you show me with a diff?

I'm using wp db query 'SELECT @@GLOBAL.SOCKET' --skip-column-names because the socket can vary between platforms.

@wojsmol
Copy link
Contributor

wojsmol commented Nov 10, 2023

@danielbachhuber Looking at tast failures --dbhost is pass twice - $ wp config create --dbname='wp_cli_test' --dbuser='wp_cli_test' --dbpass='password1' --dbhost='127.0.0.1:32768' --dbhost=/var/run/mysqld/mysqld.sock

@aonsyed
Copy link

aonsyed commented Nov 10, 2023

@danielbachhuber definitely it can change but it seems that docker environement for testing is pretty standard debian based linux for all the MySQL version from 5.3 to 8.0 in the your test environment and the socket path is always /var/run/mysqld/mysqld.sock so you can use that and it will pass the tests

The best solution for a controlled testing would be to change this in parameters
echo "WP_CLI_TEST_DBHOST=127.0.0.1:32768" >> $GITHUB_ENV use sock instead but that would mean changing the test environment for everything

Basically use the previous test method instead of getting socket path from variable and use /var/run/mysqld/mysqld.sock instead of /tmp/mysql.sock

@danielbachhuber
Copy link
Member

I think it's finding the right socket.

Unfortunately, there's an authentication error for < PHP 7.4:

CleanShot 2023-11-10 at 11 32 02@2x

I'm seeing Error: Database connection error (1045) Access denied for user 'wp_cli_test'@'localhost' (using password: YES) for PHP 8+

CleanShot 2023-11-10 at 11 32 59@2x

@aonsyed
Copy link

aonsyed commented Nov 11, 2023

@danielbachhuber the issue lies with {CORE_CONFIG_SETTINGS}, it already contains dbhost from the docker config here

  echo "MYSQL_HOST=127.0.0.1" >> $GITHUB_ENV
  echo "MYSQL_TCP_PORT=32768" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBROOTUSER=root" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBROOTPASS=root" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBNAME=wp_cli_test" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBUSER=wp_cli_test" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBPASS=password1" >> $GITHUB_ENV
  echo "WP_CLI_TEST_DBHOST=127.0.0.1:32768" >> $GITHUB_ENV

Alternatively, you can change the test like this

  When I run `wp config create  --dbname='wp_cli_test' --dbuser='wp_cli_test' --dbpass='password1' --dbhost={SOCKET}`

Alternatively, some other way to drop--dbhost='127.0.0.1:32768' from the {CORE_CONFIG_SETTINGS} would do the same thing.

@aonsyed
Copy link

aonsyed commented Nov 13, 2023

@wojtekn This test won't work since {CORE_CONFIG_SETTINGS} contains the DB settings from the enviorment, you have to test it without that for it to work

@wojtekn
Copy link
Contributor Author

wojtekn commented Nov 13, 2023

@aonsyed, thanks, I will check those.

In the meantime, I found out that my original implementation was not correct - it was accepting a socket file path, e.g., /tmp/mysql.sock, validating it correctly, and using that value in the generated wp-config.php file, but it was not enough. The wpdb() class expects that if dbhost is a socket, it should include the host part so the valid accepted and written format should be localhost:/tmp/mysql.sock.

@sitamet
Copy link

sitamet commented Nov 28, 2023

When trying to configure db socket connection:

wp config create --dbname=wordpress --dbuser=root --dbpass=secret --dbhost=localhost:/var/run/mysql/mysqld.sock --dbprefix=wp_

Returns:
Error: Database connection error (2002) Cannot assign requested address

Socket connections expressed as localhost:socketfile worked until v2.9.0

@swissspidy
Copy link
Member

@sitamet Just so I understand, are you reporting the same issue as #169? If so, this PR here is trying to solve it.

@danielbachhuber
Copy link
Member

The tests pass fine locally 😭

$ composer behat -- features/config-create.feature
> run-behat-tests 'features/config-create.feature'
...................................................................... 70
.........................

9 scenarios (9 passed)
95 steps (95 passed)
0m20.62s (10.89Mb)

My guess is that the tests are finding a socket in GitHub Actions, but the socket doesn't correlate to the MySQL test database. I don't have any great ideas to work around this.

@schlessera
Copy link
Member

@danielbachhuber @swissspidy FYI: I finally got this to work, but in order to do so, I had to rethink the way the database is handled in GHA. We previously had a GHA service with a MySQL image running. It turned out that this did not actually work as intended, as there was confusion between the MySQL server bundled with the Ubuntu OS and the one from the service. They were using the same name, and the "DB start" action we had was actually starting the Ubuntu-bundled one, as the other was already running.

The system I'm using now does not use a GHA service, but rather uses an action that downloads the MySQL server and then launches it. This has the following impact:

  • it lets us control when the DB docker container is being assembled. This important for the socket testing here, as a socket within a container needs to be created within a bind mount, otherwise the host system has no access. With the previous service-based approach, there was no way of establishing that bind mount before the socket was created (note: the socket file itself cannot be mounted into the host).
  • it is easier to configure the provided server, as we can adapt the my.cnf file through previous actions and other data generated during the job.
  • it removes the need for a GHA service, which was one of the blockers for testing against Windows and Macs, as GHA services are only supported on Linux images.

@schlessera schlessera merged commit 890b6e3 into wp-cli:main Dec 21, 2023
36 checks passed
@schlessera schlessera added this to the 2.3.3 milestone Dec 21, 2023
@wojtekn
Copy link
Contributor Author

wojtekn commented Dec 21, 2023

Thanks for solving it @schlessera !

it removes the need for a GHA service, which was one of the blockers for testing against Windows and Macs, as GHA services are only supported on Linux images.

This is a great outcome 👍

@danielbachhuber
Copy link
Member

Awesome! Thanks @schlessera 😊

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.

7 participants