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

incus/network: add dynamic command line completions #368

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

adamcstephens
Copy link
Contributor

@adamcstephens adamcstephens commented Jan 5, 2024

The shell handles the filtering for you, but we could try and filter by remote name for example in class someone hits tab again.

I'll probably finish building out network to see if I can just reuse this function or the bulk of it, but this is a start. Done.

Example

─❯ incus network show <TAB>
bond0    fast1           incus1:fast0  incus1:oob0      oob0
bridge0  incus1:bond0    incus1:fast1  incus1:vlan2010  vlan2010
fast0    incus1:bridge0  incus1:lo     lo

One can also explore these directly

─❯ go run ./cmd/incus __complete network show ""
incus1:lo
incus1:oob0
incus1:bond0
incus1:bridge0
incus1:vlan2010
incus1:fast0
incus1:fast1
lo
oob0
bond0
bridge0
vlan2010
fast0
fast1
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

@adamcstephens adamcstephens changed the title incus/network/show: add dynamic completions incus: add dynamic command line completions Jan 5, 2024
@adamcstephens adamcstephens marked this pull request as ready for review January 8, 2024 02:49
@adamcstephens adamcstephens changed the title incus: add dynamic command line completions incus/network: add dynamic command line completions Jan 8, 2024
@adamcstephens
Copy link
Contributor Author

This is far from a full set of dynamic completions, but this is a good start and handles all of the basic network subcommands and config show (as a proof of concept that the functions can be reused).

Marking as ready to review so I can get some feedback.

cmd/incus/completion.go Outdated Show resolved Hide resolved
@stgraber
Copy link
Member

stgraber commented Jan 8, 2024

Looking pretty good. Just the hardcoded local which should be replaced by the variable for the default remote.

As someone who runs with well over a dozen different remotes, a bunch of those being clusters with hundreds of instances and some of which requiring VPNs, I don't know that completing across all remotes is necessarily the right thing to do though :)

It may make more sense to only complete over the current default remote and motivate folks to incus remote switch as needed if they expect to do a bunch of interactions against a specific remote.

Signed-off-by: Adam Stephens <adam@valkor.net>
@adamcstephens
Copy link
Contributor Author

adamcstephens commented Jan 8, 2024

Thanks for the feedback. I was a bit worried about the performance impact of iterating all the remotes, but hadn't thought of a better way. Now that I understand how this all works a bit better though, you inspired me to a nicer solution.

The changes I just pushed switch it so that if you haven't typed a remote (no colon in the entry), it'll suggest all the default resources plus all the available remote names. Once you have typed the colon (or tab completed it), and hit tab again, it will query the named remote for the resources. This should provide the benefit of not hitting endpoints until requested, but still allowing for tab completion for those remotes.

@adamcstephens
Copy link
Contributor Author

With an example:

─❯ ~/go/bin/incus network attach <TAB>
bond0    fast0  incus1:  local:  test1
bridge0  fast1  lo       oob0    vlan2010
─❯ ~/go/bin/incus network attach incus1:<TAB>
incus1:enp5s0  incus1:incusbr0  incus1:lo

@stgraber
Copy link
Member

stgraber commented Jan 8, 2024

Oh neat, that's definitely the best of all worlds!

@stgraber stgraber merged commit 288d5fb into lxc:main Jan 8, 2024
25 checks passed
@stgraber
Copy link
Member

stgraber commented Jan 8, 2024

Thanks, very cool work!

@adamcstephens adamcstephens deleted the dynamic-migrations branch January 8, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants