-
Notifications
You must be signed in to change notification settings - Fork 935
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
CLI: Rework cmpInstanceKeys
for contextual completions based on instance type
#14684
base: main
Are you sure you want to change the base?
CLI: Rework cmpInstanceKeys
for contextual completions based on instance type
#14684
Conversation
lxc storage volume
completion fixes and description updateslxc storage volume
completion fixes and description updates
1c67d9f
to
2f53f43
Compare
lxc storage volume
completion fixes and description updateslxc storage volume
completion fixes and command description updates
2f53f43
to
14d9643
Compare
lxc storage volume
completion fixes and command description updates463ac01
to
33c5c93
Compare
9a7d059
to
5495fc8
Compare
@@ -274,7 +274,7 @@ func (g *cmdGlobal) cmpInstanceKeys(instanceName string) ([]string, cobra.ShellC | |||
configKey = strings.TrimSuffix(configKey, "*") | |||
|
|||
// InstanceTypeAny config keys. | |||
if configKeyField.Condition == "" { | |||
if configKeyField.Condition == "" || configKeyField.Condition == "If supported by image" { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Is it possible to split this pr into two, so we can merge the non contentious parts? |
5495fc8
to
4307f76
Compare
Fixes canonical#14689. Signed-off-by: Kadin Sayani <[email protected]>
4307f76
to
fb65418
Compare
cmpInstanceKeys
for contextual completions based on instance type
Fixes #14689.
Summary of changes:
cloud-init.*
config keys.