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

Cannot import database to additional database service since upgrade to v3.21.0 #53

Closed
drupov opened this issue May 26, 2024 · 7 comments · Fixed by lando/core#173
Closed

Comments

@drupov
Copy link

drupov commented May 26, 2024

Importing into an additional database service seems to not be possible anymore, as emptying the database in question fails.

I have this Drupal 10 recipe, but it is not related to Drupal IMO:

name: drupal10
recipe: drupal10
config:
  webroot: web
services:
  db2:
    type: mysql
    portforward: true

lando db-import initial.sql.gz works ok. lando db-import initial.sql.gz --host db2 fails though, seemingly when the command tries to empty the database:

{main} ~/apps/drupal10$ lando db-import initial.sql.gz --host db2
Preparing to import /app/initial.sql.gz into database 'database' on service 'db2' as user root...

Emptying database... 
NOTE: See the --no-wipe flag to avoid this step!
ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'database' at line 1

I can confirm, that lando db-import initial.sql.gz --host db2 --no-wipe works.

@drupov drupov added the mysql label May 26, 2024
@rtfm-47 rtfm-47 removed the mysql label May 26, 2024
@rtfm-47 rtfm-47 transferred this issue from lando/lando May 26, 2024
@drupov
Copy link
Author

drupov commented Jun 3, 2024

Just saw #52

So it seems like the issue is fixed for the database in the main container, but not when you have additional database containers:

Issue

I still have the issue, when I define multiple database containers (should not be Drupal-related as in the example):

name: myproject
recipe: drupal10
config:
  webroot: web
services:
  additionaldb1:
    type: mysql
    portforward: true
  additionaldb2:
    type: mysql
    portforward: true

Now I have this:

~/apps/drupal10$ lando db-import initial.sql.gz --host additionaldb1
Preparing to import /app/initial.sql.gz into database 'database' on service 'additionaldb1' as user root...

Emptying database... 
NOTE: See the --no-wipe flag to avoid this step!
ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'database' at line 1

Importing into the main database works though:

~/apps/drupal10$ lando db-import initial.sql.gz
Preparing to import /app/initial.sql.gz into database 'drupal10' on service 'database' as user root...

Emptying drupal10... 
NOTE: See the --no-wipe flag to avoid this step!
Gzipped file detected!
Importing /app/initial.sql.gz...
Import complete!

Workaround

The lando drush sql-drop --yes - see #52 (comment) - will clear only the main database, from the recipe. In this case I use a similar approach to #52 (comment) as a workaround:

lando ssh --service=aditionaldb1
mysql -u root
drop database `database`;
create database `database`;

and then lando db-import database.sql.gz --no-wipe --host aditionaldb1.

Version

I am on

~/apps/drupal10$ lando version
v3.21.0

@reynoldsalec
Copy link
Member

reynoldsalec commented Jun 4, 2024

I'll try to replicate this, would you mind sharing your complete Landofile?

Would be good to add a test for this in our recipe tests of the lando db-import command: https://github.com/lando/lamp/tree/main/examples/lamp-import

@drupov
Copy link
Author

drupov commented Jun 4, 2024

Hey, @reynoldsalec thanks for jumping in.

The lando file from my comment is complete, you could use that.

The database I am trying to import is one I take after an initial Drupal installation, but any dummy one would suffice, e.g. the one from here.

@reynoldsalec
Copy link
Member

Confirmed thaTI'm experiencing the same issue; it looks like recent changes to the sql-import.sh script caused a regression that wasn't covered in the relevant test coverage. Basically ${DATABASE} needs to be quoted in backticks ($SQLSTART -e "DROP DATABASE IF EXISTS \${DATABASE}`"`), see https://github.com/lando/core/blob/main/scripts/sql-import.sh#L111

I'm making a PR @drupov to address this and add test coverage, thanks for putting in the issue!

@reynoldsalec
Copy link
Member

...adding to this, I think the reason the script works otherwise is because database is a reserved word in SQL that needs to be enclosed in quotes. If the env var MYSQL_DATABASE isn't set by the service, it defaults to database and creates this issue with the new version of the script. The recipes set different default names for the database and our tests explicitly set the env vars, hence why it didn't pop up.

The earlier version of the sql-import.sh script dropped all the database's tables instead of dropping the DB, hence why it probably wasn't an issue before.

reynoldsalec added a commit to lando/core that referenced this issue Jun 5, 2024
reynoldsalec added a commit to lando/core that referenced this issue Jun 7, 2024
… tests. (#173)

* lando/mysql#53: Address db-import error by quoting database name. Add tests.

* lando/mysql#53: Update the tests with correct hostnames/envvars.

* lando/mysql#53: Fix core test issue. Remove debugging code.

* Update changelog.
@reynoldsalec
Copy link
Member

...and just a note that this won't ACTUALLY be deployed until Lando v3.21.1 is released, which will probably be this Friday...just in case someone see this.

@drupov
Copy link
Author

drupov commented Jun 23, 2024

This works now with v3.21.2

Thanks a lot

pirog pushed a commit to lando/core that referenced this issue Oct 10, 2024
… tests. (#173)

* lando/mysql#53: Address db-import error by quoting database name. Add tests.

* lando/mysql#53: Update the tests with correct hostnames/envvars.

* lando/mysql#53: Fix core test issue. Remove debugging code.

* Update changelog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants