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

Jump to bottom

lxc: Add completions for server keys and console #14115

Merged
merged 3 commits into from
Sep 27, 2024
Merged

lxc: Add completions for server keys and console #14115

merged 3 commits into from
Sep 27, 2024

Conversation

kadinsayani
Copy link
Contributor

Resolves #14098.

All the improvements made to the lxc config get/set completions have been tested locally. I made a minor change that deviates from the requirements outlined in #14098. Instead of typing lxc config get foo:core.<tab> to retrieve server configuration options, typing lxc config get foo: core.<tab> (notice the [SPACE]) will now return server configuration options. This change was implemented to differentiate from the conventional usage of remote:instance across the CLI. However, please let me know if you'd prefer server configuration completions without the [SPACE] 😄.

@kadinsayani kadinsayani marked this pull request as ready for review September 16, 2024 19:57
@simondeziel
Copy link
Member

simondeziel commented Sep 16, 2024 •
edited
Loading

I made a minor change that deviates from the requirements outlined in #14098. Instead of typing lxc config get foo:core.<tab> to retrieve server configuration options, typing lxc config get foo: core.<tab> (notice the [SPACE]) will now return server configuration options. This change was implemented to differentiate from the conventional usage of remote:instance across the CLI. However, please let me know if you'd prefer server configuration completions without the [SPACE] 😄.

That was the right thing to do as I got the example in the bug wrong, thanks for catching/fixing this!

Update: I fixed the issue description accordingly.

simondeziel
simondeziel reviewed Sep 16, 2024
View reviewed changes
lxc/completion.go Outdated Show resolved Hide resolved
simondeziel
simondeziel previously approved these changes Sep 16, 2024
View reviewed changes
markylaing
markylaing reviewed Sep 17, 2024
View reviewed changes
lxc/completion.go Outdated Show resolved Hide resolved
markylaing
markylaing reviewed Sep 17, 2024
View reviewed changes
lxc/completion.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch from 9019fd7 to 22b90b6 Compare September 17, 2024 19:16
@kadinsayani
Copy link
Contributor Author

kadinsayani commented Sep 17, 2024 •
edited
Loading

I’ve updated the cmpServerAllKeys() method to use the metadata API by calling GetMetadataConfiguration(). I believe it would be more efficient to create a MetadataConfiguration struct in the /shared/api directory. This way, we can avoid parsing the response in the completions.go file in polynomial time. Instead, we can unmarshal the necessary information to a nested struct. I'm not 100% sure how much it would improve performance, but it would definitely be more elegant. Thoughts?

Also, @simondeziel, you were right all along about strings.Cut() 😄.

@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch from 22b90b6 to 8b34b92 Compare September 17, 2024 19:33
simondeziel
simondeziel reviewed Sep 17, 2024
View reviewed changes
client/lxd_server.go Outdated Show resolved Hide resolved
simondeziel
simondeziel reviewed Sep 17, 2024
View reviewed changes
client/interfaces.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch 3 times, most recently from ca94bd8 to 42c572e Compare September 17, 2024 21:05
markylaing
markylaing previously requested changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

I think it's fine to create a type in the shared/api package for the configuration metadata. It is exposed over the API and would break the UI if changed, so we are beholden to it.

client/lxd_server.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch from 42c572e to 286a06d Compare September 20, 2024 14:51
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Sep 20, 2024
@kadinsayani
Copy link
Contributor Author

kadinsayani commented Sep 20, 2024 •
edited
Loading

@markylaing I rebased my local branch on your PR branch #14132. Is this okay, or should I wait for your PR to be merged and then rebase on main?

@tomponline
Copy link
Member

@kadinsayani should we also import lxc/incus#1231 ?

@kadinsayani
Copy link
Contributor Author

@kadinsayani should we also import lxc/incus#1231 ?

Sure - thanks for finding this!

@kadinsayani kadinsayani changed the title lxc: Add completions for server keys lxc: Add completions for server keys and console Sep 23, 2024
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch from 55689bd to 759d849 Compare September 23, 2024 17:44
@github-actions github-actions bot removed Documentation Documentation needs updating API Changes to the REST API labels Sep 23, 2024
@kadinsayani
Copy link
Contributor Author

@markylaing I rebased my local branch on your PR branch #14132. Is this okay, or should I wait for your PR to be merged and then rebase on main?

I reverted this to avoid conflicts.

@tomponline
Copy link
Member

tests failing

@kadinsayani
Copy link
Contributor Author

tests failing

Awaiting merge of #14132 to latest/edge.

@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch 2 times, most recently from d7f8817 to 2c0f521 Compare September 26, 2024 14:35
@kadinsayani
Copy link
Contributor Author

Rebased and good to go 👍

tomponline
tomponline previously approved these changes Sep 26, 2024
View reviewed changes
simondeziel
simondeziel previously approved these changes Sep 26, 2024
View reviewed changes
lxc/completion.go Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch 2 times, most recently from 7f08d58 to 6e21640 Compare September 26, 2024 17:11
simondeziel
simondeziel reviewed Sep 26, 2024
View reviewed changes
lxc/completion.go Outdated Show resolved Hide resolved
kadinsayani and others added 2 commits September 26, 2024 12:23
@kadinsayani
lxc: Add completions for server keys
d317590 
Signed-off-by: Kadin Sayani <[email protected]>
@stgraber @kadinsayani
lxc/console: Add completion
2b39578 
Closes incus bug lxc/incus#1229.

Signed-off-by: Stéphane Graber <[email protected]>
(cherry picked from commit 00aba5d5679f4703f87e19dbe4738db098cceb3a)
Signed-off-by: Kadin Sayani <[email protected]>
License: Apache-2.0
@kadinsayani kadinsayani force-pushed the 14098-shell-completion-improvements branch from 6e21640 to 2b39578 Compare September 26, 2024 18:23
@tomponline tomponline merged commit b236cea into canonical:main Sep 27, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@simondeziel simondeziel simondeziel approved these changes

@markylaing markylaing markylaing left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shell completion improvement suggestions
5 participants
@kadinsayani @simondeziel @tomponline @markylaing @stgraber