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

Add drush launcher to bin, with drush 8 fallback. #1183

Merged
merged 11 commits into from Sep 20, 2019

Conversation

@simesy
Copy link
Contributor

commented Aug 15, 2019

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated.
  • Changelog entry has been written

Drush recommends being installed as part of a site, so that a site's composer.json can specify which version of Drush is compatible. Drush launcher is the correct solution: a unified command that dynamically finds and calls Drush. It allows a fallback to the existing global drush.

This PR removes the ~/.composer/... directory from the PATH. It appears that it was only added there to allow drush to be executed and is redundant now.

The current docs only have a reference to Drush 9, no reference to Drush 8. Not sure exactly what the docs should have, so I just added a note about drush 8 in the drush9.md.

Changelog Entry

Drush launcher is now available. If you have drush available in a Drupal site, and you run drush from /app or the web root, Drush Launcher tries to run the local drush (usually vendor/bin/drush), before falling back to the global Drush. If you need to pin your Drush to the global drush, you should add /home/.composer/vendor/bin/drush to the start of PATH environment variable, or call this directly in your scripts.

Closes #1069

@simesy simesy changed the title Add drush launcher to bin, with drush 8 fallback, remove composer dir. Add drush launcher to bin, with drush 8 fallback. Aug 15, 2019
@Schnitzel

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

I like the approach here! Our current plan was that the global installed Drush8 already does the search for the site-specific Drush and if not it falls back to Drush8. But this solution is definitely more elegant and probably better for the future.

I'm not sure though if we can really remove the /home/.composer/vendor/bin from $PATH.
If anybody else that is using the Docker Images is installing something globally with composer, that person probably relies on the fact that the PATH contains the path to the executable that was installed globally. Unfortunately updating the $PATH would break functionality for these people and probably would be quite hard for them to figure out why this is.
So therefore I think we should leave the $PATH in there, even though the Image itself doesn't rely on it.

Also we just updated Drush versions: #1185, so we it's conflicting now, sorry.

@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Sounds good. There is one thing to change regarding the PATH, we'd need to move the composer reference to the end of PATH. Currently ~/.composer/vendor/bin is the first reference, so it has priority over /usr/local/bin. So I can modify the add_PATH script to put it at the end in cli/Dockerfile , or I can override it again completely cli-drupal/Dockerfile. What do you think? It is still a little problematic given most general advice on the internet suggests putting it at the start, eg export PATH=~/.composer/vendor/bin:$PATH

@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Alternatively we could install drush launcher to /usr/local/bin/drush-launcher, so that users can choose explicitly to use this (I think it would work fine).

@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

There are a lot of approaches to making drush 9 (explicitly) and drush launcher available. Some people just do $DRUSH="/path/to/drush" and use $DRUSH everywhere. Acquia has drush8 and drush9. I'm happy to implement your preference that takes into account what "updating to drush 9" on Lagoon looks like.

@Schnitzel

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Currently ~/.composer/vendor/bin is the first reference, so it has priority over /usr/local/bin. So I can modify the add_PATH script to put it at the end in cli/Dockerfile

Mhh are you sure? My understanding is that executables in the home directly should have preference over the global operating system level ones, or you could never overwrite the operation system level ones with local ones (like if you need another mysqldump version or so)

Some people just do $DRUSH="/path/to/drush" and use $DRUSH everywhere. Acquia has drush8 and drush9.

The problem is that we already started supporting drush and that's used by all the thousand of Lagoon Projects out there. I think we should do:

  1. Install the drush launcher at /usr/local/bin/drush
  2. Install Drush 8 not global anymore, but in another place inside the HOME directory
  3. point DRUSH_LAUNCHER_FALLBACK to wherever Drush 8 is installed
  4. Leave ~/.composer/vendor/bin in the path at the beginning, but nothing is existing in there anymore by default.

With this:

  • Developers don't need to change anything if they are okay with Drush8, plus projects that don't have a site specific drush installed still get a version of drush (the reason that we can't remove drush all together)
  • Developers that need a specific drush version just define it in composer.json and the drush launcher will find it.
  • We will upgrade the default drush8 to drush9 as soon as drush8 is EOL
@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Correct /home/.composer/vendor/bin is at the front of PATH, giving it priority over everything else. But that just defeats the point of drush launcher.

I like the plan you put forward I will follow up.

@simesy simesy referenced this pull request Aug 20, 2019
@Schnitzel Schnitzel added this to the v1.1.0 milestone Aug 28, 2019
@simesy simesy force-pushed the simesy:issue-1069 branch from acb3e2b to b80307a Sep 5, 2019
@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

I believe this is good to go. In the built image by default...

cli-drupal:/app$ drush --version
Drush Launcher Version: 0.6.0
 Drush Version   :  8.3.0
@simesy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

I've not been able to run the tests locally. But I take it I need to make this pass.

TASK [DRUSH - running drush -y sql-sync @drush-second @self on ci-drupal-drush-first@ssh on port 2020, searching for ''] ***

But if drush is still drush 8 by default i'm not sure why it would break - unless the Drupal site has drush 9 and this is running in the Drupal site ... so we'd expect the Drush 8 aliases to fail in a best practice situation and we need a Drush 9 alias equivalent in-place - whereever in-place is.

I see the tests eg testing the alias is there but I don't see a drush aliases file - would it be using the drush alias service?

@Schnitzel

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

so I think that error might has to do with an old drush version shipped inside the test drupal, I've updated all dependencies of that drupal, let's see

Schnitzel added 2 commits Sep 16, 2019
@Schnitzel Schnitzel merged commit 53d585f into amazeeio:master Sep 20, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.