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

Splitting VVV code in Vagrantfile into classes #2641

Conversation

aldavigdis
Copy link
Contributor

@aldavigdis aldavigdis commented Nov 8, 2022

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.

  • Moving functionality related to VVV that is run before the Vagrant.configure block into a separate Ruby module and classes
  • The resulting classes and methods now follow the Ruby Style Guide
  • Implementing simple and reasonable Rubocop ruleset
  • Adding a Rubocop Github action
  • Adding a .ruby-version file indicating the Ruby version currently used by Vagrant (2.7.6)
  • Few if any functional changes have been introduced intentionally
  • Further code cleanup

The following classes were created:

  • VVV::Info - Fetches info on the platform and environment
  • VVV::Config - Reads and processes the config file
  • VVV::SplashScreens - Displays splash screens
  • VVV::Bootstrap - Indicates when to display splash screens
  • VVV::Migrate - Handles config file and database migrations

Caveats:

  • This covers what happens before Vagrant.configure
  • The cleanup is required to keep the Ruby code maintainable and to facilitate further development.

Bugfixes:

  • The non-functionning Vagrant.has_plugin? may be replaced by VVV::Info.vagrant_local_plugins and VVV::Info.vagrant_global_plugins

To be tested:

  • Config migration
  • Database migration
  • Splash screens
  • Config defaults
  • Config processing and backwards-compatibility
  • Github action for Rubocop
  • Different host platforms and architectures
    • Windows/Virtualbox
    • Windows/Hyper-V
    • Linux/Virtualbox
    • MacARM/Parallels (Needs assistance)

Checks

  • I've updated the changelog.
  • I've tested this PR
  • This PR is for the develop branch not the stable branch.
  • This PR is complete and ready for review.

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.

- Moving functionality related to VVV that is run before the
  `Vagrant.configure` block into a separate Ruby module and classes
- The resulting classes and methods now follow the Ruby Style Guide
- Implementing simple and reasonable Rubocop ruleset
- Adding a Rubocop Github action
- Adding a `.ruby-version` file indicating the Ruby version currently
  used by Vagrant (2.7.6)
- Few if any functional changes have been introduced intentionally
- Further code cleanup

The following classes were created:

- `VVV::Info` - Fetches info on the platform and environment
- `VVV::Config` - Reads and processes the config file
- `VVV::SplashScreens` - Displays splash screens
- `VVV::Bootstrap` - Indicates when to display splash screens
- `VVV::Migrate` - Handles config file and database migrations

Caveats:

- This covers what happens before `Vagrant.configure`
- The cleanup is required to keep the Ruby code maintainable and to
  facilitate further development.

Bugfixes:
- The non-functionning `Vagrant.has_plugin?` may be replaced by `VVV::Info.vagrant_local_plugins` and `VVV::Info.vagrant_global_plugins`

To be tested:

[ ] Config migration
[ ] Database migration
[ ] Splash screens and
[ ] Config defaults
[ ] Config processing and backwards-compatibility
[ ] Github action for Rubocop
[ ] Different host platforms and architectures
@aldavigdis
Copy link
Contributor Author

aldavigdis commented Nov 8, 2022

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
Copy link
Contributor Author

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

@Mte90
Copy link
Member

Mte90 commented Nov 9, 2022

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
Copy link
Contributor Author

@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
Copy link
Member

Mte90 commented Nov 9, 2022

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
Copy link
Member

tomjn commented Nov 9, 2022

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
Copy link
Contributor Author

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
Copy link
Member

Mte90 commented Nov 9, 2022

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
Copy link
Member

tomjn commented Nov 10, 2022

+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
Copy link
Member

tomjn commented Nov 12, 2022

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:

  • a provisioner to print the go make a coffee teddy bear
  • a quick provisioner to copy the config and version into the VM and one or two commands
  • main provisioner
  • tools provisioner
  • dashboard
  • extension sources
  • extensions
  • site sources
  • sites

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
An open source Vagrant configuration for developing with WordPress - Splitting VVV code in Vagrantfile into classes · eb34890

Comment on lines +41 to +44
process_utilities
process_utility_sources
process_extensions
process_extension_sources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the wrong way around

Suggested change
process_utilities
process_utility_sources
process_extensions
process_extension_sources
process_extension_sources
process_utility_sources
process_extensions
process_utilities

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though it is just linting so this isn't as important as it looked when I read it

Comment on lines +97 to +108
def self.vagrant_local_plugins
json_file_path = File.join(vagrant_dir, '/.vagrant/plugins.json')
return [] unless File.file?(json_file_path)

json_file = File.read(json_file_path)
json_hash = JSON.parse(json_file)

json_hash['installed'].keys
end

def self.vagrant_global_plugins
json_file_path = File.join(Dir.home, '/.vagrant.d/plugins.json')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in theory we can query vagrant itself for this but this seems more logical, my only concern is wether removing the plugins would actually change this since we aren't directly checking for the plugins. Likewise for vagrant go this may not be how that info is stored

@tomjn
Copy link
Member

tomjn commented Nov 12, 2022

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
Copy link
Contributor Author

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
Copy link
Member

tomjn commented Dec 6, 2022

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

@aldavigdis
Copy link
Contributor Author

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

@tomjn
Copy link
Member

tomjn commented Oct 1, 2023

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
Copy link
Member

tomjn commented Jan 3, 2024

@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

@aldavigdis aldavigdis closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants