Open gilzow opened 2 years ago
It would be good to remove this stuff from people who use a naive workflow. 👍
The logic is good, and the problem it solves is sensible. But I have bad feelings about this amount of script embedded in yaml files. It's suddenly become twice as complex for casual developers to parse, and easier to break things badly with yaml+bash quoting and escaping issues. [opinion]Code embedded in yaml configs should be one-liners, not structured logic & syntax[/opinion]
I propose that this would be a suitable time to squash extract this logic into ./drush/platformsh_deploy_drupal.sh
and run this little sequence that way. It's partly just aesthetic/code-style, but I think that the cleaner the yamls can be kept, the more sane they will be to support. Easier to comment out for "naive workflow" types also.
counterpoint: It DOES lower the in-your-face visibility of what we do with the drush cache:clear
every time
... but I think the complexity (yaml enwrapping bash enwrapping drush enwrapping php eval) warrants being split out.
I'm not married to the idea of having it in the yaml file, and share your concerns @dman-coders. We can certainly move it into its own shell script and then heavily document the yaml file to indicate why we're executing the script. I'm more interested in a sanity-check me on the logic I was suggesting. So if you're in agreement that the logic is good, I'll work on a proper PR where it's been moved over into a separate shell script.
Logic is basically good, yes, though I have seen projects that will find ways to break it.
The first common use case that this will have to be extended for is multisites. Our documented support for the drush commands in multisites is already lacking (as in, our current multisite template does not even work) but it's been easy enough to tweak until now.
In the easy cases, with only 2 or 3 multisites, I expect a developer to basically go :
drush --uri=site1 cache-rebuild
drush --uri=site1 updatedb
drush --uri=site1 config-import
drush --uri=site2 cache-rebuild
drush --uri=site2 updatedb
drush --uri=site2 config-import
Repetitive, but straightforwards.
To this end, I would add a preamble to the proposed shell script to allow it to take an arg:
if [ ! -z "$1" ]; then
DRUSH_OPTIONS_URI="$1"
fi
So that URI definitions can be optionally invoked with :
./drush/platformsh_deploy_drupal.sh site1
./drush/platformsh_deploy_drupal.sh site2
There are other non-multisite reasons for defining the URI also . Sometimes projects - especially ones migrated from Acquia or other architectures - have been set up to define the site explicitly - it's not always default
. You'd expect this to be rare, but I've seen in in close to 10% of the enterprisey sites I've audited on platform.sh, so I think it's worth including at this time.
to get a list of the sites in a multisite, would the best method be to grab all the names of the subfolders in DRUPAL_ROOT/sites/*? For non multisites, I would assume we could grab the primary=true
entry in routes?
Then we could just loop through the list so the user doesnt have to create an entry for each one.
Been there, done that.
./drush/drush-multisite
#!/bin/bash
# Run a given drush command over all multisites.
# Substitute for the old `drush @all` command.
# Assumes you are running from the context of docroot.
for dir in sites/*/; do
if [ -f "$dir/settings.php" ]; then
subsite=$(echo "$dir" | cut -f 2 -d '/') ;
drush --uri="${subsite}" "$@"
fi
done
I turns out that half the sites with > 3 subsites also can get a bit "unique" with their preferences, so the above script that tries to follow that simple heuristic - works half the time. Other times you find it atempting to bootstrap drush on unexpected instances like sites/localdev.otto
as well.
But yes, it's a good start, or at least an example for the docs that I've been sharing with folk as they need it.
I turns out that half the sites with > 3 subsites also can get a bit "unique" with their preferences, so the above script that tries to follow that simple heuristic - works half the time. Other times you find it atempting to bootstrap drush on unexpected instances like
sites/localdev.otto
as well.
I've dealt with that before for WordPress multisites. We can always grab the list of valid domains from routes (ie production_url
) and then only use the ones that match. Let me work up a PoC and submit as a PR
Don't forget that half of the Drupal multisites out there are sub-dir-based, not subdomain based - meaning they won't show up in routes
:grin:
i think for now I'm going to build it for single site (with an eye towards multisite) since we already have multisites as a separate template.
PR created. As mentioned it doesnt include passing in the uri as I need to see an example of a single-instance site that isnt set up using default, and then some examples of multisite where things are "weird" in order to formulate a strategy to address those instances.
Describe the bug
Line 87 of .platform.app.yaml includes a
drush config-import
command in the deploy hook. If the project doesn't actively use configuration management, then the config import will fail with the error included below in the logs section.Include some logs
Reproducing
Do not use configuration management, push change to a project environment.
Expected behavior
Only run
config-import
if the project is using configuration management.Your environment
Default Drupal9 template
Screenshots
N/A
Additional context
Note, these changes are identical to #114 as they are related to one another. Suggest changing deploy to something like the following:
We get the config directory from drush, concatenate it with
/*.yml
and use that withls
. If there are config files, then the return fromls
will not be empty so we can continue withconfig-import
.There was some concern mentioned about settings not being available. According to the docs for
Settings::get
In the absence of a setting, according to the docs, the default is
if it were to return that default, the call to realpath on a non-existent path will return a
false
which when echo’ed will be an empty string. Returned back to thels
command, we’rels
ing on/
which doesnt include any yaml files.If needed we could break it out further to exit processing if realpath returns an empty string.