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

Add support for storing drupal vm in a subdirectory #378

Merged
merged 8 commits into from May 18, 2016

Conversation

@oxyc
Copy link
Collaborator

oxyc commented Jan 15, 2016

This is a work in progress and currently (probably) doesn't work with:

  • ansible_local
  • correct post_provisioning_scripts paths

I'm adding my initial proof of concept described in #305 (comment) as well as some docs on how to set it up.

Would be great to get some feedback and suggestions, so I'm opening this PR for more discussion. Personally I really like this setup as it's fairly simple, doesn't add too much code and allows me to store Drupal VM as a git submodule.

In the discussion I linked to @frob discovered one drawback though. You need to provision the machine in the project root, as the .vagrant directory needs to be created in the same location as the delegating Vagrantfile or else multiple .vagrant folders will be created. Actually now that I think about it, it's probably only an issue if you go through the delegated Vagrantfile from Drupal VM.

I honestly don't think that scenario is worth fixing... But it could probably be prevented by doing something similar to http://stackoverflow.com/a/25904132/319855

@oxyc oxyc force-pushed the oxyc:decouple branch from 86de254 to d8373f4 Jan 15, 2016
@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented Jan 15, 2016

Hmm... this is an interesting approach, for sure, and would probably jive pretty well with the approach we usually use on Acquia's projects. I'm going to kick the tires on it, would also love to hear more feedback from others from #305 as well.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 16, 2016

Due to #372 I can't test this at the moment but I think 06aa960 would make ansible_local work.

I'm also torn if we should simply require the config.yml file to be in the project root directory, or if we might want to allow users to have it in an arbitrary path, for example config/drupalvm-config.yml

@jyrkij

This comment has been minimized.

Copy link

jyrkij commented Jan 20, 2016

Regarding config file path I'd like to be able to set an arbitrary path for that. Just to minimize the number of files in project root...

@frob

This comment has been minimized.

Copy link
Contributor

frob commented Jan 22, 2016

I have not had a chance to fully test this, however I had a few thoughts.

Could we use a symlink to the drupal-vm directory? I am still not comfortable with making anything other than the drupal-vm config a part of the actual project repository.

We would have to test windows symlinks as well.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 23, 2016

@frob you mean symlinking the drupal-vm directory from outside the project root? I just tried it with rsynced folders and I only got the symlink to sync, but not the contents. I'm guessing this is something that can differ per host OS, per provider and per synced folder type. Is this supposed to be supported with drupal vm otherwise? Sounds like a nightmare to maintain without a test suite. Personally I don't really feel it's a blocker.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 23, 2016

I'll wait until I get ansible_local working again but I think ENV variables is probably a better solution than global variables. That way the delegator doesn't even need to be a Vagrantfile, it could be a shell script or a gulp task or whatever.

To support config.yml stored in an arbitrary path, we need a second variable as well. Eg.

ENV['DRUPALVM_PROJECT_ROOT'] = File.dirname(File.expand_path(__FILE__))
# Relative to the project root so it's easier to reference with `ansible_local`
ENV['DRUPALVM_CONFIG'] = "config/drupalvm-config.yml"
# Could also add a variable for where the playbook is
ENV['DRUPALVM_PLAYBOOK'] = "config/provisioning/playbook.yml"
ENV['DRUPALVM_GALAXY_ROLE_FILE'] = "config/provisioning/requirements.yml"

load "#{ENV['DRUPALVM_PROJECT_ROOT']}/box/Vagrantfile"

Then set the paths somewhere along the lines of

project_root = ENV['DRUPALVM_PROJECT_ROOT'] || dir
vagrant_root = ENV['DRUPALVM_PROJECT_ROOT'] ? Pathname.new(dir).relative_path_from(Pathname.new(project_root)) : dir
vconfig_path = ENV['DRUPALVM_CONFIG'] || "config.yml"
playbook_path = ENV['DRUPALVM_PLAYBOOK'] || "provisioning/playbook.yml"
galaxy_role_file_path = ENV['DRUPALVM_GALAXY_ROLE_FILE_PATH'] || "provisioning/requirements.yml"

