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

Add ability to set vm instance name #514

Open
dalepgrant opened this issue Feb 21, 2025 · 3 comments
Open

Add ability to set vm instance name #514

dalepgrant opened this issue Feb 21, 2025 · 3 comments
Labels
enhancement New feature or request

Comments

@dalepgrant
Copy link

Summary

Currently when adding a new site to Trellis with Lima, if the new site is higher in the alphabet then Lima treats this as a new instance. This seems to be because Trellis CLI looks for the first name in the list of sites in development and this list is sorted alphabetically when it's created.

As a result, if you add a site beginning with 'a' to a Trellis setup that has sites beginning with 'b' or lower, you get a message:

VM does not exist for this project. Run trellis vm start to create it.

and the CLI proceeds to create a new instance.

Two options:

  • Change that approach,
  • Allow setting the instance name, falling back to the existing method.

I'm advocating for the latter because

  • having control gives people options in their use cases
  • it doesn't break backwards compat. If people run into this in future, they'd be able to solve it by specifying the old instance name.
  • I don't have a suggestion for the former (changing the approach).

This would probably be good as an option in trellis.cli.local.yml so that you don't always have to specify it in a flag.

Additional context

If for example you're using Trellis for multiple sites and you're naming your production servers, even if it's server1, server2 etc, it would make more sense to be able to refer to each instance by the server name rather than by whichever site is highest alphabetically on it. It would also help in instances where you have to have the instance name hardcoded, see Discourse for one example.

@dalepgrant dalepgrant added the enhancement New feature or request label Feb 21, 2025
@dalepgrant dalepgrant changed the title Add ability to set vm name Add ability to set vm instance name Feb 21, 2025
dalepgrant added a commit to dalepgrant/trellis-cli that referenced this issue Feb 24, 2025
@swalkinshaw
Copy link
Member

Two things:

  1. supporting a custom instance_name config option is good regardless 👍 so your PR will be good for that
  2. I don't think this is the best solution for this specific bug because the UX is still confusing

Correct me if this is wrong, but the workflow would be:

  1. have existing VM
  2. add new site which makes it first alpabetically
  3. runs vm start
  4. gets an error
  5. has to manually set instance_name in the config

Ideally we just solve this automatically which still allowing the custom instance_name.

My proposal:

  • generate an automatic instance name like we do now (maybe even just keep the "first" one logic)
  • write the instance name to a file in .trellis/lima (the CLI already writes an inventory file there)
  • use that file to lookup the instance name whenever we need it

Then once we've generated it the first time, it's stable regardless of any future config changes?

@dalepgrant
Copy link
Author

That does sound like a better approach. It's almost like you know what you're talking about! 🤪

So finding the instance name would then be, in order of priority:

  • a config file in .trellis/lima (as yet un-named)
  • trellis.cli.local.yml
  • autogenerated (existing "first" one logic, or maybe first site in wordpress-sites which might be more what devs would expect)

If either option 2 or 3 is used, the config file is written and can then be used from then on.

Putting my time management hat on: Are you happy for these to be split out? As in, do you see the ability to set an instance name as being dependent on it being written and fetched from a config file, or would two PRs be acceptable, one as a progression of the other?

@swalkinshaw
Copy link
Member

So finding the instance name would then be, in order of priority:

* a config file in `.trellis/lima` (as yet un-named)

* `trellis.cli.local.yml`

* autogenerated (existing "first" one logic, or maybe first site in wordpress-sites which might be more what devs would expect)

I think we could roll 1 and 2 together? So even if instance_name in trellis.cli.local.yml is set, we'd still write it to .trellis/lima/instance. It's not necessary, but it reduces one permutation basically.

Since the instance_name option is new, anyone using it would also have the instance name written to .trellis/lima/instance. They can't really get out of sync either since even if you changed the name in the CLI config, it wouldn't do anything until you re-created the VM (which would write to the instance file)

Separate PRs is good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants