LXC: Improve configuration key validation and add missing completions for lxc config unset#14584
Merged
tomponline merged 5 commits intocanonical:mainfrom Dec 13, 2024
Merged
Conversation
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
5bdb1ac to
e3d591c
Compare
simondeziel
reviewed
Dec 4, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
e3d591c to
3d3e39b
Compare
simondeziel
previously approved these changes
Dec 4, 2024
simondeziel
reviewed
Dec 4, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
3d3e39b to
405119e
Compare
simondeziel
previously approved these changes
Dec 4, 2024
kadinsayani
changed the title
lxc config unsetlxc config unset
tomponline
reviewed
Dec 5, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
3 times, most recently
from
058864c to
9592285
Compare
simondeziel
reviewed
Dec 5, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
2 times, most recently
from
46078e5 to
d325f4c
Compare
simondeziel
reviewed
Dec 5, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
d325f4c to
6694a60
Compare
kadinsayani
changed the title
lxc config unsetlxc config unset and improve configuration key validation
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
3 times, most recently
from
14d5b9e to
9629d66
Compare
simondeziel
reviewed
Dec 5, 2024
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
4 times, most recently
from
776e643 to
8f2ea5c
Compare
tomponline
reviewed
Dec 10, 2024
tomponline
reviewed
Dec 10, 2024
tomponline
reviewed
Dec 10, 2024
tomponline
requested changes
Dec 10, 2024
Member
tomponline
left a comment
tomponline
left a comment
There was a problem hiding this comment.
I'm afraid i didn't get on well with reviewing this PR.
I had a hard time following the changes. Please could you expand the comments, commit descriptions and potentially refactor the code so its clearer to follow.
Thanks
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
46ee1ba to
81c1216
Compare
Member
Author
I've refactored the code, expanded the use of comments, and reworded the commit messages and descriptions. |
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
2 times, most recently
from
0953121 to
fc863b9
Compare
Member
Author
|
Marking as draft until the tests are passing and code is ready for re-review. |
This commit introduces usage validation for prefixed configuration keys, ensuring that container specific prefixed keys are not set for VMs and valid keys include a subkey. Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
2 times, most recently
from
5ae4426 to
8273775
Compare
…nce type Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
…r validation in `ConfigKeyChecker` Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
…ssage returned by `ConfigKeyChecker` Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
8273775 to
66806e2
Compare
kadinsayani
changed the title
lxc config unset and improve configuration key validationlxc config unset
kadinsayani
force-pushed
the
user-config-key-completion-fix
branch
from
66806e2 to
e6a2caa
Compare
tomponline
reviewed
Dec 12, 2024
tomponline
reviewed
Dec 13, 2024
tomponline
approved these changes
Dec 13, 2024
Member
|
@kadinsayani I think this PR caused a regression: https://github.com/canonical/lxd-ci/actions/runs/12367790805/job/34516721509#step:11:93 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14570.
This PR reworks configuration key validation in
validConfigKey, inlxd/instance/instance_utils.go. The intent of the first two commits is to:user.*andlinux.sysctl.*; andFor example, when attempting to set
linux.sysctl.*for a VM instance, a meaningful error message should be shown indicating the key is not valid for VMs, rather than the genericUnknown configuration key: <key>.This PR also expands the use of the new slices
instancetype.ConfigKeyPrefixesAnyandinstancetype.ConfigKeyPrefixesContainerto contextually provide completions for prefixed config keys when runninglxc config unset, based on instance type.