...
vconfig = YAML::load_file("#{project_root}/#{vconfig_path}")
....

    # Provisioning. Use ansible if it's installed on host, ansible_local if not.
    if which('ansible-playbook')
      config.vm.provision "ansible" do |ansible|
        ansible.playbook = "#{project_root}/#{playbook_path}"
        ansible.extra_vars = {
          config_file: "#{project_root}/#{vconfig_path}"
        }
      end
    else
      config.vm.provision "ansible_local" do |ansible|
        ansible.playbook = "#{vagrant_root}/#{playbook_path}"
        ansible.galaxy_role_file = "#{vagrant_root}/#{galaxy_role_file_path}"
        ansible.extra_vars = {
          config_file: "#{vagrant_root}/#{vconfig_path}"
        }
      end
    end
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 23, 2016

Added a commit that uses environment variables instead. Not sure what's going on with the ansible_local provisioner though, it might be an upstream bug because it fails with the following message:

ERROR! Syntax Error while loading YAML.
The error appears to have been in '': line 1, column 13, but may be elsewhere in the file depending on the exact syntax problem.

while running the following command:

cd /vagrant && PYTHONUNBUFFERED=1 ANSIBLE_FORCE_COLOR=true ansible-playbook --limit='drupalvm' --inventory-file=/tmp/vagrant-ansible/inventory --extra-vars={"config_file":"/vagrant/config/config.yml"} box/provisioning/playbook.yml

Note that the --extra-vars value is not quoted and therefore throws an error. If I ssh in and modify the command to the following it works.

cd /vagrant && PYTHONUNBUFFERED=1 ANSIBLE_FORCE_COLOR=true ansible-playbook --limit='drupalvm' --inventory-file=/tmp/vagrant-ansible/inventory --extra-vars='{"config_file":"/vagrant/config/config.yml"}' box/provisioning/playbook.yml

Disregarding the issue I mentioned above, it works in the following scenarios:

  1. Current setup with config.yml in the Drupal VM root (both ansible and ansible_local provisioners)
  2. Drupal VM in a sub directory and config.yml in another subdirecotry (both ansible and ansible_local provisioners)

The paths are really messy though, I cant even follow it myself. I'm just pushing the proof of concept.

@oxyc oxyc force-pushed the oxyc:decouple branch from 4810c0e to 9b6f622 Jan 23, 2016
@frob

This comment has been minimized.

Copy link
Contributor

frob commented Jan 23, 2016

I was reffering to symlinking to the .vagrant directory in the drupalvm directory from the project directory. This would be to stop the confusing vagrantfile proxy issue.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 24, 2016

Cleaned up a bit and found the upstream issue about --extra-vars not being quoted hashicorp/vagrant#6726

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 25, 2016

@frob symlinking works with VirtualBox on OSX at least (I do not have access to any other setup).

.
├── .vagrant
│   ├── machines
│   └── provisioners
├── Vagrantfile # Delegates to box/Vagrantfile
├── box # Drupal VM checkout
│   ├── .git
│   ├── .gitignore
│   ├── .travis.yml
│   ├── .vagrant -> ../.vagrant
│   ├── LICENSE
│   ├── README.md
│   ├── Vagrantfile
│   ├── config.yml -> ../config/config.yml
│   ├── docs
│   ├── example.config.yml
│   ├── example.drupal.make.yml
│   ├── examples
│   ├── mkdocs.yml
│   └── provisioning
└── config
    └── config.yml

Steps:

pushd box
ln -s ../.vagrant .vagrant
ln -s ../config/config.yml config.yml

