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.

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

CLI: Rework cmpInstanceKeys for contextual completions based on instance type #14684

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

CLI: Rework cmpInstanceKeys for contextual completions based on instance type #14684

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lxc/completion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
// cmpClusterGroups provides shell completion for cluster groups and their remotes.
// It takes a partial input string and returns a list of matching cluster groups along with a shell completion directive.
func (g *cmdGlobal) cmpClusterGroups(toComplete string) ([]string, cobra.ShellCompDirective) {
var results []string

Check failure on line 44 in lxc/completion.go

View workflow job for this annotation

GitHub Actions / Code 

Consider pre-allocating `results` (prealloc)
cmpDirectives := cobra.ShellCompDirectiveNoFileComp

resources, _ := g.ParseServers(toComplete)
Expand Down Expand Up @@ -105,7 +105,7 @@
return nil, cobra.ShellCompDirectiveError
}

var results []string

Check failure on line 108 in lxc/completion.go

View workflow job for this annotation

GitHub Actions / Code 

Consider pre-allocating `results` (prealloc)
for k := range member.Config {
results = append(results, k)
}
Expand Down Expand Up @@ -273,7 +273,7 @@
configKey = strings.TrimSuffix(configKey, "*")

// InstanceTypeAny config keys.
if configKeyField.Condition == "" {
if configKeyField.Condition == "" || configKeyField.Condition == "If supported by image" {
Copy link
Member

Choose a reason for hiding this comment

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

this looks fishy, whats it comparing to?

Copy link
Contributor Author

@kadinsayani kadinsayani Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The condition field for a cloud-init.* instance config key:

			"cloud-init": {
				"keys": [
					{
						"cloud-init.network-config": {
							"condition": "If supported by image",
							"defaultdesc": "`DHCP on eth0`",
							"liveupdate": "no",
							"longdesc": "The content is used as seed value for `cloud-init`.",
							"shortdesc": "Network configuration for `cloud-init`",
							"type": "string"
						}
					},

Alternatively, we could add all keys under the cloud-init map.

Copy link
Member

Choose a reason for hiding this comment

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

why are we needing to consider the Condition field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check the condition field to determine if a key is valid for instance type any, container, or virtual-machine.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so we really dont want to be comparing to arbitrary free-text values.

This seem super hacky to me.

Copy link
Contributor Author

@kadinsayani kadinsayani Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm in agreement with moving away from checking "condition" values.

But that would then be restricted to keys known about by the client, and not by the server leading to situations where the client may suggest something the server doesnt support or miss out on suggesting something it does support.

We can also consider checking the keys from the metadata API against instancetype.InstanceConfigKeysAny/VM/Container to deduce compatible instance types. Aren't the keys generated by lxd-metadata sourced from lxd/instance/instancetype/instance.go?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but remember lxc would be built against a list at a point in time, but the server maybe newer and have additional keys only in metadata api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about adding a "supported instance type" field to the metadata? I'm trying to avoid hard coding a list of accepted keys based on instance type in the client, since these may not line up with the server. Also, there are cases where a prefix contains keys supported by containers and VMs. For example, the migration prefix contains migration.incremental.memory which is only supported by containers, and migration.stateful which is only supported by VMs.

Copy link
Contributor Author

@kadinsayani kadinsayani Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Another consideration for adding an instance type field to config keys in the metadata is the potential usability improvements that could be achieved in lxd-ui. I don't use it much so I'm not sure if completions are served when setting profile or instance config keys, but having an instance type field in the metadata could facilitate smart completions for config keys in the ui.

Copy link
Contributor Author

@kadinsayani kadinsayani Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another option could be including the condition metadata string with the completion results in brackets. For example:

$ lxc config set c1 [TAB]
limits.cpu.pin_strategy (virtual machine)
cloud-init.network-config (If supported by image)
...

With this option, we effectively mirror the documentation and inform users of all available configuration keys and their corresponding condition (virtual machine only, unprivileged container, if supported by image, storage volumes managed by LXD, etc.)

cc. @simondeziel

configKeys = append(configKeys, configKey)
continue
}
Expand Down Expand Up @@ -510,7 +510,7 @@
return nil, cobra.ShellCompDirectiveError
}

var results []string

Check failure on line 513 in lxc/completion.go

View workflow job for this annotation

GitHub Actions / Code 

Consider pre-allocating `results` (prealloc)
for k := range instanceNameOnly.Devices {
results = append(results, k)
}
Expand Down Expand Up @@ -1182,7 +1182,7 @@
return nil, cobra.ShellCompDirectiveError
}

var configs []string

Check failure on line 1185 in lxc/completion.go

View workflow job for this annotation

GitHub Actions / Code 

Consider pre-allocating `configs` (prealloc)
for c := range profile.Config {
configs = append(configs, c)
}
Expand Down Expand Up @@ -1283,7 +1283,7 @@
return nil, cobra.ShellCompDirectiveError
}

var configs []string

Check failure on line 1286 in lxc/completion.go

View workflow job for this annotation

GitHub Actions / Code 

Consider pre-allocating `configs` (prealloc)
for c := range project.Config {
configs = append(configs, c)
}
Expand Down
Loading