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

Make the database container in the Docker setup more generic #5014

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented Aug 15, 2023

Currently, mysql is hard coded in several locations within the docker-compose.yml file. Although changing database types works interchangeably for the most part (Core also supports MariaDB), it is confusing to have a container named mysql that runs MariaDB.

For previous MariaDB versions, the mysql command is symlinked to the mariadb one. However, in MariaDB 11.0 that symlink is removed and causes failures. Making this container more generic will help spot commands still using mysql, and other potential issues.

Trac ticket: https://core.trac.wordpress.org/ticket/59110


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@desrosj desrosj self-assigned this Aug 15, 2023
@desrosj desrosj marked this pull request as ready for review August 15, 2023 14:56
@johnbillion
Copy link
Member

Can you add some instructions on testing this locally? I presume it needs a stop, install, and start again so the container is restarted with the correct name and the wp-config.php file is updated with the correct host name.

@desrosj
Copy link
Contributor Author

desrosj commented Aug 15, 2023

Can you add some instructions on testing this locally? I presume it needs a stop, install, and start again so the container is restarted with the correct name and the wp-config.php file is updated with the correct host name.

Sure!

To test this, you'll need to perform the following on a pre-existing setup:

  • npm run env:pull
  • npm run env:restart
  • npm run env:install

This will test the new compose file with MySQL (the default). To then test with MariaDB, run the same commands as above but run the following two commands first (or update the .env file to reflect these instead):

  • export LOCAL_DB_TYPE=mariadb
  • export LOCAL_DB_VERSION=11.0 (or any other valid MariaDB version).

Unfortunately, after this change, I believe that any local setup using the current configuration would see their database reset. I'm not sure how to avoid that with this change.

@johnbillion
Copy link
Member

We could avoid the data loss by retaining the mysql volume name, but that would somewhat reduce the effect of the ticket.

@desrosj
Copy link
Contributor Author

desrosj commented Aug 15, 2023

We could post a brief note on Making WordPress Core detailing this. Another possible path we could take is to export the database when running npm run env:stop. However, it would need to be run prior to the new compose file being put in place locally. So it would need to be a changeset preceding any including this PR.

@johnbillion
Copy link
Member

johnbillion commented Aug 15, 2023

Perhaps retaining the mysql volume name is ok?

@desrosj
Copy link
Contributor Author

desrosj commented Aug 15, 2023

I don't have strong opinions against that, but it doesn't fully resolve the potential confusion the ticket aims to address.

What if we were to include both the new one in this PR and mysql for a period of time? Then the volume would still be there, but by default it would switch to the new one. Someone could use a .docker-compose.override.yml file to switch back to export their data. We could include that in the post.

…r-images

# Conflicts:
#	.github/workflows/phpunit-tests.yml
#	docker-compose.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants