Varying-Vagrant-Vagrants / VVV

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

Lock CustomFile and custom main provisioners behind a flag #2571

Open tomjn opened 2 years ago

tomjn commented 2 years ago

Describe alternatives you've considered

We should follow composers lead and lock these features behind a config.yml flag so that they have to be explicitly turned on to work. Specifically, legacy features to replace the main provisioner or add custom hooks on triggers such as CustomFile, provision-custom.sh in the main provision folder, pre-provision.sh, files such as vagrant_up_custom etc in the homebin folder.

Additional context

Support for these features is only present for legacy reasons and because people are still using them. Removing them would require a VVV 4.0, but until then this should be dissuaded.

Ideally, VVV would notice these files exist, and show a clear warning message indicating that it found the file but will not use the file because permission has not been given, and that support is deprecated and going away in a future major version.

tomjn commented 2 years ago

Perhaps something like this:

deprecated:
  enable_customfile: false
  enable_vagrant_custom: false
  enable_pre_provision: false
  enable_provision_custom: false
tomjn commented 2 years ago

There are also per site custom files, I don't think this particular method is optimal for those, we'll want to circleback and evaluate how effective this is before touching that

Mte90 commented 2 years ago

So a first analysis on how to do the various implementations for tracking purpose:

Customfile for provisioners

On https://github.com/Varying-Vagrant-Vagrants/VVV/blob/develop/Vagrantfile#L153 there is a flag for this, we need to expose this in the config.yml

Customfile for vagrant

The lines that involve this are https://github.com/Varying-Vagrant-Vagrants/VVV/blob/develop/Vagrantfile#L677

Pre provisioners

To run before all the provisioners it's the case to do there https://github.com/Varying-Vagrant-Vagrants/VVV/blob/develop/provision/provision.sh#L49

Pre provision site

It's the case to act there after updating the git repository https://github.com/Varying-Vagrant-Vagrants/VVV/blob/develop/provision/provision-site.sh#L481

After provision site

Same file above but at the end before the file ending

tomjn commented 2 years ago

@Mte90 it may be that we've deliberately not documented site provisioner customfiles to dissuade people from using them. I'm not a fan of undoing that. In this case the documentation would be VVV telling you in the terminal in big yellow letters that it's turned off, deprecated, and you have to turn it back on by doing X Y Z, and a mention in the changelog that these options now have to be enabled. The last thing we want is to encourage people to do this.

Mte90 commented 2 years ago

As today for customfile there is already an alert so should be enough write in a different color. Also create the new parameter to remap to the old one so we keep the compatibility.

In the meantime I will work on the other part that is more easy about what to do.

I was thinking as parameters:

general:
   custom:
       customfile: false
       pre_provision: [path/to/sh]
       pre_provisioners: [path/to/sh]
       post_provision: [path/to/sh]

As I think that let users to execute stuff before/after provision should be something not legacy but part of the tool to adapt VVV to their usage.

tomjn commented 2 years ago

That would introduce new features though, we're trying to make it easier to get rid of these features and dissuade people from using them. They were put behind a deprecated: section to further reinforce that these are going away in the future and are only there for legacy reasons.

I know there's a very particular thing that you want to do in a post_provision file, but this is not the way to do it. You've proposed an X Y problem. Instead raise a new ticket for that, and keep in mind the thing you want to do is itself not a problem, but a proposed solution to another problem you have. If the phrasing of it can be worded as "how can I do this thing that fixes this problem" then you need to go one step further down.

tomjn commented 2 years ago

As I think that let users to execute stuff before/after provision should be something not legacy but part of the tool to adapt VVV to their usage.

Not like this, this approach is dangerous, that's why it's deprecated, and there are other ways to solve problems.