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

Splitting VVV code in Vagrantfile into classes #2641

Closed aldavigdis closed 6 months ago

aldavigdis commented 1 year ago

This is an unusually large commit that should untangle some of the additional Ruby code run by the Vagrantfile, before the Vagrant.configure block is run.

The following classes were created:

Caveats:

Bugfixes:

To be tested:

Checks

aldavigdis commented 1 year ago

I removed the Rubocop linter Github action in a separate commit. It would be great to get assistance with that part - or we could simply skip it for the time being.

aldavigdis commented 1 year ago

A rubocop Github action has been introduced again, using a shell line this time.

Mte90 commented 1 year ago

I still need to test it but I have doubts about the folder structure. As we are working/thinking on a custom launcher and docker support a generic lib folder with just the stuff for vagrantfile can create confusion. It is better to move it in a subfolder like vagrant in the lib folder.

aldavigdis commented 1 year ago

@Mte90: One of the aims here is to facilitate being able to end up with a more portable codebase that could be used in a different context, such as pre-loading things for Docker provisioning in the future, while keeping things such as configuration management, migrations and the general look and feel.

Replacing calls to class methods in the Vagrant Ruby module, such as Vagrant::Util::Platform.terminal_supports_colors and Vagrant.has_plugin? could (and should in my mind) be done as a separate task, and as many of those methods are proving unreliable already, I will look into that as soon as this has been tested and accepted.

It was @tomjn who suggested adding a .vvv subdirectory and it is my understanding was to underline that the intention is to refactor the code away from being Vagrant-specific.

The .vvv/lib directory is a Ruby convention and may enable us to keep specific code for, say, Vagrant (as .vvv/vvv-vagrant.rb) and Docker (as .vvv/vvv-docker.rb) one directory level higher up than the more generic class definitions. (A separate readme file and other documentation specific to the Ruby code could also live there.) -- It could also help with turning the code into a separate Ruby gem if we want to go that route.

I'd rather suggest removing the dot from the .vvv directory name to make it visible.

I understand that this is a large changeset but the intention for the time being is not to introduce breaking changes in this PR while making it a foundation for future portability and maintainability, which I'll be happy to help with as long as we're on the same page in general, as I've been aware of the need to move away from virtualisation to containers for a couple of years.

Cleaning up what's inside the Vagrant.configure block is then going to be a different construction site on top of all this, which I will also look into.

Mte90 commented 1 year ago

The code is perfect as it is but the problem to me is the code organization just it. We are working to improve Docker (without using Vagrant) and it will be bash stuff and the custom launcher will be in golang probably so creating a generic lib folder inside .vvv can create confusion. VVV is a suite and in our plans is to create a unique tool that you can execute in different ways based on your needs. For this i was suggesting .vvv/vagrant/lib as the folder in this way we can avoid any confusion for the future, just it.

Because who will use Docker don't need ruby and probably in the future neither Vagrant as they are migrating internally to pure golang implementation (now various things are supported like for the Vagrantfile). So it is just that, I don't know what think @tomjn about it but as I think that is better to do that code organization now and not in due years as example when we have issues as this project is not young and we have often to keep retro compatibility, so if we can just change right now will simplify.

tomjn commented 1 year ago

I actually like that .vvv won't be visible, and I'd like us to move the provision and a lot of the config folder into it, as well as the dashboard and PHPCS.

In an ideal world, a user opens the VVV folders and all they see are their sites.

As for moving away from vagrant, I think it'll always be there, the vagrant + virtualbox combination has served us well. What doesn't serve us well is a monstrous long vagrantfile. So perhaps moving the ruby code down from lib to rb/lib or vagrant/lib makes sense. It's possible in the future we may want to bundle a provider or plugin directly to save steps for users.

For the planned golang CLI tool, I still expect that to call vagrant commands internally for most users. It may be that for some providers it bypasses it entirely, and it'll do extra things that vagrant itself doesn't do. One hope is to detect common vagrant error messages we know about and offer a more helpful message or auto-fix it if we can.

aldavigdis commented 1 year ago

So, any concrete suggestions about where to place this?

Naming things is not something I'm good at, but perhaps referring to this as a library/gem then and galling it something like vvv-vagrant-bootstrap or something that fits into the jargon would be a good idea?

What about .vvv/vvv-vagrant-bootstrap/?

Mte90 commented 1 year ago

To me .vvv/vagrant/lib it is better as in this way is just for vagrant and right now is only the vagrantfile as the only thing we can add is like an extension package in that folder. So I think that is better to keep it simple after all in vagrant the only you can do is the vagrantfile or an extension.

tomjn commented 1 year ago

+1 to a .vvv/vagrant subfolder, I have no preferences on the sub-folders inside it so you have free reign to do as you see fit

tomjn commented 1 year ago

This job failure looks like it needs work:

https://github.com/Varying-Vagrant-Vagrants/VVV/actions/runs/3427707635/jobs/5711040160

When provisioning, things go in this order:

The "sources" clone the repos and pull down updates, but they don't run the provisioners, so there's a separation between execution and fetching/updating

GitHub
Splitting VVV code in Vagrantfile into classes · Varying-Vagrant-Vagrants/VVV@eb34890
An open source Vagrant configuration for developing with WordPress - Splitting VVV code in Vagrantfile into classes · Varying-Vagrant-Vagrants/VVV@eb34890
tomjn commented 1 year ago

I'm a little puzzled having read through the changeset why it didn't run the source provisioner for the extension in the clean provision, perhaps there's a bug in the workflow 🤔

aldavigdis commented 1 year ago

I'm a little puzzled having read through the changeset why it didn't run the source provisioner for the extension in the clean provision, perhaps there's a bug in the workflow 🤔

@tomjn: Perhaps there's a regression related to what I had considered to be a formatting or style issue. I'll have a look in the next couple of days in addition to anything else that you spotted.

Any chance to allow me to re-run those provisioners/jobs if I need to?

tomjn commented 1 year ago

@aldavigdis I'm not sure that's related to this PR, checks should auto-rerun if not poke me

aldavigdis commented 1 year ago

@tomjn I'll have a better look after the weekend. I was just assuming that some checks were simply assumed to fail.

tomjn commented 1 year ago

I'd like to revisit this, if only to take parts of it into develop, e.g. putting the sudo warning in a text file was a great idea

tomjn commented 9 months ago

@aldavigdis I was hoping to cherry pick some things out of this and keep the commit credits, particularly the rubocop/linter stuff, I can do that myself but it'll have my name on it :( do you want to do a follow up PR with just those changes?

I'm keen to move forward with the changes, but it's a bit much to do all at once all together