phenaproxima / starshot-prototype

Prototype of a new kind of Drupal, based on recipes and loaded up with contrib's best modules and themes. Not a fork or a distribution.
https://drupal.org/starshot
106 stars 36 forks source link

Remove the `drush` config file. #68

Closed abhisekmazumdar closed 2 weeks ago

abhisekmazumdar commented 1 month ago

I feel we should remove the drush configuration file, as it is overwriting the DRUSH_OPTIONS_URI setting when running ddev drush uli.

Which outputs: http://localhost/user/reset/1/1715615952/V4pMvp5c1aWQEmBgFFGbdZHeQno1xkPWy_nU7n1FAzM/login

Removing the Drush configuration file will ensure that the DRUSH_OPTIONS_URI value is used when running drush uli.

After removing: https://starshot.ddev.site/user/reset/1/1715616070/dPXXBM1fnUh6Nr17Snep7G1SH1hgZViuWiEyPg3El0E/login

phenaproxima commented 1 month ago

Actually the environment variable is supposed to override drush.yml. The fact that it doesn't is a bug in Drush, which was fixed in 13.x, but not in 12.x (which is what we use).

That being said, I'm not sure we will really need the drush.yml file much longer, if at all. The main reason why it's there is to ensure that Drush commands work outside of Docker containers.

So what might be better here is to adjust the .ddev/commands/host/install command, and/or any related DDEV configuration, such that it deletes (or suppresses) drush.yml inside the container, but leaves it alone on the host machine.

abhisekmazumdar commented 1 month ago

Oh, yes. Sometimes I forget there is a world outside ddev.

I will try to find a possible solution to make changes just for ddev.

abhisekmazumdar commented 1 month ago

@phenaproxima

I suggest adding rm -f ./drush/drush.yml to the install command. However, this will result in a Git change. Do you think thats a good idea ?

phenaproxima commented 1 month ago

I would probably not merge that, precisely because it will affect things outside the container.

I would maybe see if there's a way DDEV can be configured to exclude the drush directory from being shared with the container.

rfay commented 1 month ago

I think this is probably a drush issue and not a DDEV issue.

Best to solve in drush.

Discord discussion: https://discord.com/channels/664580571770388500/1239629371279671316

You might be able to bind-mount an empty directory on top of the .drush directory using something like https://stackoverflow.com/questions/57426306/ddev-mount-additional-folders-for-shared-composer-packages/57432155#57432155

abhisekmazumdar commented 1 month ago

Thank you @rfay for your inputs.

That's what I was thinking when I looked at it: https://stackoverflow.com/questions/29181032/add-a-volume-to-docker-but-exclude-a-sub-folder

Okay, I will brainstorm this a bit more.

abhisekmazumdar commented 1 month ago

As mentioned by @phenaproxima above on https://github.com/phenaproxima/starshot-prototype/pull/68#issuecomment-2108124090

Actually, the environment variable is supposed to override drush.yml. The fact that it doesn't is a bug in Drush, which was fixed in 13.x, but not in 12.x (which is what we use).

I found the PR: https://github.com/drush-ops/drush/pull/5961 And the Drupal #drush slack discussion: https://drupal.slack.com/archives/C62H9CWQM/p1713918009718199?thread_ts=1713885632.318469&cid=C62H9CWQM

I'm not opening an issue on Drush as it has been fixed in the beta version of Drush. Although I was not able to validate this on my end.

That being said, I feel we can either live with this issue for now or follow what @rfay has suggested

You might be able to bind-mount an empty directory on top of the .drush directory using something like https://stackoverflow.com/questions/57426306/ddev-mount-additional-folders-for-shared-composer-packages/57432155#57432155

phenaproxima commented 2 weeks ago

I think #108 fixed this; it configures Mutagen to prevent drush.yml from being synced into the web container, and makes it explicit that the file is only intended to take effect on the host machine.