8000 Persistent vm naming by jasperf · Pull Request #524 · roots/trellis-cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Persistent vm naming #524

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Persistent vm naming #524

wants to merge 9 commits into from

Conversation

jasperf
Copy link
@jasperf jasperf commented Mar 8, 2025 •
edited
Loading

This pull request introduces several significant changes to the VM management and configuration handling within the Trellis CLI. The most notable updates include the addition of a new instance name management system, the centralization of VM manager creation logic, and the enhancement of testability for VM-related commands.

Key changes:

VM Instance Management:

  • Added InstanceName field to VmConfig struct in cli_config/cli_config.go to allow custom naming of VM instances.
  • Implemented GetVMInstanceName and SaveVMInstanceName methods in trellis/vm_instance.go to manage VM instance names with a priority system.
  • Updated VmStartCommand to use GetVMInstanceName for setting the VM name and saving it for future reference. [1] [2]

Centralization and Testability:

  • Centralized VM manager creation logic in cmd/main.go to avoid duplicate implementations and enable easier mocking in tests.
  • Removed redundant newVmManager function from cmd/vm.go and updated references to use the centralized version.

Testing Enhancements:

  • Added tests for the new instance name management methods in trellis/vm_instance_test.go.
  • Enhanced VmDeleteCommand and VmStartCommand tests to verify instance file handling and VM name resolution. [1] [2]

These changes improve the flexibility, maintainability, and testability of the Trellis CLI's VM management functionality.

VM Instance Name Resolution

The VM creation and start processes now follow a structured approach to determine which instance name to use. This is a significant improvement over the previous implementation which always defaulted to the first site alphabetically.

How VM Name Resolution Works Now

Priority Order for VM Name Resolution
The new code in trellis/vm_instance.go implements a priority-based approach:

  1. First, check for a saved instance name in .trellis/lima/instance file
  2. If not found, check for a configured instance name in CLI config
  3. Only if both above methods fail, fall back to using the first WordPress site alphabetically

VM Start Process

When running trellis vm start:

  1. The command calls GetVMInstanceName() instead of directly using MainSiteFromEnvironment()
  2. GetVMInstanceName() first tries to read from the instance file
  3. If that file exists and contains a name (like imagewize.com), it will use that name
  4. The VM start command will then use that name to start the VM
    This ensures that once you've created a VM for a specific site, subsequent vm start commands will consistently use the same VM, rather than creating or starting a different one based on alphabetical order.

VM Creation Process

When creating a new VM:

  1. If no VM exists, the code follows the same name resolution process
  2. After creating the VM, it saves the instance name to .trellis/lima/instance file using SaveVMInstanceName()
  3. This file persists the association between your project and a specific VM name

Benefits
This solves your specific issue where vm start was always using "blocks.imagewize.com" instead of "imagewize.com":

  1. If you've previously created a VM with imagewize.com, that name is saved in the instance file
  2. Future vm start commands will read this file first rather than defaulting to alphabetical order
  3. Even if blocks.imagewize.com comes first alphabetically, the saved instance name will be preferred

This provides consistency and predictability when managing VMs across multiple sites in a Trellis project.

@jasperf jasperf closed this Mar 11, 2025
@jasperf jasperf reopened this Mar 11, 2025
swalkinshaw
swalkinshaw reviewed May 19, 2025
View reviewed changes
Copy link
Member
@swalkinshaw swalkinshaw left a comment

Choose a reason for hiding this comment

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

Sorry forgot about this after #515.

Can it be rebased and updated based on that?

Also I assume AI was used to help with most of this? Not against that, but it generates a ton of useless code comments.

.gitignore
@@ -1,3 +1,4 @@
trellis-cli
dist
tmp
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

This should only exist on user's personal global gitignore and not here

cmd/main.go
Comment on lines +14 to +17
// This file provides centralized declarations of functions as variables
// to enable mocking in tests and prevent duplicate implementations.
// Previously, these functions were defined in multiple places, causing
// "redeclared in this block" errors during compilation.
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid all these changes for now? This is a whole new pattern that I'm not sure about yet

I'm okay if this limits the testability temporarily

@jasperf
Copy link
Author
jasperf commented May 20, 2025 •
edited
Loading

Need to think about this pull request. Also kind of forgot this since last improvement in pull request #515 which got merged. This pull request will add some additional functionality now like

After creating the VM, it saves the instance name to .trellis/lima/instance file using SaveVMInstanceName()

but the vm naming can be set already by adding name to trellis/trellis.cli.yml like

# Trellis CLI example config file
# https://roots.io/trellis/docs/cli/#configuration
#
# database_app: sequel-ace
# open:
#   admin: "https://mysite.com/wp/wp-admin"
vm:
  instance_name: "imagewize.com"

and yes, did get AI help here and if we go through I would need to clean up stuff some more and remove some user only stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swalkinshaw swalkinshaw swalkinshaw left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasperf @swalkinshaw
0