Adding support for config_split. #1102
Conversation
phing/tasks/setup.xml
Outdated
|
|
||
| <target name="setup:config-import:config-split"> | ||
| <drush command="en" assume="yes" alias="${drush.alias}" passthru="false"> | ||
| <param>config-split</param> |
There was a problem hiding this comment.
This command doesn't seem like it would work...?
There was a problem hiding this comment.
Oh yeah, need an underscore rather than a hyphen.
326173d to
68ddbb8
Compare
|
See also: #1138 |
|
I'm working to clean up documentation for this here, I'll open a PR when it's ready: Note that this replaces the features-workflow doc. |
phing/tasks/setup.xml
Outdated
| <drush command="config-import" assume="yes" alias="${drush.alias}" passthru="false"> | ||
| <option name="partial"></option> | ||
| <param>${cm.core.config-dir.key}</param> | ||
| <mkdir dir="${cm.core.path}/${env.AH_SITE_ENVIRONMENT}"/> |
There was a problem hiding this comment.
Can you do this in a Cloud environment? Wouldn't the filesystem at ${cm.core.path} be read-only?
There was a problem hiding this comment.
Yeah I guess we can't do it in a cloud env. Really, the user should be creating this directory manually. Not sure what happens when this doesn't exist. This may be problematic for ODEs... perhaps we should just skip import of the dir doesn't exist? @geerlingguy thoughts?
There was a problem hiding this comment.
perhaps we should just skip import of the dir doesn't exist?
I like that idea.
|
We should bump the Drush version to |
|
This looks good to me except for my comments above. |
|
This PR is currently failing because we need some example config_split config to use during CI testing. |
phing/tasks/setup.xml
Outdated
| <drush command="config-split-import" assume="yes" alias="${drush.alias}"> | ||
| <!-- Cloud environments require vcs parameter. --> | ||
| <param>vcs</param> | ||
| <option name="split">${environment}</option> |
There was a problem hiding this comment.
Note—the 'split' is now a param and not an option—so it should be:
<param>${environment}</param>
There was a problem hiding this comment.
You actually shouldn't even put ${environment} as an option. The environment needs to be specified in settings.php
See
@geerlingguy @grasmash See a PR where I got the new version of config_split/config_filter working with Canary. Specifically:
https://github.com/acquia-pso/canary/pull/25/files#diff-e56cbd5daab22087fd0e4849dc38055dR14
https://github.com/acquia-pso/canary/pull/25/files#diff-2737999ec8b2368b193b40b46c5679c2R758
There was a problem hiding this comment.
@arknoll Does it have to be done that way? I'd much rather use a param than write to settings.php.
There was a problem hiding this comment.
It does appear that you have to use the settings.php approach.
Based on the maintainers statement it appears it was by design:
"I think it is better to simplify the options of the command so that it is easier for everybody to use and understand. In my opinion and my experience it is better and safer to define which splits should be imported and exported in settings.php rather than scripts." -- https://www.drupal.org/node/2861266#comment-11990329
phing/tasks/setup.xml
Outdated
| <drush command="config-import" assume="yes" alias="${drush.alias}" passthru="false"> | ||
| <param>${cm.core.config-dir.key}</param> | ||
| <drush command="config-split-import" assume="yes" alias="${drush.alias}"> | ||
| <option name="split">${environment}</option> |
There was a problem hiding this comment.
Note—the 'split' is now a param and not an option—so it should be:
<param>${environment}</param>
|
@grasmash - Note also, it looks like beta4 might have a bug where it doesn't combine the split with |
|
@geerlingguy @grasmash See a PR where I got the new version of config_split/config_filter working with Canary. Specifically: https://github.com/acquia-pso/canary/pull/25/files#diff-e56cbd5daab22087fd0e4849dc38055dR14 https://github.com/acquia-pso/canary/pull/25/files#diff-2737999ec8b2368b193b40b46c5679c2R758 |
|
Yeah it looks like it's not really a bug in beta4, just a change in how config_split works. It now relies on overriding the configuration via settings.php (like Alex did for Canary) rather than passing the command line argument. |
f88a46c to
86f7cb9
Compare
05bf66a to
ec1551f
Compare
| $config['config_split.config_split.dev']['status'] = FALSE; | ||
| $config['config_split.config_split.stage']['status'] = FALSE; | ||
| $config['config_split.config_split.prod']['status'] = FALSE; | ||
| $config['config_split.config_split.ci']['status'] = FALSE; |
There was a problem hiding this comment.
@grasmash - There are no extra settings for RA (many subscriptions have an ra environment as well). Do you think that might be something to add? Or just note that people should add an ra-specific split in their own settings.php if they need it?
One other note—for my sites I'm currently using a test split (not stage) to match the fact that Acquia Cloud still uses test as the machine name for the Stage environment... for those projects who are already set up using this kind of naming convention, I'll try to document how to switch the split name correctly. Just wanted to put that out there.
There was a problem hiding this comment.
I'm also using test (mainly because that's what canary does... I haven't tested to see how it works yet).
Besides RA, a project could have any number of custom environments (typically dev1, dev2, etc... but they could be named anything). I think it would be awesome if we could create this configuration automatically, or as part of a wizard (i.e. "here are the typical environments, do you have any others?"). Short of that, just documenting it would be good.
There was a problem hiding this comment.
@danepowell - Also, ODE could throw a wrinkle depending on how you structure your config... it might be nice to say "any non-prod environment gets split xyz"...
There was a problem hiding this comment.
Also see my notes-in-progress here: #965 (comment)
No description provided.