CLI Completion

[dandeandean]

CLI Refactor

This PR overhauls the CLI command creation and parsing. Each command is now a struct, and exists in a command tree under the CmdRoot node. The NewCommand function now takes place of the previous Get*Params methods. The runner for each command does the work.

Completion

A step forward for #26.
We can now use the well structured tree & walk it to provide completions. Currently we just hard code the command map, but that should be replaced with using the CmdRoot.SubCommands. We only include the base commands in the completion, so we do not reach out to the Kubernetes API for any information.

Quirks

• It is impossible to use a parents flag in a child command. We work around this in the schematics sub commands, but it's dubious.
• We don't actually register global flags everywhere, I've matched the current main behavior.
• We don't register the completion function as a YokeCommand, this is because we want to go into the completions before calling flag.Parse().
• I've also opted to to not go embed the command_help. This is because it includes a #, which broke it. I'd like to keep that so that users can simply run yoke complete >> ~/.bashrc.

Future Work

• Completion per function i.e. yoke blackbox <tab> would yield a list of flights in the cluster
• Completion for global flags
• Figure out a better way to share flags between parent/children
• Completion for aliases

[dandeandean]

@davidmdm It looks like this is going to be a very large refactor of the current model. This is still a heavy WIP, but I was wondering if you want to give this a once-over before I continue down this path? I've wrapped all of the commands in the new YokeCommand struct & added them to the root during the init funcs to form a tree of YokeCommands. I'd like to also roll in the main switch statement into a map[string]*YokeCommand.Run() call. This is all very heavily inspired by how Cobra works, but the well-structured commands are pretty much necessary for completions.

[davidmdm]

I think this is very cool.

And yes I agree that we need to build the command structure if we want to be able to inspect/build completions out of it.

One thing that I would love to avoid though if possible, is pushing flag variables into global state and depending on init functions.

Perhaps a command when created could return both their flagset and run functions?

Pseudo Go

type CommandFunc func(context.Context) error

func NewCommand(name string, aliases []string, builder func() (*flag.Flagset, CommandFunc)) Command {
  flagset, run := builder()
 return Command{
   Name: name,
   Aliases: aliases,
   Flagset: flagset,
   Run: runner,
  }
} 

And in this way, we can keep all state scoped to the commands themselves:

takeoffCmd := NewCommand("takeoff", []string{"apply", "up"}, func() (*flag.Flagset, CommandFunc) {
  var takeoffParams yoke.TakeoffParams
  flagset := flag.NewFlagset()
  
  // register flags
  
  return flagset, func(ctx context.Context) error {
    flagset.Parse()
    // validate params
    // run core logic
  }
})

What do you think?

[dandeandean]

I 100% agree about the global flag vars. I will start working on getting something like the above drafted up. The commands themselves will still be in the global scope, but this will be much more manageable when they're self contained.

[davidmdm]

Im afk and only on mobile but this is shaping up quite nicely!!!

I am gonna check out the PR tomorrow and do some QA!

But I am super stoked!

[dandeandean]

Great! I've updated the PR description with some of my notes. It's definitely still a work in progress, but I'm working on getting the tests in a better spot.

[dandeandean]

Okay, I'm feeling pretty good about this. One testing gap I'm seeing is the lack of tests focusing on using the CmdRunner in the main_test file, making changes to the test cluster. I think it would be a great thing to add, but not sure if you'd like to see that get rolled into this PR. I would say that's more of an E2E style test, but there's not really coverage from the CLI perspective to guard against regressions.

[davidmdm]

Yeah it's true that the flag to Param mapping functions haven't really been tested and that's an issue in and of itself.

The design of yoke is that everything is built around the yoke sdk or commander object.

So the tests call the sdk, and the cli is just a wrapper to invoke said sdk.

Same for the ATC, and same for any future extensions.

I don't know if I would fully rewrite the same sdk E2E tests but via the cli entrypoint or run function.

However perhaps eventually flag parsing functions should be tested.

For now this is awesome! And what's nice is that the flags are all preserved as before just moved within the command builder.

[davidmdm]

One ask for when you set this ready for review, please squash your commits (I don't particularly care if its one or multiple) and make sure they are prefixed with the component or package you were working on, in this case yoke:

so like:

yoke: implement completions command
yoke: add completions tests

Feel free to checkout out the git log for examples. Not all commits are equal but we do what we can.
This is because the yoke project produces multiple artifacts: yoke cli, atc and yokecd docker images, atc and yokecd installer wasm flights, public SDK for working with flights, and so on :)

[dandeandean]

Okay! I've given it a final pass. Feel free to tear this apart. If this PR is too large, I'd be happy to break it up into many.

[davidmdm]

Hey! If I'm taking a while getting to it, it's because I am prepping for a talk I am giving soon. I promise to get to it soon :)

[davidmdm]

On a cursory try, I tried to install the completions, and now when I double tab in bash, I don't get the expected output:

This is my full .bashrc:

export PS1='${PWD/*\//}$ '

# Please add the following to your bashrc file to enable tab completions

function _yoke() {
  COMPREPLY=($(yoke complete $COMP_LINE));
};

complete -F _yoke yoke

[dandeandean]

Bizarre. It seems almost like that inner most yoke call isn't getting the Env. I will try to duplicate with a minimal shell. I'm the mean time can you try just completely exiting your terminal emulator and restarting it?

[davidmdm]

And if I manually try to invoke the completions I still just get the bashrc file:

[dandeandean]

I did most of my testing in zsh with

autoload -Uz compinit
compinit

I was able to reproduce the issue with just a bash shell, so I can continue to dig into the issue.

[dandeandean]

@davidmdm It looks like we just needed to export the variable in the bash script. I also encountered a bash-specific bug where the commands completions were off by one. Not sure why zsh has been working this whole time. You'll have to edit the script in your bashrc from the new generated bash.

I was able to confirm the functionality using the following two methods :
a) $ yoke complete >> .bashrc
b) $ yoke complete >> ~/.bash_completion

[davidmdm]

That's awesome! It already works better for me!

I am gonna start doing a pass of the code soon. I already have ideas off the top of my head such as move Seek to be a method on command, etc.

I am also thinking we should ship with zsh support as well.

[dandeandean]

Sounds good; I'm open to any suggestions!

The zsh setup is the same as the bash one, except you need the following:

autoload -Uz compinit
compinit
...

I think anyone who has there Kubernetes completions working in zsh should have this already (The Kubernetes docs that mention it).

I can re-squash after I fix any feedback.