Open calbrecht opened 5 years ago
Hi @calbrecht - you can already set the NFS version with nfs_version
: https://www.vagrantup.com/docs/synced-folders/nfs.html#nfs_version
Any other specific options for NFS can be set with mount_options
. Otherwise the update you proposed is already how synced_folders
works. It takes a hash that Vagrant uses to set various synced folder options.
The synced_folder option should be fairly agnostic to any plugin, so I'm not sure what specifically you are needing with the puppet or chef provisioner. I believe these plugins might do a check that the correct folders are in place on the guest, but I don't think it should care how the folders got there.
I'll go ahead and close this, but if you have any more feedback feel free to comment here. Otherwise if you have more questions I recommend directing them to the mailing list. Thanks!
@briancain The puppet and probably the chef provisioner are creating synced_folders where i am not able to set the before mentioned options.
See plugins/provisioners/puppet/provisioner/puppet.rb
line 31 through 35
folder_opts = {}
folder_opts[:type] = @config.synced_folder_type if @config.synced_folder_type
folder_opts[:owner] = "root" if !@config.synced_folder_type
folder_opts[:args] = @config.synced_folder_args if @config.synced_folder_args
folder_opts[:nfs__quiet] = true
and line 44 through 46
root_config.vm.synced_folder(
File.expand_path(@config.environment_path[1], root_path),
environments_guest_path, folder_opts)
@briancain please reopen as these plugins set/overwrite any synced_folder options specific to those provisioner folder paths i set in vm config. -- edit i do it myself. -- edit i am not allowed to reopen :/
@calbrecht - Ah ok, I see now. It's not an enhancement for the synced_folder
type, but something for the two providers specifically. This would be an enhancement to these config options:
https://www.vagrantup.com/docs/provisioning/puppet_apply.html#synced_folder_type
https://www.vagrantup.com/docs/provisioning/chef_solo.html#synced_folder_type
I think it should probably take some "additional" hash argument like you mentioned that could be merged into folder_opts
there. Then as long as the keys match up to what synced_folder expects, the options should "just work".
Thank you @briancain exactly that is the patch i got at hand for the puppet provisioner, so i will go ahead and craft a PR for those two provisioners if it fits for the chef provisioner as well.
I guess i should deprecate the synced_folder_type option like it was done with the nfs option before and update the docs too, correct?
@calbrecht - I think we should leave the existing options in place, otherwise upgrades will break existing users Vagrantfiles. We should check if both keys exist for the options that are already there inside the new config option hash, and put a warning that the passed in hash options will collide with synced_folder_type
and will be ignored in favor of the new passed in hash. It looks like there's really only a couple of options we need to worry about colliding with: type
and args
, where args looks to be similar to mount_options
? I'm not 100% sure about that though so it would be good to check. I also only looked at the puppet provisioner for this, so the chef provisioner might be different. Either way they should behave the same way!
Also yes, an update to the website will be needed along with unit tests! Thanks :)
I thought about marking synced_folder_type
as deprecated but leaving it in place for now, while at the same time introducing the new option synced_folder_opts
that is holding a hash of valid options (same as in https://www.vagrantup.com/docs/synced-folders/basic_usage.html#options).
And then let synced_folder_opts
overwrite synced_folder_type
.
like so:
diff --git a/plugins/provisioners/puppet/config/puppet.rb b/plugins/provisioners/puppet/config/puppet.rb
index 1f31cce6a..178e98e86 100644
--- a/plugins/provisioners/puppet/config/puppet.rb
+++ b/plugins/provisioners/puppet/config/puppet.rb
@@ -36,11 +36,16 @@ module VagrantPlugins
@options = []
@facter = {}
@synced_folder_type = UNSET_VALUE
+ @synced_folder_opts = {}
@temp_dir = UNSET_VALUE
@working_directory = UNSET_VALUE
@structured_facts = UNSET_VALUE
end
+ def synced_folder_opts(**opts)
+ @synced_folder_opts = @synced_folder_opts.merge!(opts)
+ end
+
def nfs=(value)
puts "DEPRECATION: The 'nfs' setting for the Puppet provisioner is"
puts "deprecated. Please use the 'synced_folder_type' setting instead."
diff --git a/plugins/provisioners/puppet/provisioner/puppet.rb b/plugins/provisioners/puppet/provisioner/puppet.rb
index be4b8c79f..f846857eb 100644
--- a/plugins/provisioners/puppet/provisioner/puppet.rb
+++ b/plugins/provisioners/puppet/provisioner/puppet.rb
@@ -34,6 +34,10 @@ module VagrantPlugins
folder_opts[:args] = @config.synced_folder_args if @config.synced_folder_args
folder_opts[:nfs__quiet] = true
+ if @config.synced_folder_opts
+ folder_opts = folder_opts.merge!(@config.synced_folder_opts)
+ end
+
if @config.environment_path.is_a?(Array)
# Share the environments directory with the guest
if @config.environment_path[0].to_sym == :host
Good point about args
, i need to investigate that, and thanks for reminding me about testing :)
@calbrecht - the merging should happen inside of the finalize!
method in the config for the plugin I believe. And then within the validate
method we can show errors if both the synced_folder_opts has a key type
set and synced_folder_type
are set (along with other colliding options, if any, like args versus mount_options)
The args
option seems to be specific to the rsync synced_folder type as written in the docs:
"synced_folder_args (array) - Arguments that are passed to the folder sync. For example ['-a', '--delete', '--exclude=fixtures'] for the rsync sync command." https://www.vagrantup.com/docs/provisioning/puppet_apply.html#synced_folder_args
Also i could not find any code relevant to synced_folders when grepping for :args
besides within /plugins/synced_folders/rsync/*
@briancain i see, will try that -- i had a similar merge within initialize
of the config for the plugin, like:
index 1f31cce6a..178e98e86 100644
--- a/plugins/provisioners/puppet/config/puppet.rb
+++ b/plugins/provisioners/puppet/config/puppet.rb
@@ -36,11 +36,16 @@ module VagrantPlugins
@options = []
@facter = {}
@synced_folder_type = UNSET_VALUE
+ @synced_folder_opts = {}
@temp_dir = UNSET_VALUE
@working_directory = UNSET_VALUE
@structured_facts = UNSET_VALUE
end
+ def synced_folder_opts(**opts)
+ @synced_folder_opts = @synced_folder_opts.merge!(opts)
+ end
+
def nfs=(value)
puts "DEPRECATION: The 'nfs' setting for the Puppet provisioner is"
puts "deprecated. Please use the 'synced_folder_type' setting instead."
if it will suffice to do the merging once within the finalize!
method, i'll be glad to do it there, please note that i'm not familiar with ruby in general and happy about any hints how to improve the solution.
@calbrecht - I'm starting to think maybe the synced_folder_args
option is left over from when the provider was using nfs? It gets passed through to synced_folder
as :args
but nothing actually picks up and uses it. I tested it by having some bogus args:
config.vm.define "puppet" do |p|
p.vm.box = "bento/ubuntu-16.04"
p.vm.provision "shell", inline: <<-SHELL
wget https://apt.puppetlabs.com/puppet5-release-xenial.deb
sudo dpkg -i puppet5-release-xenial.deb
sudo apt update
sudo apt-get install puppet-agent
SHELL
p.vm.provision :puppet do |p|
p.module_path = ['modules', 'site']
p.synced_folder_args = ["lol", "nope"]
p.synced_folder_type = "nfs"
end
p.vm.provider :virtualbox
p.vm.network :private_network, type: "dhcp"
p.vm.provision "shell", inline: <<-SHELL
puppet --version
SHELL
end
Nothing actually went wrong! So I'm assuming it shouldn't even be there :man_facepalming:
Anyway, in that case I think the only arg we need to worry about is synced_folder_type
versus :type
from the additional arguments. Not sure what we should do about synced_folder_args
, but maybe it makes sense to give that arg a deprecation warning inside the validation method :man_shrugging:
@briancain, i see. that makes sense :+1:
I encountered the lack of possibility to specify finer grained synced_folder options to the puppet provisioner (but i think this could be relevant to the chef provisioner as well).
Currently i am able to specify
synced_folder_type
which does not let me specify neither thenfs_version
in case i want to use nfsv4 nor thenfs_export
option, which i need to set to false because i'm running on NixOS and i'm handling exports through NixOS configuration that is leading to /etc/exports not beeing writeable.I propose to replace the configuration setting
synced_folder_type
with a settingsynced_folder_opts
which could hold a hash of synced_folder configuration options that will be merged into thefolder_opts
of the relevant provisioner plugins.P.S. i already have a patched solution for my puppet provisioner needs and am willing to create a PR for chef and puppet provisioner plugins.