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

Vagrant config #828

Merged
merged 2 commits into from
Apr 22, 2017
Merged

Vagrant config #828

merged 2 commits into from
Apr 22, 2017

Conversation

swalkinshaw
Copy link
Member

@swalkinshaw swalkinshaw commented Apr 14, 2017

Inspired by https://github.com/geerlingguy/drupal-vm

  • Move methods into separate file
  • Intro Vagrant config

Todo:

  • Better name for vagrant.yml. drupal-vm uses default.config.yml?
  • Support for untracked config file?

@mAAdhaTTah
Copy link
Contributor

mAAdhaTTah commented Apr 14, 2017

Can I make a suggestion: I edit my local Vagrantfile to mount WP plugins I'm working on into the VM. Can the configuration be edited to allow one to include a src/dest array:

mount_dirs:
    - src: 'path/to/plugin'
      dest: 'path/to/wp-content/plugins/plugin'
    - src: 'path/to/plugin2'
      dest: 'path/to/wp-content/plugins/plugin2'

or something like this?

@swalkinshaw
Copy link
Member Author

@mAAdhaTTah sure, makes sense 👍

@swalkinshaw
Copy link
Member Author

@mAAdhaTTah added, try it out 👼

@swalkinshaw swalkinshaw changed the title [WIP] Vagrantfile refactor Vagrantfile refactor Apr 14, 2017
@swalkinshaw swalkinshaw changed the title Vagrantfile refactor Vagrant config Apr 14, 2017
Adds a new `vagrant.default.yml` config file which is used for Vagrant
related settings in `Vagrantfile.`

`vagrant.local.yml` is also supported as a local, untracked config file
which takes precedence over the default.
Copy link
Contributor

@fullyint fullyint left a comment

Choose a reason for hiding this comment

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

This PR will make the Trellis Vagrant config so much nicer and cleaner!

Important. The one issue I think must be dealt with is how the vagrant_synced_folders loop handles bindfs folders. I've added two inline comments in the code to explain. It avoids a vagrant-bindfs error such as this:

vagrant-bindfs:
* Path '../db' is relative but an absolute path is attended

Probably. One issue you probably want to address is fixing the sequence of the CHANGELOG.md entries to be chronological (fixing sequence with Ansible 2.3 compatibility entry).

Maybe. There are two issues you'll only maybe want to address:

  1. You could change the vagrant_ansible_version to 2.3
  2. You could move def hosts(sites) up under def load_wordpress_sites (in lib/trellis/vagrant.rb) because they're similar in concept and that would match the sequence and proximity in which they are called in Vagrantfile.

Probably not. One issue you will probably not care to address is that you could put a comment at the top of vagrant.default.yml saying "Jinja2 variables and templating will not function in this file."

end

def nfs_path(site_name)
"/vagrant-nfs-#{site_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be used for nfs path names in the vagrant_synced_folders loop:

- def nfs_path(site_name)
-   "/vagrant-nfs-#{site_name}"
+ def nfs_path(path)
+   "/vagrant-nfs-#{File.basename(path)}"
  end

Vagrantfile Outdated
config.vm.synced_folder folder['local_path'], folder['destination'], options

if folder.fetch('bindfs', true)
config.bindfs.bind_folder folder['local_path'], folder['destination'], options
Copy link
Contributor

Choose a reason for hiding this comment

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

When using vagrant-bindfs, the convention appears to be...

  1. do a regular vagrant folder sync from somedir to /vagrant/somedir-nfs
  2. do a bindfs sync from /vagrant/somedir-nfs to /final_path_on_vm/somedir

Combined with the File.basename() adjustment to nfs_path() proposed in a separate comment, the following seems to handle it.

+ destination_folder = folder.fetch('bindfs', true) ? nfs_path(folder['destination']) : folder['destination']

- config.vm.synced_folder folder['local_path'], folder['destination'], options
+ config.vm.synced_folder folder['local_path'], destination_folder, options

  if folder.fetch('bindfs', true)
-   config.bindfs.bind_folder folder['local_path'], folder['destination'], options
+   config.bindfs.bind_folder destination_folder, folder['destination'], folder.fetch('bindfs_options', {})

The other piece to this is the bindfs_options, which are different from a regular vagrant sync dir options and must be treated differently. The above appears to successfully handle the following example

# vagrant.local.yml
vagrant_synced_folders:
  - local_path: ../db
    destination: /home/vagrant/db
    create: false
    type: nfs
    bindfs: true
    bindfs_options:
      o: 'nonempty'
      p: '0644,a+D'
  - local_path: ../test
    destination: /home/vagrant/test
    create: false
    type: virtualbox
    bindfs: false
    mount_options:
      - 'dmode=755'
      - 'fmode=644'

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 updated

@fullyint
Copy link
Contributor

👍 Looks great to me!

@swalkinshaw swalkinshaw merged commit 560b523 into master Apr 22, 2017
@swalkinshaw swalkinshaw deleted the vagrantfile-refactor branch June 6, 2017 02:04
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.

3 participants