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

ACSF support for deploy step #18

Merged
merged 6 commits into from
May 5, 2016
Merged

ACSF support for deploy step #18

merged 6 commits into from
May 5, 2016

Conversation

damontgomery
Copy link
Contributor

  • Add a flag in project.yml
  • Add a deploy task that primarily copies settings.php needed for ACSF

We cannot re-use setup:acsf:init because it explicitly excludes the settings.php file for ACSF. It also runs against the local site, which doesn't work for deploy.

This allows us to use the default bolt setup for local dev and build an artifact that works on ACSF.

@danepowell
Copy link
Contributor

Why would we configure settings.php on every deployment, instead of just configuring it once and committing the "correct" settings.php to the source repo?

I just worry that this is going to cause confusion, since it's effectively fragmenting the source code and deploy artifact... for instance it will overwrite any custom changes you make to ACSF files in the source repo on deploy (such as .htaccess).

@danepowell
Copy link
Contributor

I just spoke with Rok, this really shouldn't be necessary at all.

If you run bolt acsf:init, this will prep your repo for ACSF deployment. The only thing it doesn't do is replace settings.php with the stock ACSF settings.php, but that's okay, because the ACSF module is patched (via composer.json) to ignore validation of settings.php:
https://github.com/acquia/bolt/blob/8.x/template/composer.json#L59

When you deploy to ACSF, it runs the version of acsf-init-verify that is in the application codebase, so it will happily ignore the hacked settings.php.

Use drush as a dev dependency only.
This resolves issues with ACSF.
@damontgomery
Copy link
Contributor Author

damontgomery commented May 4, 2016

I don't think we should be modifying ACSF files. The work in this PR supports a local default/settings.php and follows the ACSF guidelines of not touching their code.

ACSF manages settings.php and changes should be done with factory hooks.

The deploy tasks will re-run ACSF init and copy the necessary files over and everyone is happy.

Deploying to multiple hosting environments is an edge case and could be achieved with a separate custom deploy task that does not overwrite settings.php. These are supported by bolt as custom targets.

Additionally, this update requires fewer patches and fewer exceptions to the rule.

@@ -99,11 +106,13 @@

<!-- Copy required files from docroot. -->
<copy todir="${deploy.dir}/docroot" overwrite="true">
<fileset dir="${docroot}">
<fileset dir="${docroot}" defaultexcludes="false">
Copy link
Contributor

Choose a reason for hiding this comment

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

Which necessary files does setting defaultexcludes="false" cause to be included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACSF .gitignore, see https://www.phing.info/docs/guide/trunk/FileSet.html

Included files are

Default excludes are: *~, #*#, .#*, %*%, CVS, CVS/**, .cvsignore, SCCS, SCCS/**, vssver.scc, .svn, .svn/**, ._*, .DS_Store, .darcs, .darcs/**, .git, .git/**, .gitattributes, .gitignore, .gitmodules

The cleanup task already removes .git directories.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than disabling all of these default excludes, I think we should simply include .gitignore files. E.g., <include name="**/.gitignore"/>. This seems like a more precise way to accomplish the end goal.

Copy link
Contributor Author

@damontgomery damontgomery May 4, 2016

Choose a reason for hiding this comment

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

I tried that and it didn't work for me. The other option is to check for ACSF and explicitly include it's .gitignore.

I tried a general rule and I even tried another copy command just for that folder and included the file name directly with no luck.

I didn't see any reason we should be ignoring all those files and most do not apply.

@kylebrowning
Copy link
Contributor

Also this begs the question.

(And not something that will be normal)

What If I want to deploy to both ACSF and ACE?

included `-r ${deploy.dir}/docroot`
@damontgomery
Copy link
Contributor Author

@kylebrowning, if you want to deploy to multiple environments, you can use Bolt as an example and make your custom deployment tasks in Phing.

If it becomes more common, let's add it. If not, I think we try to address the more common situations.

@kylebrowning
Copy link
Contributor

This fully works and is required to get deployment artifacts up to ACSF.

It still allows local development to be unaffected and if settings.php things are needed we can use pre-settings-php and post-settings-php as described here https://docs.acquia.com/site-factory/tiers/paas/workflow/hooks

@damontgomery
Copy link
Contributor Author

damontgomery commented May 4, 2016

I'm going to make a comment about factory hooks in the readme... One second.

DONE


## Factory Hooks and settings.php

Acquia Cloud Site Factory does not allow changes to `settings.php`. In order to make additions normally done with `settings.php`, factory hooks are used.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we may want to include example scripts for pre-settings-php and post-settings-php. I'd be interested in how Bolt generated projects on ACSF are using these. I'd imagine at least a script with a require statement for cache.settings.php.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #21.

I don't think that is necessary for this PR.

@grasmash
Copy link
Contributor

grasmash commented May 4, 2016

As @danepowell said, we shouldn't need to fragment our settings.php configuration in this way. But, it seems that we do need to--it's a constraint imposed by the architecture of ACSF. That may change in the future, but until it does, it seems that we should work within the constraints rather than maintaining a strategy that isn't supported by the ACSF module.

@kylebrowning
Copy link
Contributor

Im usually of the opinion that Acquia stuff shouldn't be including in this, but were clearly not stating that.

The fact that this is all buried under a hosting variable means many will never use it, unless we add more features to the project.yml hosting variable.

@grasmash grasmash merged commit c43430c into acquia:8.x May 5, 2016
@@ -28,6 +27,7 @@
"behat/mink-goutte-driver": "*",
"behat/mink-selenium2-driver": "*",
"behat/mink-browserkit-driver": "*",
"drush/drush": "dev-master",
Copy link
Contributor

@danepowell danepowell May 5, 2016

Choose a reason for hiding this comment

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

I think Moshe has said repeatedly that applications should provide their own version of Drush (instead of using Cloud Drush)... it seems like we're going back on that here. If so, we should discuss whether that's still best practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I hadn't considered that. Will running drush @alias status use the drush bin in the remote application's vendor dir?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think until ACE is fixed with 1.91 release, we should leave this here so peoples installs for D8 do not break like they did for us.

Once its released, we can move it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

#34

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.

None yet

4 participants