It works as long as you provision it in the project root the first time. Guess it works in the opposite direction too. BUT the config file now has to be inside the drupal vm directory again! No way to go around this as the path is no longer available from the delegating Vagrantfile (you could create a wrapper shell function that sets the ENV paths I guess). I think this is unnecessary to document and we could just mention in the docs that the user shouldn't run commands through box/Vagrantfile directly.

Currently I'm quite pleased with this setup. Only waiting for the vagrant issue to be solved. Hopefully it gets fixed by 1.8.2

@oxyc oxyc changed the title [WIP] add support for storing drupal vm in a subdirectory Add support for storing drupal vm in a subdirectory Jan 25, 2016
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Jan 25, 2016

@geerlingguy and anyone else, please give further input and I can make adjustments if necessary. Before merging I'll squash the commits but I'll keep them separated for now so you can see the changes I've made.

Edit: Still needs to be tested under Windows (which I dont have access to). Using /vagrant/.. for the ansible_local paths could possibly cause issues #372 (comment). If I understand the mapping described in the upstream issue correctly, it should work with 1.8.2.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Feb 18, 2016

Postponed by #450

@geerlingguy geerlingguy added the planned label Feb 21, 2016
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Feb 27, 2016

Maybe it's not worth adding support for a custom playbook. Sounds like you should be maintaining a fork if that's something which is interesting.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Feb 28, 2016

One way to avoid having to define DRUPALVM_PROJECT_ROOT, would be to upload the config file using Vagrants file provisioner when using ansible_local. This would be the entire change of the Vagrantfile (compare it to what this current PR contains, a lot easier to grasp).

diff --git a/Vagrantfile b/Vagrantfile
index b872b14..94f0d00 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -27,10 +27,11 @@ end
 # Use config.yml for basic VM configuration.
 require 'yaml'
 dir = File.dirname(File.expand_path(__FILE__))
-unless File.exist?("#{dir}/config.yml")
+config_file = ENV['DRUPALVM_CONFIG'] || "#{dir}/config.yml"
+unless File.exist?(config_file)
   raise 'Configuration file not found! Please copy example.config.yml to config.yml and try again.'
 end
-vconfig = YAML.load_file("#{dir}/config.yml")
+vconfig = YAML.load_file(config_file)

 # Replace jinja variables in config.
 vconfig = walk(vconfig) do |value|
@@ -112,6 +113,9 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
   if which('ansible-playbook')
     config.vm.provision 'ansible' do |ansible|
       ansible.playbook = "#{dir}/provisioning/playbook.yml"
+      ansible.extra_vars = {
+        "config_file": config_file
+      }
     end
   else
     config.vm.provision 'shell' do |sh|
@@ -121,9 +125,13 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
   end
   # ansible_local provisioner is broken in Vagrant < 1.8.2.
   # else
+  #   config.vm.provision "file", souce: config_file, destination: "/tmp/config.yml"
   #   config.vm.provision "ansible_local" do |ansible|
   #     ansible.playbook = "provisioning/playbook.yml"
   #     ansible.galaxy_role_file = "provisioning/requirements.yml"
+  #     ansible.extra_vars = {
+  #       "config_file": "/tmp/config.yml"
+  #     }
   #   end
   # end

You would use a delegating Vagrantfile with:

ENV['DRUPALVM_CONFIG'] = "#{__dir__}/config/drupal-vm.config.yml"
load "box/Vagrantfile"

Once we support local.config.yml, we could even reference the example.config.yml in Drupal VM and simply override what's necessary.

ENV['DRUPALVM_CONFIG'] = "#{__dir__}/box/example.config.yml"
ENV['DRUPALVM_LOCAL_CONFIG'] = "#{__dir__}/config/drupal-vm.config.yml"
load "box/Vagrantfile"
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Feb 29, 2016

Here's a far simpler patch which only tackles the issue of storing Drupal VM in a subdirectory.

This should work already as we aren't relying on ansible_local anymore. Currently it fails due to the upstream Galaxy issue though. I'll test it and make sure it works as soon as the galaxy bug is fixed and I have some spare time again.

@geerlingguy what are your thoughts about this? By only adding support for a custom config location this patch is a lot smaller and less scary. If you like this direction more I'll rebase it over the stuff in this PR.

master...oxyc:subdir

@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented Feb 29, 2016

@oxyc - I do like having it a bit simpler, though an environment variable could be slightly perplexing if you have multiple Drupal VM instances in different subdirs...

Also, any changes to provisioning/JJG-Ansible-Windows/windows.sh should be submitted against the upstream project JJG-Ansible-Windows.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Feb 29, 2016

Also, any changes to provisioning/JJG-Ansible-Windows/windows.sh should be submitted against the upstream project JJG-Ansible-Windows.

Ah I wasn't aware, if you're interesting in getting this in before we move back to ansible_local I can clean it up as a generalized extra_vars flag.

The environment variable isn't exported though so it's just available within the process. Same as the general unix convention of DEBUG=1 cmd. Without the delegating Vagrantfile this would do exactly the same:
DRUPALVM_CONFIG="~/foo/config.yml" vagrant up.

This opens up the possibility for non-ruby wrappers to interact with it, and makes more sense to me than ruby global $config_path

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented Mar 1, 2016

So back to having to define DRUPALVM_PROJECT_ROOT. It really is required to know the location of the parent Vagrantfile and I can't find a way to figure it out without manually specifying it:

ENV['DRUPALVM_PROJECT_ROOT'] = "#{__dir__}"
ENV['DRUPALVM_CONFIG'] = "#{__dir__}/config/drupal-vm.config.yml"
load "box/Vagrantfile"

I feel like I've exhausted all my ideas, there doesn't seem to be an easier way to implement it really. Happy to rework the PR if someone has some feedback though.

@joestewart

This comment has been minimized.

Copy link
Contributor

joestewart commented Mar 24, 2016

@oxyc This is looking great. Worked after some small changes but have only lightly tested. Will be nice to have a local.config.yml with the box in a subdir too. thanks.

I had to make these changes:

diff --git a/Vagrantfile b/Vagrantfile
index 9fef925..2ada931 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -20,7 +20,7 @@ host_project_root   = ENV['DRUPALVM_PROJECT_ROOT'] || host_drupalvm_root
 guest_drupalvm_root = "/vagrant/#{Pathname.new(host_drupalvm_root).relative_path_from(Pathname.new(host_project_root))}"
 guest_project_root  = "/vagrant"

-host_vconfig_path       = ENV['DRUPALVM_CONFIG']       ? "#{host_project_root}/#{ENV['DRUPALVM_CONFIG']}"        : "#{host_drupalvm_root}/config.yml"
+host_vconfig_path       = ENV['DRUPALVM_CONFIG']       ? "#{ENV['DRUPALVM_CONFIG']}"        : "#{host_drupalvm_root}/config.yml"
 host_playbook_path      = ENV['DRUPALVM_PLAYBOOK']     ? "#{host_project_root}/#{ENV['DRUPALVM_PLAYBOOK']}"      : "#{host_drupalvm_root}/provisioning/playbook.yml"
 guest_vconfig_path      = ENV['DRUPALVM_CONFIG']       ? "#{guest_project_root}/#{ENV['DRUPALVM_CONFIG']}"       : "#{guest_drupalvm_root}/config.yml"
 guest_playbook_path     = ENV['DRUPALVM_PLAYBOOK']     ? "#{guest_project_root}/#{ENV['DRUPALVM_PLAYBOOK']}"     : "#{guest_drupalvm_root}/provisioning/playbook.yml"
@@ -28,6 +28,7 @@ guest_requirements_path = ENV['DRUPALVM_REQUIREMENTS'] ? "#{guest_project_root}/

 # Use config.yml for basic VM configuration.
 require 'yaml'
+print
 if !File.exist?(host_vconfig_path)
   raise 'Configuration file not found! Please copy example.config.yml to config.yml and try again.'
 end
@@ -100,7 +101,7 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
     config.vm.provision "ansible" do |ansible|
       ansible.playbook = host_playbook_path
       ansible.extra_vars = {
-        "config_file": host_vconfig_path
+        "config_file" => host_vconfig_path
       }
     end
   else
@@ -108,7 +109,7 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
       ansible.playbook = guest_playbook_path
       ansible.galaxy_role_file = guest_requirements_path
       ansible.extra_vars = {
-        "config_file": guest_vconfig_path
+        "config_file" => guest_vconfig_path
       }
     end
   end
@oxyc oxyc force-pushed the oxyc:decouple branch from df87df5 to 397aa88 May 15, 2016
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

Rebased and my version of that commit disappeared. So good to go if no other changes are needed.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

Or no. Found an issue.

@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented May 15, 2016

K, I'm fading fast, so I'll take another look tomorrow. Thanks!

@oxyc oxyc force-pushed the oxyc:decouple branch from 397aa88 to baa20ca May 15, 2016
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

Yeah, this is a pretty big change so better to review it properly.

Btw. for quickly testing different setups you can do stuff like:

mkdir config-scenario
mkdir config-second-scenario
cp example.config.yml config-scenario/config.yml
cp example.config.yml config-second-scenario/config.yml

DRUPALVM_CONFIG_DIR="config-scenario" vagrant provision

Pretty handy to be honest.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

To make example.config.yml work out of the box in a subdirectory we can change drush_makefile_path to

drush_makefile_path: "{{ config_dir }}/drupal.make.yml"

Then it assumes it's located in the same directory as config.yml.

@oxyc oxyc force-pushed the oxyc:decouple branch from b70b443 to 2257643 May 15, 2016
@oxyc oxyc force-pushed the oxyc:decouple branch from 2257643 to b793d0d May 15, 2016
@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

I updated the drush_makefile_path and the docs but if you prefer I can revert that commit.

@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented May 15, 2016

@oxyc - You know, one other thing that I'm wondering... with our new with_fileglob setup, why not make config.yml optional as well. Load in example.config.yml (maybe name it default.config.yml) as the base, since it will always be present, then load config.yml, local.config.yml (and in the future others) iff they exist? (Probably something for a different issue, but I'm still reading through here right now).

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 15, 2016

I was considering using config.yml as a symlink to Drupal VMs example.config.yml and use local.config.yml for our team config so that people at work have easier keeping track of what's different and hopefully so they can do the upgrades themselves. Your idea would work very nicely for this setup.

That's also something I personally liked about beetbox.

@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented May 18, 2016

@oxyc I'm going to merge this as-is—can you maybe open a follow-up issue or PR to start using example.config.yml as the default (so config.yml is optional)?

@geerlingguy geerlingguy merged commit df7819c into geerlingguy:master May 18, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented May 18, 2016

Now I start the gauntlet of tests on my Mac and Windows laptops... please everyone test in different scenarios to see what might be broken, so we cut a nice clean 3.0.0 release!

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 18, 2016

@oxyc I'm going to merge this as-is—can you maybe open a follow-up issue or PR to start using example.config.yml as the default (so config.yml is optional)?

Sure, I'll work on getting a PR sent over a bit later today.

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 18, 2016

Also yey! This is the following up on my first PR #147 😃

@oxyc

This comment has been minimized.

Copy link
Collaborator Author

oxyc commented May 19, 2016

I tested and got one of our client projects set up using Drupal VM as a composer dependency by following the docs. Awesome!

P.S. aside from Drupal VM not having a stable release on packagist yet.

@geerlingguy

This comment has been minimized.

Copy link
Owner

geerlingguy commented May 19, 2016

Awesome! I'll try to tag 3.0.0 in a little bit. Need to get my notes together first...

@oxyc oxyc deleted the oxyc:decouple branch Apr 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.