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

lxd: select server based on name from result #181

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

thp-canonical
Copy link
Contributor

@thp-canonical thp-canonical commented Feb 22, 2024

Due to prefix or regular expression matching, lxd list may list multiple instances. To account for that, instead of assuming it will always return a slice where the first item is the one we're interested in, iterate over the result and pick the one that matches the name exactly. Return an error If no such match exists.

According to lxd documentation, the argument to the list command can be:

  • A prefix of the instance name
  • A regular expression on the instance name
  • A key/value pair referring to a configuration item
  • A key/value pair with a shorthand key
  • A regular expression matching a config item

Copy link

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

@cmatsuoka cmatsuoka changed the title fix(lxd): Select server based on name from result (fixes #151) lxd: select server based on name from result Jan 28, 2025
@cmatsuoka cmatsuoka requested a review from niemeyer January 28, 2025 12:09
@cmatsuoka cmatsuoka added the Reviewed Supposedly ready for tuning or merging label Jan 28, 2025
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

We might just format the name so it does match exactly, but that seems fine too.

@niemeyer niemeyer merged commit 413817e into canonical:master Jan 28, 2025
1 check passed
@niemeyer
Copy link
Contributor

Thanks @thp-canonical and @cmatsuoka.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Supposedly ready for tuning or merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants