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 vagrant-bindfs for custom NFS permissions #83

Merged
merged 1 commit into from Oct 21, 2014

Conversation

fullyint
Copy link
Contributor

This PR stems out of #82.

Notes about options. I added the options u: 'vagrant' and g: 'www-data' to override the vagrant-bindfs default of 'vagrant' for each. This frees the wordpress-sites role from having to perform a chown to vagrant:www-data.

The bindfs man page indicates that -u and -g cause chown and chgrp on the mounted filesystem to always fail. So, I assume anyone customizing their playbooks to chown to something other than vagrant:www-data will see a failure warning. In that case, the --chown-ignore and --chgrp-ignore options might be of use.

Other Notes

  • I tested on OSX only. I'm not aware of any reason it wouldn't work on other NFS-compatible hosts.
  • I omitted warnings for missing vagrant-bindfs plugin, a la if !Vagrant.has_plugin? 'vagrant-binfs'
  • I didn't try anything with mount_options: ["dmode=775,fmode=664"]

@swalkinshaw
Copy link
Member

Thought this didn't work for a bit then realized my local shared folder had its permissions previously changed by the normal NFS. Reset them back and it's all working as it should.

@fullyint @austinpray @nathanielks hijacking this PR for a vacation notice. I'll be gone until Nov 10th or thereabouts with limited internet so apologies if PRs/Issues are stuck until then.

swalkinshaw added a commit that referenced this pull request Oct 21, 2014
Add vagrant-bindfs for custom NFS permissions
@swalkinshaw swalkinshaw merged commit d459485 into roots:master Oct 21, 2014
@fullyint fullyint deleted the bindfs branch October 21, 2014 20:55
@austinpray
Copy link
Contributor

@swalkinshaw all is lost, glorious leader has abandoned us.

@austinpray
Copy link
Contributor

@fullyint

I omitted warnings for missing vagrant-bindfs plugin, a la if !Vagrant.has_plugin? 'vagrant-binfs'

why?

@fullyint
Copy link
Contributor Author

@austinpray Glad you asked. I mentioned omitting the check/warnings so others could share feedback. I wasn't sure about it. My rationale:

  • the README now lists vagrant-bindfs in the Requirements section
  • it's maybe not justified to add the extra code, given that vagrant itself will provide this warning:
There are errors in the configuration of this machine. Please fix
the following errors and try again:
Vagrant:
* Unknown configuration section 'bindfs'.

Presumably a user would see that and think, "Huh, what's this 'bindfs'? Let me go double-check the README." But, admittedly, it would be clearer if we provided a custom warning, "You need to install the vagrant-bindfs plugin." What do you think?

@nathanielks
Copy link
Contributor

I'd lean on adding warnings. Most likely will end up lessening debug'ing time for those who may not have it. Yeah, they can refer to the README, or we can just nudge them in the right direction straight from the console :)

@austinpray
Copy link
Contributor

def wall_of_text

Presumably a user would see that and think, "Huh, what's this 'bindfs'? Let me go double-check the README." But, admittedly, it would be clearer if we provided a custom warning, "You need to install the vagrant-bindfs plugin." What do you think?

That sounds like something that, in your head, sounds like a fine idea. However, once you actually type it out and explain to someone else it very clearly sounds like a bad idea. If we can catch an error and provide helpful error messages, we should do it.

One thing we should keep in mind is that we should do everything in our power to keep the perceived complexity of the roots stack to an absolute minimum. I will clarify what I mean by "perceived complexity".

As a consultant I am often presenting the bedrock/12 factor app way to do things to people who think "web development" means "edit in sublime text and ftp to production to see if it works". A comment I often get is "geez this way of doing things is really complex". When I get comments like that I have NO idea what they are talking about. The bedrock/12 factor app way of doing things is incredibly predictable and easy to manage. People are saying bedrock is "really complex" while completely forgetting the world of hurt you run into when FTPing things to production crossing your fingers and hoping it works. Where the "really complex" comment comes from is from people who are trying to set things up for the first time and run into trouble. Every time they have to take a trip to the readme they get closer and closer to quitting.

Personally, if I got the error message

There are errors in the configuration of this machine. Please fix
the following errors and try again:
Vagrant:
* Unknown configuration section 'bindfs'.

I would right away know how to fix it. In fact I wouldn't even need to look up the vagrant command to install the plugin. Even if I didn't know how to fix it a quick google search or just looking at the source would fix it.

However some people just immediately fire off an email to their boss and or me saying "yeah the thing isn't working, this is frustrating and I can't get any work done". I then say "well it says right here in the instructions that you need to have this installed". Now that guy feels dumb (rightfully so perhaps) and has an adversarial relationship with the tools. This actually happens haha. Twice in the past two months.

Now I am not saying we should dumb down anything to pander to dummies, but we could avoid all of the above with an extra two lines of code. The CLI is our "interface", so it should be as friendly as possible so more people can use bedrock. The more people use bedrock, perhaps the more people put pressure on wordpress to support our methodologies out of the box.

end

@fullyint
Copy link
Contributor Author

Awesome! I'm convinced! Thank you so much @nathanielks and @austinpray.

I have no ruby experience, so set me straight with the code below.

  • Detects platform like this (inspiration) -- no more manual uncommenting for windows.
  • People may find the bedrock_path variable simpler than hunting for multiple places to edit path.
  • The "forward slash" thing is required by basename.
# Define path to bedrock directory on your local host machine
#   - relative to Vagrantfile
#   - use forward slashes ("/") regardless of your OS
bedrock_path = '../example.dev'

# Sync bedrock directory
bedrock_path_server = '/srv/www/' + File.basename(bedrock_path) + '/current'
if Vagrant::Util::Platform.windows?
  config.vm.synced_folder bedrock_path, bedrock_path_server, owner: 'vagrant', group: 'www-data', mount_options: ['dmode=776', 'fmode=775']
else
  if !Vagrant.has_plugin? 'vagrant-bindfs'
    puts 'vagrant-bindfs missing, please install the plugin:'
    puts 'vagrant plugin install vagrant-bindfs'
    abort
  else
    config.vm.synced_folder bedrock_path, '/vagrant-nfs', type: 'nfs'
    config.bindfs.bind_folder '/vagrant-nfs', bedrock_path_server, u: 'vagrant', g: 'www-data'
  end
end

Alternatively, we could auto-install the missing vagrant-bindfs like this, but that feels heavy handed.

@nathanielks
Copy link
Contributor

I'll need to test this out, but at first glance it looks alrighty @fullyint. Definitely concur with not doing the heavy-handed approach. Need to give the user the option to install, not force them into it.

@fullyint
Copy link
Contributor Author

Seems better with Vagrant's error handling...

 if !Vagrant.has_plugin? 'vagrant-bindfs'
-  puts 'vagrant-bindfs missing, please install the plugin:'
-  puts 'vagrant plugin install vagrant-bindfs'
-  abort
+  raise Vagrant::Errors::VagrantError.new,
+    "vagrant-bindfs missing, please install the plugin:\nvagrant plugin install vagrant-bindfs"
 else

fullyint added a commit to fullyint/trellis that referenced this pull request Oct 30, 2014
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