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 DrupalVM 3.0 support to BLT projects #93

Closed
wants to merge 4 commits into
base: 8.x
from

Conversation

Projects
None yet
5 participants
@dpagini
Copy link
Contributor

dpagini commented May 31, 2016

Offering up this commit to BLT project, as I am trying to incorporate something similar into acquia-pso/voya_im (private repo), a BLT based project.
This will add DrupalVM support to all BLT projects, regardless of the desire to use it. I don't think it's huge overhead, however, as it's a few files (2?), and an additional composer dependency. There are other tools brought in through this default composer.json (drupal console, for example) that might be precedent for pulling in optional tools.

Please feel free to reject if this is not an appropriate direction, or offer up suggested changes. @danepowell suggested I create this as a possible BLT change since I implemented a similar approach in our repo.

I look forward to hearing any thoughts.

cc: @geerlingguy - making sure you get a chance to comment on this. And, thank you for all your work on DrupalVM!!

@@ -80,6 +80,15 @@ There are also other changes you can make if you choose to match the Acquia Clou

Once you've made these changes and completed the steps in Drupal VM's Quick Start Guide, you may run `vagrant up` to bring up your local development environment, and then access the site via the configured `drupal_domain`.

### Using DrupalVM 3.0

This comment has been minimized.

@geerlingguy

geerlingguy May 31, 2016

Member

Should be 'Drupal VM' (here and in other instances).

This comment has been minimized.

@geerlingguy

geerlingguy May 31, 2016

Member

Also, we might want to just drop the '3.0' part, since presumably, I won't ever break this kind of integration in 3.x/4.x and beyond :D

This comment has been minimized.

@dpagini

dpagini May 31, 2016

Author Contributor

If I leave the above section, where drupal-vm was meant to be pulled into a "box" folder, I wanted to state that specifically "> 3.0" was now supported automatically in the project. This entire README should be looked at when a decision is made on this, I think.

@@ -80,6 +80,15 @@ There are also other changes you can make if you choose to match the Acquia Clou

Once you've made these changes and completed the steps in Drupal VM's Quick Start Guide, you may run `vagrant up` to bring up your local development environment, and then access the site via the configured `drupal_domain`.

### Using DrupalVM 3.0

This project has been configured to run with [DrupalVM 3.0](geerlingguy/drupal-vm). You can call `vagrant up` directly from the project root as of version 3.0. The new DrupalVM is pulled in as a dev dependency through composer, and uses a delegating Vagrantfile to call the project's Vagrantfile in the `vendor/geerlingguy/drupal-vm` folder. The project is pre-configured with a config.yml file located at /config/drupalvm/config.yml. You may use a local.config.yml file to overwrite any values in the repositories copy of the file.

This comment has been minimized.

@geerlingguy

geerlingguy May 31, 2016

Member

We might not want to commit an entire config.yml to BLT directly—config.yml itself can and will change over time, as new variables are added, old ones deprecated, etc.

It would be much more maintainable if the build process just copied example.config.yml to config.yml when the local environment is set up (or maybe make this happen as part of the delegated Vagrantfile?).

This comment has been minimized.

@dpagini

dpagini May 31, 2016

Author Contributor

That makes sense to me, I think. I might need a little help with that as I'm not too well-versed in Phing. Would we want to do any example.config.yml substitutions to make it project specific?

This comment has been minimized.

@geerlingguy

geerlingguy May 31, 2016

Member

Technically you'd want to do something along these lines: https://github.com/geerlingguy/drupal-vm/blob/master/examples/acquia/acquia.overrides.yml (though that needs updating too).

Also, the vagrant_synced_folders should be updated for Drupal VM... hmm...

(Opened upstream issue: geerlingguy/drupal-vm#682)


This project has been configured to run with [DrupalVM 3.0](geerlingguy/drupal-vm). You can call `vagrant up` directly from the project root as of version 3.0. The new DrupalVM is pulled in as a dev dependency through composer, and uses a delegating Vagrantfile to call the project's Vagrantfile in the `vendor/geerlingguy/drupal-vm` folder. The project is pre-configured with a config.yml file located at /config/drupalvm/config.yml. You may use a local.config.yml file to overwrite any values in the repositories copy of the file.

It is recommended you have the following set up on your local machine.

This comment has been minimized.

@geerlingguy

geerlingguy May 31, 2016

Member

I don't like making people depend on vagrant-auto_network, as I've had little issues with it here or there, and it is not always consistent with new version of Vagrant/across OSes. It would possibly be better to leave the default Drupal VM IP setup, and recommend it (and a change to the IP address in a dev's local.config.yml file) iff the dev is comfortable installing the extra plugin.

@danepowell

This comment has been minimized.

Copy link
Collaborator

danepowell commented May 31, 2016

@geerlingguy I was also wondering what you thought about using the /config/drupalvm directory. I think /config should only contain Drupal config export yml files, and Drupal VM config should live elsewhere... but I don't really know where. /drupalvm/config?

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented May 31, 2016

@danepowell - Ideally, there would be no additional config required (besides the delegated project Vagrantfile)—but if there does need to be an additional config file, that's a better question for @grasmash maybe... I don't want to have to help debug weird issues where BLT's config goes out of sync with the latest stable version of Drupal VM's.

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented May 31, 2016

Originally I was opposed to including a config.yml file here. But maybe I'm okay with it in the case of BLT as long as we stabilize on a minor version of Drupal VM. Basically, I won't break your config.yml on 3.0.x, or 3.1.x, but between 3.0 and 3.1 there might be a change required.

The thing is, someone will need to make sure BLT's config.yml is updated any time there's a minor release of Drupal VM if we go that route.

It could just be a documentation thing—"If you use Drupal VM, you should also make these changes to the project's config.yml", and list the changes.

@dpagini

This comment has been minimized.

Copy link
Contributor Author

dpagini commented May 31, 2016

So, I almost feel like the config.yml that actually makes it into a project repo should be project specific, no? If your project uses solr, you would want your project's config.yml to build that into the VM. ...but to your point, that probably means fixing onto a version of drupal-vm.

@oxyc

This comment has been minimized.

Copy link

oxyc commented May 31, 2016

Drupal VM 3.1.0 would make this a lot easier with the defaults pre-set.

Then you'd just need:

vagrant_hostname: ${project.machine_name}.local
vagrant_machine_name: ${project.machine_name}
vagrant_ip: 0.0.0.0

vagrant_synced_folders:
  - local_path: ./
    destination: /var/www/${project.machine_name}
    type: nfs
    create: true

drupal_core_path: "/var/www/${project.machine_name}/docroot"
drupal_site_name: "${project.human_name}"

Why not even default the upstream local_path to ./ instead of the current ~/Sites/drupalvm. Seems like a saner default no?

Then the config.yml could be as short as:

vagrant_hostname: ${project.machine_name}.local
vagrant_machine_name: ${project.machine_name}
vagrant_ip: 0.0.0.0
drupal_site_name: "${project.human_name}"
@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented May 31, 2016

@oxyc++ - maybe I could fast-track that issue... :)

@oxyc

This comment has been minimized.

Copy link

oxyc commented May 31, 2016

Forgot we also need (@dpagini remembered :)) :

build_makefile: false

I guess that will be removed in 3.2.0 or something? :)

@@ -0,0 +1 @@
local.config.yml

This comment has been minimized.

@oxyc

oxyc May 31, 2016

Then there's the Vagrantfile.local as well.

@grasmash

This comment has been minimized.

Copy link
Collaborator

grasmash commented Jun 1, 2016

I'd like to keep all of the drupal vm related config isolated in one directory, if possible. In past we've used box because it's simple. Seems like the yml and Vagrantfile would go well there.

@oxyc

This comment has been minimized.

Copy link

oxyc commented Jun 1, 2016

The Vagrantfile needs to be in a parent directory of both the config and the Drupal VM codebase. As composer is used that means the Vagrantfile needs to be in a directory above vendor/.

Edit: Actually I guess it works if you adjust the DRUPALVM_PROJECT_ROOT variable to point to a mutual parent directory.

@dpagini

This comment has been minimized.

Copy link
Contributor Author

dpagini commented Jun 1, 2016

So "defaults" are coming in Drupal VM 3.1? That sounds like an awesome new feature!

I tend to think with the "box" approach that has been in BLT up to now, that folder was the optional place to put Drupal VM if you wanted to use it (it was not even gitignored though). If we do pull the tool into all projects with this PR, I sort of think there's a benefit to being able to run all commands from the root (which means the Vagrantfile really needs to be in the root). Maybe there could be a blt phing task to fire up drupal-vm? Then we could put the Vagrantfile and config.yml wherever we want?

If the Vagrantfile could go anywhere, @grasmash, you're saying it makes the most sense in a new "box" folder? Is that to say that "config" should only be for Drupal related config? What if in "config" we had a "tools" folder, and a "drupal" folder. Just throwing an idea out there. If we did that, maybe the "drush" folder could move under "config" as well, as that folder seems to be primarily configuration related items...?

As I'm pretty new to this open source contributing, please feel free to boss me around if I need to update this PR at some point. I'm not totally sure on next steps, and feel a little over my head here. I hope my contributions and opinions are helpful.

@danepowell

This comment has been minimized.

Copy link
Collaborator

danepowell commented Jun 1, 2016

I'm not opposed to consolidating directories, but /config has a very specific use in D8, so I'd prefer not to overload that with anything not directly related to the D8 config sync system. I don't know that it would necessarily break anything, but it just feels dirty 😄

@dpagini

This comment has been minimized.

Copy link
Contributor Author

dpagini commented Jun 1, 2016

Maybe just a "tools" directory? ./tools/drupal-vm/ and ./tools/drush/, for starters.

(I say that not even knowing/testing if the "drush" directory can move)

@grasmash

This comment has been minimized.

Copy link
Collaborator

grasmash commented Jun 1, 2016

I think that we should keep the top level config dir for drupal config only. "Configuration" is now a loaded term in Drupal 8 given the existence of the configuration management system. While other things like drush could be considered config, I think we need to be very careful with when and where we choose to use the term.

I'd suggest using the box dir for the VM. If you need a config dir for the VM, you can use box/config or other various subdirectories. That seems clear enough.

I would not be opposed to adding a few VM targets. The trouble is that we can't test them via Travis, but that's unavoidable.

@geerlingguy

This comment has been minimized.

Copy link
Member

geerlingguy commented Jun 3, 2016

Note that Drupal VM's 3.1.0 release should happen within a couple days—currently the last issue being worked on is composer.json Drupal site build support. Once that's done, I'm going to tag a 3.1.0 release that includes the default.config.yml configuration setup, so BLT would only need to generate a very small/efficient config.yml file with BLT/Acquia-recommended overrides, and then developers could add additional local overrides (e.g. extra RAM if the dev is lucky enough to have 16+ GB of RAM) in a gitignored local.config.yml file.

@grasmash

This comment has been minimized.

Copy link
Collaborator

grasmash commented Jun 3, 2016

Based on the comments in this PR, I've started another PR for Drupal VM 3.1 integration: #105

This makes use of a vm:init target, which will initialize the project for Drupal VM by moving files into the correct places.

@grasmash

This comment has been minimized.

Copy link
Collaborator

grasmash commented Jun 8, 2016

Close in favor of #105, thanks for all the help everyone.

@grasmash grasmash closed this Jun 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment