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

Remove the drush config file. #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abhisekmazumdar
Copy link

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
Copy link
Owner

phenaproxima commented May 13, 2024

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Owner

phenaproxima commented May 13, 2024

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
Copy link

rfay commented May 13, 2024

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
Copy link
Author

abhisekmazumdar commented May 13, 2024

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
Copy link
Author

abhisekmazumdar commented May 14, 2024

As mentioned by @phenaproxima above on #68 (comment)

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: drush-ops/drush#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

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

3 participants