Skip to content

fix: Install drush without affecting git index #33

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

Closed
wants to merge 8 commits into from

Conversation

rfay
Copy link
Contributor

@rfay rfay commented May 1, 2024

This PR installs drush (on demand) so it can be widely used. This is also a first step toward being able to install database types other than sqlite3

  • Script install_drush.sh saves away current git index state, installs drush, then rolls back to original image state
  • Custom "drush" command checks to see if drush is available, and if not, runs install_drush.sh

Testing:

Test with

ddev get https://github.com/rfay/ddev-drupal-core-dev/tarball/20240501_drush
  • Start a project
  • Use ddev drush st to see it get installed
  • Then ddev drush sql-cli and ddev drush si -y demo_umami --account-pass=admin
  • git status should show no changes of any kind.

Automated tests

Coming

Copy link

@penyaskito penyaskito left a comment

Choose a reason for hiding this comment

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

As someone who uses the stash a lot I'm worried this might affect people workflows, so let's try to leave the stash list as it started.

@simesy
Copy link
Contributor

simesy commented May 2, 2024

I tested this and it works well.

I like that it clearly doesn't change the installation unless you run ddev drush.....

I was also keen see that it did not change the container or service definition of the application. I dumped $container_definition, service IDs and then compared these before and after and I could not see any difference between them, so my conclusion is that adding drush does not change the drupal application state.

@rfay
Copy link
Contributor Author

rfay commented May 2, 2024

Thanks for the review. This just introduces drush and makes it available. It doesn't change anything about the code. When we introduce the use of different database types and such we can explain how to use ddev drush more fully.

@rfay rfay force-pushed the 20240501_drush branch from 8aa6423 to cdc627c Compare May 23, 2024 19:10
@rfay
Copy link
Contributor Author

rfay commented May 26, 2024

@justafish this has now been rebased and is passing tests and has tests for the drush usage. I think everybody will like it :) Hoping you can pull it, and we can move on to using other database types in

@rfay
Copy link
Contributor Author

rfay commented Jun 17, 2024

@justafish I'd love if you could take a look at this (and

I'm hoping to do some performance comparisons using tmpfs/memory-based database mounts (mariadb) and it will be easy and fun to do with these in.

@justafish
Copy link
Owner

Hi @rfay - thanks for this PR. This is a tricky one for me - I think when we initially discussed this it was to do it in a way that would put everything back to how it was before running the command. However, for me that includes the vendor directory otherwise I'm developing against an unknown set of dependencies. For me it's absolutely fine to have visible changes in composer.json/composer.lock when I've installed an outside dependency like this - I actively want to know that what I'm working on isn't the same anymore as there's no way to distinguish between this situation and when you're purposefully adding/upgrade/removing packages.

So closing this PR for now, but please do re-open if there's any ideas for creative solutions that don't break the workflow I described.

@justafish justafish closed this Oct 7, 2024
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.

5 participants