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

Extend completion logic to include alias commands #738

Closed
ArcorPius opened this issue Apr 7, 2024 · 8 comments · Fixed by #755
Closed

Extend completion logic to include alias commands #738

ArcorPius opened this issue Apr 7, 2024 · 8 comments · Fixed by #755
Assignees
Labels
Easy Good for new contributors Feature New feature, not a bug
Milestone

Comments

@ArcorPius
Copy link

ArcorPius commented Apr 7, 2024

Required information

  • Distribution: Debian
  • Distribution version: 12 (Bookworm)
  • The output of "incus info" or if that fails:
    • Kernel version: 6.6.13+bpo-amd64
    • LXC version: N/A
    • Incus version: 6.0.0 (The Zabbly package)
    • Storage backend in use: btrfs

Issue description

Hi! and congratulations for this release!

During the 0.x cycle :

  1. incus shell would autocomplete as a command
  2. incus shell would autocomplete the instance name

Also, access to an instance was a tad quicker, especially with remote containers. But may be it's just me...

I am aware that this is now an alias.

Coming from the systemD-nspawn world, this was a very nice feature; very quick to get into an instance, much like machinectl shell <instance>.

Thank you for all the work! This software is awesome!

Steps to reproduce

  1. type : incus sh
  2. type : shell
  3. type : incus shell i
  4. type : instance

Information to attach

None.

@stgraber
Copy link
Member

stgraber commented Apr 7, 2024

We may be able to complete the alias names but with the new logic, we won't be able to complete the actual arguments for those.

@adamcstephens is that something you think you could sort out? Expanding the top level completion logic to also consider what's defined as aliases in the config.

@stgraber
Copy link
Member

stgraber commented Apr 7, 2024

The reason it worked before was that the logic was hard-coded in the hand written profile.

I'm actually surprised that we did that given that the user could have removed or modified that alias, breaking the completion logic.

@adamcstephens
Copy link
Contributor

Yes I can look into this.

@stgraber stgraber added this to the incus-6.1 milestone Apr 8, 2024
@stgraber stgraber added Feature New feature, not a bug Easy Good for new contributors labels Apr 8, 2024
@stgraber stgraber changed the title Autocompletion : incus shell <instance> Extend completion logic to include alias commands Apr 8, 2024
@slovdahl
Copy link

Really annoying that incus shell xx<TAB> has stopped working 😒 For quick local testing of various images the incus launch images:os/version followed by incus shell [first one or two chars of the random name]+[TAB] has been extremely handy.

So, what you're saying is that it's impossible to complete container names for the shell alias going forward? I hadn't even realized it wasn't a "normal" command.

@stgraber
Copy link
Member

It may be possible to special case it in the completion logic, so if the shell alias is at its default value, then complete the instance name, if it's been altered in any way, then don't do it.

@adamcstephens
Copy link
Contributor

adamcstephens commented Apr 13, 2024

I looked at this some last night. It may be possible to intercept completion requests for aliases and re-call the binary with the target completion. These aliases are handled outside the cobra command environment though so I don’t think we can use it at all.

@ArcorPius
Copy link
Author

Thank you!
Can't wait for the release of v6.1!

@slovdahl
Copy link

Much much appreciated that you were able to bring this back, thanks a lot! 🙇 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Good for new contributors Feature New feature, not a bug
Development

Successfully merging a pull request may close this issue.

4 participants