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

LXC: Improve configuration key validation and add missing completions for lxc config unset #14584

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kadinsayani
Copy link
Contributor

@kadinsayani kadinsayani commented Dec 4, 2024

Fixes #14570.

This PR reworks configuration key validation in validConfigKey, in lxd/instance/instance_utils.go. The intent of the first two commits is to:

  1. Validate keys that begin with a prefix, such as user.* and linux.sysctl.*; and
  2. Validate keys based on their instance type.

For 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 generic Unknown configuration key: <key>.

This PR also expands the use of the new slices instancetype.ConfigKeyPrefixesAny and instancetype.ConfigKeyPrefixesContainer to contextually provide completions for prefixed config keys when runninglxc config unset, based on instance type.

@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch from 5bdb1ac to e3d591c Compare December 4, 2024 18:47
lxc/completion.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch from e3d591c to 3d3e39b Compare December 4, 2024 19:01
simondeziel
simondeziel previously approved these changes Dec 4, 2024
lxc/completion.go Outdated Show resolved Hide resolved
lxc/completion.go Outdated Show resolved Hide resolved
simondeziel
simondeziel previously approved these changes Dec 4, 2024
@kadinsayani kadinsayani changed the title LXC: Add missing completions for lxc config unset CLI: Add missing completions for lxc config unset Dec 4, 2024
lxc/completion.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 3 times, most recently from 058864c to 9592285 Compare December 5, 2024 17:34
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 2 times, most recently from 46078e5 to d325f4c Compare December 5, 2024 20:00
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch from d325f4c to 6694a60 Compare December 5, 2024 20:25
@kadinsayani kadinsayani changed the title CLI: Add missing completions for lxc config unset CLI: Add missing completions for lxc config unset and improve configuration key validation Dec 5, 2024
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 3 times, most recently from 14d5b9e to 9629d66 Compare December 5, 2024 21:48
lxd/instance/instance_utils.go Outdated Show resolved Hide resolved
lxd/instance/instance_utils.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 4 times, most recently from 776e643 to 8f2ea5c Compare December 5, 2024 22:54
simondeziel
simondeziel previously approved these changes Dec 9, 2024
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

I've refactored the code, expanded the use of comments, and reworded the commit messages and descriptions.

@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 2 times, most recently from 0953121 to fc863b9 Compare December 10, 2024 16:18
@kadinsayani kadinsayani marked this pull request as draft December 10, 2024 17:19
@kadinsayani
Copy link
Contributor Author

kadinsayani commented Dec 10, 2024

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 <[email protected]>
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch 2 times, most recently from 5ae4426 to 8273775 Compare December 11, 2024 00:04
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch from 8273775 to 66806e2 Compare December 11, 2024 00:09
@kadinsayani kadinsayani changed the title CLI: Add missing completions for lxc config unset and improve configuration key validation LXC: Improve configuration key validation and add missing completions for lxc config unset Dec 11, 2024
@kadinsayani kadinsayani force-pushed the user-config-key-completion-fix branch from 66806e2 to e6a2caa Compare December 11, 2024 21:53
@kadinsayani kadinsayani marked this pull request as ready for review December 11, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-completion for unset ignores user.* configuration keys
3 participants