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.

Sign up for GitHub

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

Jump to bottom

Multisite setup enhancements and bugfixes #2997

Merged
merged 15 commits into from
Aug 7, 2018
Merged

Multisite setup enhancements and bugfixes #2997

merged 15 commits into from
Aug 7, 2018

Conversation

lcatlett
Copy link
Contributor

@lcatlett lcatlett commented Aug 6, 2018 •
edited
Loading

Fixes #2907, #2996, #2814, #2930

Changes proposed:

  • Validate mysql connection with output of drush status rather than drush sqlq for blt internal:drupal:install and blt drupal:install
  • Create database with drush credentials if it does not exist
  • Grants needed mysql user / db permissions for all multisite database names for all hosts including localhost
  • Remove drupal_* namespace restriction for mysql database name grants
  • Create sites.php file if it does not exist
  • Generate sites.php sites array mapping project multisite local uris to site directories
  • Remove default local.* config/settings files from new multisite directory to regenerate db and drush config from input
  • Fix multisite local drush alias config
  • Populate multsite database credentials from blt and user input
  • Simplify use of $acsf_site_name in multisite setup
  • Exclude default site files from multisite directory
  • Update README

Testing steps

Generate multisite site files, vm config, and settings.php config

  1. run blt multisite. Validate that the task:
  • creates new multisite in docroot/sites/[sitename]
  • generates vm host in box/config.yml
  • generates vm db config in box.config.yml
  • generates local.settings.php and local.drush.yml with new multisite config
  • generates sites.php with new multisite mapping local site hostname to site directory:
  • Excludes files directory from default site
  • Creates drush alias in drush/sites/[sitename].site.yml
  • drush alias returns correct uri, site directory, and db credentials
  1. Re-provision vm to add new multisite host and database
  • Exit from vm and reprovision with vagrant reload --provision
  • ssh to vm: vagrant ssh
  1. Multisite install/setup
  • Install drupal / set up blt: blt setup --site=[sitename]
  • Validate that database is dropped and created automatically
  • Validate that site is installed according to config in sites/[sitename].blt.yml / local.settings.php
  • Once installed, both drush @sitename.local st and drush --root=path/to/docroot --uri=hostname should return the correct site uri and site path and the database in sites/[sitename]/settings/local.settings.php should bootstrap

ba66e77
ba66e77 suggested changes Aug 6, 2018
View reviewed changes
Copy link
Contributor

@ba66e77 ba66e77 left a comment

Choose a reason for hiding this comment

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

In testing, I also found an unrelated issue with the way users are setup in the box/config.yml https://github.com/acquia/blt/blob/9.x/src/Robo/Commands/Generate/MultisiteCommand.php#L105. That line really should 'priv' => $newDBSettings['database'] . '.*:ALL', (notice the . in front of the *:ALL)

src/Robo/Commands/Setup/SettingsCommand.php Outdated
$site_local_hostname = $this->getConfigValue('project.local.hostname');
$sites[$site_local_hostname] = $multisite;
$contents = "<?php\n \$sites = " . var_export($sites, TRUE) . ";";
file_put_contents($this->getConfigValue('docroot') . "/sites/sites.php", $contents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this in the foreach loop results in the file being written repeatedly. Since the $sites variable is accreting keys in the loop, could writing the file be moved out of the loop and done a single time at the end?

src/Robo/Commands/Setup/SettingsCommand.php Outdated
@@ -80,6 +80,12 @@ public function generateSiteConfigFiles() {
$default_local_drush_file = "$multisite_dir/default.local.drush.yml";
$project_local_drush_file = "$multisite_dir/local.drush.yml";

// Generate sites.php for local multisite.
$site_local_hostname = $this->getConfigValue('project.local.hostname');
$sites[$site_local_hostname] = $multisite;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's pedantic of me, but I'd like to see the $sites variable declared above the start of the loop so that the code is a bit more readable.

tests/phpunit/BltProject/MultiSiteTest.php Outdated
@@ -42,7 +42,7 @@ public function testMultisiteGenerate() {
$this->assertEquals("$this->site1Dir.clone", $site1_blt_yml['drush']['aliases']['remote']);

$site2_blt_yml = YamlMunge::parseFile("$this->sandboxInstance/docroot/sites/$this->site2Dir/blt.yml");
$this->assertEquals("$this->site2Dir.local", $site2_blt_yml['drush']['aliases']['local']);
$this->assertEquals("self", $site1_blt_yml['drush']['aliases']['local']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be using $site2_blt_yml, shouldn't it?

@ba66e77 ba66e77 mentioned this pull request Aug 7, 2018
Merged
ba66e77 and others added 2 commits August 7, 2018 07:46
@ba66e77
Merge branch '9.x' into multisite-fixes
621d2ac
@lcatlett
Merge pull request #7 from ba66e77/multisite-fixes
d6baead 
Fixing a typo in privileges that caused the new user to not have expe…
@ba66e77 ba66e77 merged commit d0da339 into acquia:9.x Aug 7, 2018
This was referenced Aug 7, 2018
Closed
Closed
This was referenced Aug 24, 2018
Closed
Closed
danepowell added a commit that referenced this pull request Aug 24, 2018
@danepowell
Revert "Multisite setup enhancements and bugfixes (#2997)"
cf9eeca 
This reverts commit d0da339.
@danepowell danepowell mentioned this pull request Aug 24, 2018
Merged
danepowell added a commit that referenced this pull request Aug 24, 2018
@danepowell
Revert "Multisite setup enhancements and bugfixes" (#3028)
73af46a 
* Revert "Updating CHANGELOG.md and setting version for 9.1.2."

This reverts commit 5e52325.

* Revert "Minor code review docs update (#3013)"

This reverts commit 25e3887.

* Revert "SAML Config Refactor (#2953)"

This reverts commit 312d471.

* Revert "Improve template/README.md (#3004)"

This reverts commit 316967d.

* Revert "Multisite setup enhancements and bugfixes (#2997)"

This reverts commit d0da339.
@danepowell danepowell mentioned this pull request Aug 24, 2018
Closed
This was referenced Aug 25, 2018
Closed
Closed
Closed
@lcatlett lcatlett mentioned this pull request Oct 10, 2018
Closed
@lcatlett lcatlett mentioned this pull request Dec 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ba66e77 ba66e77 ba66e77 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lcatlett @ba66e77