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

Platform integration: Download all databases, Allow a primary relationship for the main 'db', fixes #4415, fixes platformsh/ddev-platformsh#59 #4426

Merged
merged 9 commits into from Dec 12, 2022

Conversation

rfay
Copy link
Member

@rfay rfay commented Dec 2, 2022

The Problem/Issue/Bug:

How this PR Solves The Problem:

  • Use relationships to download all the upstream databases
  • If one should be primary, set it with PLATFORM_PRIMARY_RELATIONSHIP and it will go into the database named 'db'

This also fixes a bug in handling blank --environment in ddev pull

Manual Testing Instructions:

Try it out on a multisite or whatever, ddev config --web-environment-add=PLATFORM_PRIMARY_RELATIONSHIP=maindb or ddev pull platform --environment=PLATFORM_PRIMARY_RELATIONSHIP=maindb

Automated Testing Overview:

Multiple databases are not yet covered in TestPlatformPull.

Related Issue Link(s):

Release/Deployment notes:

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

@rfay rfay marked this pull request as draft December 3, 2022 23:29
@bserem
Copy link
Collaborator

bserem commented Dec 5, 2022

I can verify that this works nicely. I tried it with the config.yaml and then did a "plain" ddev pull platform. All was smooth.

Thanks

@rfay
Copy link
Member Author

rfay commented Dec 5, 2022

Thanks @bserem - I think I may try to just get it to pull all databases.

@rfay rfay force-pushed the 20221202_platform_db_relationship branch from 5a80864 to 1e1eefa Compare December 8, 2022 18:50
@rfay rfay changed the title Platform integration: Allow a primary relationship for db download Platform integration: Download all databases, Allow a primary relationship for the main 'db' Dec 8, 2022
@rfay rfay requested a review from bserem December 8, 2022 18:55
@rfay rfay marked this pull request as ready for review December 8, 2022 18:57
@rfay
Copy link
Member Author

rfay commented Dec 8, 2022

@bserem @gilzow @lolautruche this should be ready for your review. Much more mature regarding databases and ddev pull platform now. If possible, please try it out sooner rather than later so it can make it into v1.21.4, which is in the next couple of days.

@gilzow
Copy link
Collaborator

gilzow commented Dec 8, 2022

I don't know if these are "issues" or not and whether they are ddev issues or add-on issues, but here's what I observed using this version of ddev when adding in the platformsh-ddev addon

  • After answering the question regarding my platform project id, ddev "hung" for more than 90 seconds, but less than 120. I was just about to Ctrl+C the process when it finally continued.
  • After the above, I was warned about reconfiguring an existing project and that i could now run ddev start BUT ddev wasn't actually finished and then prompted me for an environment name
  • I was prompted to enter my environment name. after doing so, I was again given the same warning about reconfiguring an existing application and then told I could do a restart but the same as previous, ddev wasn't actually finished.
  • DDEV finished the add-on setup and I ran ddev start. During start, about half of my composer dependencies failed to install. Example:
  - Installing wpackagist-plugin/guest-author (2.2): Extracting archive
    Install of wpackagist-plugin/guest-author failed

After ddev start completed, I ssh'ed into the web app container and ran composer install manually. Everything installed correctly.

  • Exited ssh, and then ran ddev pull platform. I only have a single database. DB pull worked just fine, but DDEV errored out during the files pull portion with
  [ProcessFailedException]
  The command failed with the exit code: 255

@rfay
Copy link
Member Author

rfay commented Dec 8, 2022

Thanks for testing @gilzow !

  • A new ddev version has to grab new images, and unless you have DDEV_DEBUG=true that might not be visible to you.
  • DDEV doesn't know about environment names, so can't prompt for them, so that is likely something from the add-on I would imagine. But when you give it an environment name, it does a ddev config to update it, so that's why you saw the info about updating the config a second time. That will be smoothed out in the future.
  • About Install of wpackagist-plugin/guest-author failed - You're using macOS right? But are you using Docker Desktop/Colima/etc? If colima, you really have to have mutagen enabled for ddev composer to work right. In that case I recommend using it globally with ddev config global --mutagen-enabled
  • but DDEV errored out during the files pull portion with... The add-on is not compatible with multiple relationships at this point. I should have mentioned that.

Could you please test without the add-on, since the add-on isn't compatible with multiple db relationships? You can just clone your project into a different directory, and use ddev pull platform on it.

@rfay rfay changed the title Platform integration: Download all databases, Allow a primary relationship for the main 'db' Platform integration: Download all databases, Allow a primary relationship for the main 'db', fixes #4415, fixes platformsh/ddev-platformsh#59 Dec 8, 2022
@bserem
Copy link
Collaborator

bserem commented Dec 9, 2022

Thanks @rfay! Here's my test-run and some thoughts.

Platform.sh Config

For reference, this how DBs are under PSH. This is used for both tests below.

.platform.app.yaml:

relationships:
  database: 'mysqldb:mysql'
  legacy: 'legacydb:mysql'

.platform/services.yml:

mysqldb:
  type: mariadb:10.4
  disk: 1500
legacydb:
    type: mariadb:10.2
    disk: 1500

Test run 1

  1. Downloaded the artifact from the comment above. It was different from the one I got 5 days ago, so I suppose it's a new one with all the latest changes.
  2. Fired up a project with multiple relationships
  3. The project already has PLATFORM_PRIMARY_RELATIONSHIP=database into config.yaml
  4. ddev pull platform --skip-files pulls and imports 2 databases, named db and main.
    • main doesn't really mean anything to me at this point. Where does the word main come from? It is hard to understand what's going on while the script runs.
    • It would be great to allow configuration to download+import only one of the databases.
    • Post DB import hooks run twice, but since these are drush commands in my case they run against the same DB twice.

Output:

Obtaining databases...
Creating gzipped SQL dump file: /var/www/html/.ddev/.downloads/main.sql.gz
PLATFORM_PRIMARY_RELATIONSHIP=database so using it as database 'db' instead of the upstream 'main'
Creating gzipped SQL dump file: /var/www/html/.ddev/.downloads/db.sql.gz
Downloaded db dumps for databases 'main main'
Importing databases [.../.ddev/.downloads/db.sql.gz .../.ddev/.downloads/main.sql.gz]

Test run 2

  1. Try again on a project where PLATFORM_PRIMARY_RELATIONSHIP is not configured.
  2. ddev pull platform --skip-files pulls and imports only one database, called main
    • psh:main gets imported into ddev:main
    • ddev:db database remains empty
  1. Drupal project can't start without db database.

Output

Obtaining databases...
Creating gzipped SQL dump file: /var/www/html/.ddev/.downloads/main.sql.gz
File exists: /var/www/html/.ddev/.downloads/main.sql.gz. Overwrite? [Y/n] y
Creating gzipped SQL dump file: /var/www/html/.ddev/.downloads/main.sql.gz
Downloaded db dumps for databases 'main main'
Importing databases [.../.ddev/.downloads/main.sql.gz]

Thoughts

  1. It would be nice to display the names as in .platform.app.yaml, so developers know what's going when the script runs
    1. If no primary_relationship is configured things won't work properly. It would make sense to get the relationship called database for the primary, and only that one. Currently getting only the other one.
  2. An option to only download and import one database would make sense, since both of them might not be required for a handful of scenarios (frontenders don't need migration data for example etc)
  3. Overall I feel it's getting there, but it's a bit rough on the edges. Verbose output of the script makes you wonder what's going on and post import hooks run twice.
  4. Definitely on a good path here! It would make things easier for multi-db projects. Thanks once again!

I hope I expressed my thoughts clearly :)


Like #4417, my time regarding ddev+platformsh is sponsored by Annertech

cmd/ddev/cmd/pull.go Outdated Show resolved Hide resolved
Co-authored-by: Bill Seremetis <bill@seremetis.net>
@rfay
Copy link
Member Author

rfay commented Dec 9, 2022

I'd be interested in votes on whether this should go in as is. I'm pretty sure the only risk is to platform.sh users doing multiple databases, which they can't do right now, and they or we can improve the platform.yaml as we go. However, it could wait and not go into this release, although there's a bugfix in there that does need to go in.

@rfay
Copy link
Member Author

rfay commented Dec 12, 2022

