Varying-Vagrant-Vagrants / VVV

An open source Vagrant configuration for developing with WordPress
https://varyingvagrantvagrants.org
MIT License
4.54k stars 848 forks source link

Fixing faulty Vagrant plugin check #2638

Closed aldavigdis closed 1 year ago

aldavigdis commented 1 year ago

This replaces some Vagrant.has_plugin? checks in the Vagrantfile with config.vagrant.plugins.include? as the latter is not available when the Vagrantfile is run in version 2.1.4 and newer of Vagrant.

There are other Vagrant.has_plugin? calls in the Vagrant file, but they don't seem to affect things in the same way. I can mend that in a separate PR, but I am not sure about the effectiveness, even if config.vagrant.plugins.include? seems more reliable.

See also: https://github.com/hashicorp/vagrant/issues/10161

Checks

aldavigdis commented 1 year ago

By the same logic, the Vagrant.has_plugin? calls in the unless statement in the block staring in line 376 will always return false, so VVV is always going to insist on vagrant-goodhosts being installed no matter if the other host management plugins are present -- and I think that is the case here as the line config.vagrant.plugins = ['vagrant-goodhosts'] gets run no matter which plugins are installed.

Perhaps it would be a better idea to simply remove support for other host management plugins?

tomjn commented 1 year ago

@aldavigdis I read through the vagrant issue and it seems Chris Roberts made a fix or to use a change similar to:

https://github.com/hashicorp/vagrant/issues/10161#issuecomment-417701063

In terms of policy, goodhosts is best maintained and we have some ties with the owners as @Mte90 built the vagrant plugin, hostsupdater is no longer maintained and hostsmanager has issues

aldavigdis commented 1 year ago

@tomjn We could consider this a quick fix to a specific issue, but it is tempting for me to streamline the Vagrantfile in a separate PR to reflect the state of hostsupdater and hostsmanager by removing references to them.

Is it okay if I create an issue for it and refer to it in a larger PR next week or so?

tomjn commented 1 year ago

@aldavigdis I know there are people who still use hostsupdater and hostmanager who would kick up a fuss about it, some because of issues with Windows and large numbers of hosts. A lot of the idiosyncracies in the vagrant file are related to rubycop

I do think it would be nice to move a lot of it out into separate files in a .vvv/vagrant/ subfolder, and make it a little more functional and OO, e.g. the mapped folders for example don't need re-declaring every single time for each folder, and for each provider, too much repetition

welcome[bot] commented 1 year ago

Congrats on merging your first pull request!

aldavigdis commented 1 year ago

I don't even think most rubocop setups would be too happy about the current Vagrantfile TBF and there is no .rubocop.yml file present as far as I can see, but I'll be happy to have a more comprehensive look at this in the next couple of days or weeks.