Make ddev_drush_settings.php actually work on host, fixes #454 #1036

JeremySkinner · 2018-08-07T15:33:11Z

The Problem/Issue/Bug:

drush.settings.php never did what we actually wanted it to do, see OP #454. We wanted it to work on the host so drush commands could be executed on the host. Here's hoping that @JeremySkinner isn't the only person in history that actually wanted/needed this.

How this PR Solves The Problem:

ddev_drush_settings.php is generated at ddev start time for drupal and backdrop. It's automatically included if on the host and the file is available.
Since it's generated at start time, the db host and port can be gathered and used, so the info in there is actually correct.
docs mention this and how to use it.

Manual Testing Instructions:

On a drupal6/7/8 project,

use ddev config to regenerate settings
use ddev start to start it (and generate the ddev_drush_settings.php)
Verify the content and existence of ddev_drush_settings.php
Use drush to do something, for example drush sql-cli or drush ws

Automated Testing Overview:

TestWriteDrushConfig() was completely redone.

Related Issue Link(s):

OP #454

Thanks to @JeremySkinner for making sure we finally actually got this to work.

This is currently slated for v1.1.0 but if we can't get it reviewed before that it can come in v1.2.0

CLAassistant · 2018-08-07T15:33:18Z

All committers have signed the CLA.

rfay

This is actually the issue in #454 I think and I suspect you've fixed it properly, but it should be removed from the docs and actually fixed in the generated settings.ddev.php (which is created in templates.go)

Thanks for chasing this, I think you've actually got it! The requirement is that it try to work both inside the container and on the host though. There may be a more elegant way of determining whether we're on the host. This has been untouched for like 2 years :)

JeremySkinner · 2018-08-07T17:00:03Z

Thanks @rfay, I didn't realise this was auto-generated as part of settings.ddev.php, just assumed it was an incorrect example. I've gone ahead and modified the templates and removed the snippet from the docs page and replaced it with some new wording.

I haven't changed the use of the DEPLOY_NAME as the way of checking we're outside the container...it seems good enough for now, and at least drush.settings.php is then usable.

Let me know if there's anything else you'd like changed.

rfay · 2018-08-07T17:02:36Z

This probably needs an automated test, but it's nontrivial to check the host-side drush usage on every platform, where we don't currently have drush installed. I think there are drush usage tests sprinkled around that would make sure it's working inside the web container.

JeremySkinner · 2018-08-07T17:17:18Z

I'm happy to try and add some tests for drush usage on the host side, although I'm not familiar with go so might take me a bit of time.

rfay · 2018-08-07T17:25:10Z

That seems like a lot to ask. When we test this we'll see if we can do some tests, if you'll allow push by rfay and andrewfrench to your fork (if you haven't already).

JeremySkinner · 2018-08-07T17:29:55Z

Thanks - sounds good. Yes you should have access to push to the branch.

rfay

I think the biggest problem here is that the db port is transient, which will annoy people no end, as in #941 - I'm not sure this is really even work working on if we can't fix #941 because people will just stumble on broken drush all the time.

rfay · 2018-08-08T03:14:38Z

docs/users/developer-tools.md

@@ -53,7 +53,7 @@ The port referenced is unique per running project, and randomly chosen from avai
 **Note:** The host database port is likely to change any time a project is stopped/removed and then later started again.

 ### Using Drush Installation on Host Machine
-If you have Drush installed on your host system, you can use it to interact with a ddev project by defining a `drush.settings.php` file at the docroot of your code base, and referencing it from your `settings.php` file. The `drush.settings.php` file should look similar to below, using the host port information for your project retrieved from `ddev describe`:
+If you have Drush installed on your host system, you can use it to interact with a ddev project by defining a `drush.settings.php` file at the docroot of your code base. This will be automatically referenced from the generated `settings.ddev.php` file which is located under `web/sites/default`. The `drush.settings.php` file should look similar to below, using the host port information for your project retrieved from `ddev describe`:


Despite my statement to the contrary, I don't see ddev generating this automatically on d8. Also, the path listed "web/sites/default" is highly variable, I assume that should be <docroot>/sites/default

I've changed to <docroot>/sites/default. I tested this with D8 and the include statement is actually automatically generated in settings.ddev.php (not setttings.php)

rfay · 2018-08-08T03:20:00Z

pkg/ddevapp/drupal.go

@@ -163,7 +163,7 @@ if (empty($config_directories[CONFIG_SYNC_DIRECTORY])) {
 // This determines whether or not drush should include a custom settings file which allows
 // it to work both within a docker container and natively on the host system.
 if (!empty($_SERVER["argv"]) && strpos($_SERVER["argv"][0], "drush") && empty($_ENV['DEPLOY_NAME'])) {
-  include __DIR__ . '../../../drush.settings.php';
+  include __DIR__ . '/../../../drush.settings.php';


This works inside the container (I think) but I don't think there's any guaranteed way it can work on the host.

If we're assuming that the drush.settings.php is to be in sites/default/drush.settings.php, wouldn't we just use

$drush_settings = dirname(__FILE__) . '/drush.settings.php'; if (is_readable($drush_settings)) { require $ddev_settings; }

I'd assumed there was a reason why it was referencing the doc root, rather than looking in sites/default, but if there isn't then this seems better as all the config files are then in one place.

I assume this should still check the environment variable and only do the include if we're not inside the container? Is there any reason why we'd even want this to work inside the container, if really the reason for it is to be able to access the db from the host? If so, then should we have 2 files, eg drush.settings.php and drush.host.settings.php?

JeremySkinner · 2018-08-08T07:48:26Z

Re #941 and transient ports, personally I'd much prefer to have a single db container exposed on a single port which can then be used by all the dev sites. I know that's not the 'docker way' and others probably wouldn't like this, but would fit my workflow better.

At the moment I'm just running ddev describe and updating the port in drush.settings.php manually whenever I start the project. Would it be feasible to automate that following a successful ddev start?

rfay

Well wow, I didn't intend to actually go to town with this, but thanks for the inspiration @JeremySkinner - I think this is now what it always wanted to be. The ddev_drush_settings.php is actually generated during ddev start and it has the right port number for db; also fixed that it wouldn't have had the right host on docker toolbox. The docs are now much shorter because people don't have to manage this themselves.

I'm going to take the liberty of updating the OP testing instructions etc.

Thanks so much for the inspiration, and I hope you can test it out a bit @JeremySkinner .

JeremySkinner · 2018-08-10T06:52:07Z

Thanks @rfay, that looks good and will be much more useful. Looking forward to playing with it. Thanks again for getting this in so quickly 👍👍

rfay · 2018-08-10T15:05:31Z

Artifacts from this PR can be downloaded for testing from https://circleci.com/gh/drud/ddev/3225#artifacts/containers/0 for testing - there are builds for macOS, Linux, and Windows there.

JeremySkinner · 2018-08-10T18:34:54Z

I've been using this today and it works nicely with no issues.

andrewfrench

Code changes look good and I was able to manually confirm that ddev_drush_settings.php were correctly generated on ddev start.

rfay · 2018-08-13T17:27:37Z

@andrewfrench there are probably a couple of things I slipped in here that you'll want to look at again and make sure you're good with them:

Settings directory gitignore creation happens in app.Init() instead of in post-config, so we always write our own in there. It's the same for every CMS type.
The settings.php or main settings file is not included in the gitignore. So many people actually check in a settings.php (without db credentials), that I thought it shouldn't be in there. Of course... maybe i added that and then reconsidered.

andrewfrench · 2018-08-13T18:34:47Z

pkg/ddevapp/apptypes.go

@@ -133,6 +133,10 @@ func (app *DdevApp) CreateSettingsFile() (string, error) {
 		if err != nil {
 			util.Warning("Unable to create settings file: %v", err)
 		}
+		if err := CreateGitIgnore(filepath.Dir(app.SiteSettingsPath), filepath.Base(app.SiteLocalSettingsPath), "ddev_drush_settings.php"); err != nil {


This may be my CMS ignorance showing, but isn't this very Drupal-centric? Do TYPO3 and Wordpress projects have a ddev_drush_settings.php file?

I suppose there's no harm in ignoring something that may not exist, but I think this block could be flagged for a CMS-specific refactor at some point.

My assumption is that it's ok to ignore something that doesn't exist to make the code simpler. Drush is for Drupal only, but I guess it shouldn't be offensive to people to find a non-their-CMS file gitignored.

This is actually undoing the previous CMS-specific approach, which involved 4x duplicated code.

Gotcha, I see that now. That's fine with me.

andrewfrench

Re-approved after changes

…d test

…config, piggy-back gitignore creation on that

rfay · 2018-08-14T02:20:09Z

This one fails with the same #fail as master on TestDdevRestoreSnapshot, but only there, and only on Windows 10 Home. We'll definitely have to find out what's going on there, hopefully tomorrow, but it's not related to this PR. I'm going to go ahead and pull it.

rfay requested changes Aug 7, 2018

View reviewed changes

JeremySkinner changed the title ~~Docs: Fix example path to drush.settings.php~~ #454: Fix path to drush.settings.php Aug 7, 2018

rfay changed the title ~~#454: Fix path to drush.settings.php~~ Fix usage path to drush.settings.php, fixes #454 Aug 7, 2018

rfay requested changes Aug 8, 2018

View reviewed changes

rfay force-pushed the patch-1 branch from 1e5d14d to 534a3ba Compare August 10, 2018 05:06

rfay changed the title ~~Fix usage path to drush.settings.php, fixes #454~~ Make ddev_drush_settings.php actually work on host, fixes #454 Aug 10, 2018

rfay approved these changes Aug 10, 2018

View reviewed changes

rfay self-assigned this Aug 10, 2018

rfay added this to the v1.1.0 milestone Aug 10, 2018

rfay requested review from andrewfrench and rickmanelius August 10, 2018 05:23

andrewfrench approved these changes Aug 11, 2018

View reviewed changes

rfay force-pushed the patch-1 branch 3 times, most recently from c761007 to e1d04bd Compare August 13, 2018 17:24

andrewfrench reviewed Aug 13, 2018

View reviewed changes

andrewfrench approved these changes Aug 13, 2018

View reviewed changes

rfay force-pushed the patch-1 branch from e1d04bd to bf082a2 Compare August 13, 2018 23:27

Fix example path to drush.settings.php

59902c0

JeremySkinner and others added 8 commits August 13, 2018 18:50

Update template to generate valid path to drush.settings.php

29a12f7

Update documentation on using local drush settings file

38aa19a

Update path to sites/default in documentation

b3fc35e

Make ddev_drush_settings.php automatically generated and included, ad…

2d455b2

…d test

Fix mistaken assert.EqualValues() call

d717538

Update inputs-digest in Gopkg.lock

6dcd1fe

Move app.CreateSettingsFile to where it always gets executed in ddev …

312aede

…config, piggy-back gitignore creation on that

Fix error message in CreateGitIgnore, was wired to one usage

b2a031d

rfay force-pushed the patch-1 branch from bf082a2 to b2a031d Compare August 14, 2018 00:51

rfay merged commit a6171c0 into ddev:master Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ddev_drush_settings.php actually work on host, fixes #454 #1036

Make ddev_drush_settings.php actually work on host, fixes #454 #1036

JeremySkinner commented Aug 7, 2018 •

edited by rfay

CLAassistant commented Aug 7, 2018 •

edited

rfay left a comment

JeremySkinner commented Aug 7, 2018

rfay commented Aug 7, 2018

JeremySkinner commented Aug 7, 2018

rfay commented Aug 7, 2018

JeremySkinner commented Aug 7, 2018

rfay left a comment

rfay Aug 8, 2018

JeremySkinner Aug 8, 2018 •

edited

rfay Aug 8, 2018

JeremySkinner Aug 8, 2018 •

edited

JeremySkinner commented Aug 8, 2018 •

edited

rfay left a comment •

edited

JeremySkinner commented Aug 10, 2018

rfay commented Aug 10, 2018

JeremySkinner commented Aug 10, 2018

andrewfrench left a comment •

edited

rfay commented Aug 13, 2018

andrewfrench Aug 13, 2018 •

edited

rfay Aug 13, 2018

andrewfrench Aug 13, 2018

andrewfrench left a comment

rfay commented Aug 14, 2018

Make ddev_drush_settings.php actually work on host, fixes #454 #1036

Make ddev_drush_settings.php actually work on host, fixes #454 #1036

Conversation

JeremySkinner commented Aug 7, 2018 • edited by rfay

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

CLAassistant commented Aug 7, 2018 • edited

rfay left a comment

Choose a reason for hiding this comment

JeremySkinner commented Aug 7, 2018

rfay commented Aug 7, 2018

JeremySkinner commented Aug 7, 2018

rfay commented Aug 7, 2018

JeremySkinner commented Aug 7, 2018

rfay left a comment

Choose a reason for hiding this comment

rfay Aug 8, 2018

Choose a reason for hiding this comment

JeremySkinner Aug 8, 2018 • edited

Choose a reason for hiding this comment

rfay Aug 8, 2018

Choose a reason for hiding this comment

JeremySkinner Aug 8, 2018 • edited

Choose a reason for hiding this comment

JeremySkinner commented Aug 8, 2018 • edited

rfay left a comment • edited

Choose a reason for hiding this comment

JeremySkinner commented Aug 10, 2018

rfay commented Aug 10, 2018

JeremySkinner commented Aug 10, 2018

andrewfrench left a comment • edited

Choose a reason for hiding this comment

rfay commented Aug 13, 2018

andrewfrench Aug 13, 2018 • edited

Choose a reason for hiding this comment

rfay Aug 13, 2018

Choose a reason for hiding this comment

andrewfrench Aug 13, 2018

Choose a reason for hiding this comment

andrewfrench left a comment

Choose a reason for hiding this comment

rfay commented Aug 14, 2018

JeremySkinner commented Aug 7, 2018 •

edited by rfay

CLAassistant commented Aug 7, 2018 •

edited

JeremySkinner Aug 8, 2018 •

edited

JeremySkinner Aug 8, 2018 •

edited

JeremySkinner commented Aug 8, 2018 •

edited

rfay left a comment •

edited

andrewfrench left a comment •

edited

andrewfrench Aug 13, 2018 •

edited