Some responses to your questions @bserem

  1. "main doesn't really mean anything to me at this point. Where does the word main come from? It is hard to understand what's going on while the script runs." - main is the name of one of your relationships; you can platform relationships to see the relationships.
  2. "It would be great to allow configuration to download+import only one of the databases" - I don't disagree, but since this is already an edge case for most people, I think it can wait and see if people need that.
  3. "Post DB import hooks run twice, but since these are drush commands in my case they run against the same DB twice" - post-import-db hooks run after a db import, and in your case there are two DB imports. You could improve your post-import-db hook to check which database it's importing and skip the drush install for one that doesn't matter though.
  4. Test Run 2: Without PLATFORM_PRIMARY_RELATIONSHIP, you would need to use $PLATFORM_RELATIONSHIPS to determine the proper settings. But your settings.ddev.php won't know what those are. They can be configured a number of ways. PLATFORM_PRIMARY_RELATIONSHIP short-circuits that and puts the primary db into the 'db' database. But in some cases you'd want the databases to be named the same in DDEV as they are on Platform.sh. That's what you were actually doing in this test run.

Also note that platformsh/ddev-platformsh cannot currently work against multiple-database projects at all, unrelated to this PR.

Thanks so much for the great and detailed feedback.

@rfay
Copy link
Member Author

rfay commented Dec 12, 2022

@gilzow I just finally "processed" your statement "Exited ssh, and then ran ddev pull platform. I only have a single database. DB pull worked just fine, but DDEV errored out during the files pull portion with The command failed with the exit code: 255"

Would you be able to share that project with me?

@rfay
Copy link
Member Author

rfay commented Dec 12, 2022

I'm going to go for it and pull this; I don't believe it will affect folks with a single database, and will do significant testing of this feature in the release testing process. Conversation and responses here and additional issues are all very welcome.

@rfay rfay merged commit 3104ea0 into ddev:master Dec 12, 2022
@rfay rfay deleted the 20221202_platform_db_relationship branch December 12, 2022 17:40
@bserem
Copy link
Collaborator

bserem commented Dec 13, 2022

Good news on this becoming part of DDEV. It'll help a lot :)

Now, regarding main, I tested what you mention above and I can't find a main relationship:
image

My relationships are:

That's why main sounds strange, it doesn't seen to come from my config.

@rfay
Copy link
Member Author

rfay commented Dec 13, 2022

main is the name of the database/schema, given in path. Sorry I misled you.

If you platform ssh and connect to db server and SHOW DATABASES; you'll see it there I think.

@@ -42,6 +42,20 @@ web_environment:
4. Run `ddev pull platform`. After you agree to the prompt, the current upstream database and files will be downloaded.
5. Optionally use `ddev push platform` to push local files and database to Platform.sh. The [`ddev push`](../basics/commands.md#push) command can potentially damage your production site, so we don’t recommend using it.

If you have more than one database on your Platform.sh project, you'll need to choose which one you want to use
Copy link
Sponsor Collaborator

@mattstein mattstein Dec 13, 2022

Choose a reason for hiding this comment

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

@rfay I understand only one database will be handled by the integration, but what would you recommend if I had additional databases I also wanted to pull into DDEV?

Create my own modified version of platform.yaml and modify db_pull_command?

I ask because I’m looking at the docs, and designating a primary naturally begs the question of what I should do with my other ones. Even if it’s just a hint, it seems like it’d be worth addressing.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this version, all databases will be handled by the ddev pull platform, so line 45 is just incorrect. Thanks for catching that.

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

Am I understanding correctly that all databases will be pulled, but that I still need to designate a PLATFORM_PRIMARY_RELATIONSHIP so DDEV knows what to use for its (default) 'db' database?

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

Ultimately asking while I attempt to improve the wording over here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mattstein this is correct.

  • All databases will be downloaded
  • The remote database that is flagged as primary will be imported into the db database of DDEV (the default one)
  • All other remote databases will be imported into ddev databases with the same as in the remote

I believe I've never used the word database that much in such a short message

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

@bserem Thanks for clarifying the database behavior for this database-related documentation adjustment regarding databases. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all projects will need for a "db" database to be populated. For example, as the ddev-platformsh add-on matures, it should be able to handle databases of any number that have the same names they do on Platform.sh, and the "db" database not be used at all.

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

So this is more of an if you have multiple databases and want to designate a primary to use with 'db', yes? If so I’ll update #4469 accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct. Most people who have ever used DDEV will want that. There may be contexts in the future where that's not needed. The same may be true for Acquia or Pantheon or other hosts, where the settings are wired to particular database names and people are not using DDEV's automatically generated settings, like settings.ddev.php or Craft CMS's .env configuration.

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

Revised in this commit, which is hopefully more accurate! 😅

mattstein added a commit to mattstein/ddev that referenced this pull request Dec 19, 2022
rfay pushed a commit that referenced this pull request Dec 20, 2022
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.

None yet

4 participants