CLI using Click

[naved001]

• Issue #431

• This is a rough draft/almost done (waiting on the switch_register client lib call).

• So far I have added all commands for node (adding more in new commits)

• My plan is to put all commands in this PR. That will make the PR huge, but since most of the pattern will be repeated so it might not be as difficult to review.

• In another PR I'll move the serve and serve_networks command to hil-admin and the entry_points mechanism. Edit: Moved serve_networks, serve, and create_admin_user to hil-admin, and also updated the entry point mechanism.

• Finally, right now we just dump the output of the api calls (see show_node), in some other PR we'll clean that output to make it human readable.

Collaborative effort with @xuhang57

[xuhang57]

Thanks and just a quick question. Should we call it newcli? I thought we are replacing the old cli.

[naved001]

you are right, ill delete the old file and rename this to cli.

[naved001]

looking at the diff is terrible right now because the whole file has changed. If it makes the review process difficult, I can change the name of the file (to make it look like a new file so it doesn't compare with the old version).

[xuhang57]

To summarize,

• hil node connect network <node> <network> <nic> <channel> vs.
• hil node network connect <node> <network> <nic> <channel>

I am fine with either way. We should make a decision and proceed.

Just one minor thing bugs me that: hil project node xxx hil node project xxx could be a bit confused on some occasions.

[naved001]

okay, now that I have split it up into different modules, I am going to address reviewers' comments.

[naved001]

I have added all commands now except for show_network_action. Any ideas how should it read?

[zenhack]

Why not just hil network-action show?

[naved001]

hil network-action show

cool, will do that. I'll put it in cli/misc.py

[xuhang57]

I have added all commands now except for show_network_action. Any ideas how should it read?

My thought on that is, this should be like hil version, which is a standalone command.

So we may group them together? Or we should put in the hil network?

for example, hil network show-action?

[xuhang57]

hil network-action show works too.

so if we have more than one more for objects or actions, we should use - to connect them.

[naved001]

no, networking-action command has little to do with the nework object. The version command would go in misc.py but Click had a nice way of implementing it by putting the decorator on top of my cli group.

[naved001]

@zenhack @xuhang57 I think this is ready for review.

Note:

The command for granting projects access to a network is in the network module, not project because that's how our api is network/<network>/project/<project>. If you guys agree, I can move it to project module.

[naved001]

I am just tempted to move the serve, serve_networks and create_admin_user commands to hil-admin script in this PR.

Edit: or not, Ill wait on reviewers before proceeding.

[naved001]

hil-admin runserver -p PORT already exists, then why did we implement hil serve PORT ?

Additional state checking before running the server I guess.

[xuhang57]

if hil help is more intuitive than hil --help, we could figure out a way that maps hil help to hil --help.

For reference, git help and git --help both bring up the usage of git CLI.

Also, if we want to make the hil CLI more intuitive, we should add more docstring, which gives the users more information about the CLI and we should have a good guide for the users to read. And that should suffice.

[jeremyfreudberg]

@xuhang57

for example, something like hil help network create and hil network create --help feel nearly equally valid, and I too have seen other CLIs which support both. (But keep in mind we can't actually use the string help as a positional argument in a command... for example hil network create help would be ambiguous... is help the name of some parameter (like network name) or the actual call for help.)

[xuhang57]

sure, and I tried it out using git:

(.venv) ➜ ubuntu@hil-dev  ~/naved/hil git:(click-cli) git fetch help
fatal: 'help' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

and using hil:

(.venv) ➜ ubuntu@hil-dev  ~/naved/hil git:(click-cli) hil network help
Usage: hil network [OPTIONS] COMMAND [ARGS]...

Error: No such command "help".

I would say click handles that ok?

But yeah, maybe making a subcommand with help in hil would work?

[xuhang57]

@SahilTikale as Sahil points out, we should make the CLI more intuitive. But I don't think we have a critical disadvantage of using click and we should be working together to make the CLI more institutive with using click.

[naved001]

whoever reviews this, please try running it too to get the feel of the CLI.

[naved001]

It feels really awkward to have the server-side commands in here. I think at some point (maybe now, but I could be convinced either way)

I'll do that in the next commit along with the changes you suggested for those commands.

[zenhack]

Quoting Lucas H. Xu (2018-03-14 22:11:36)

maybe we should do nic-name

Can't, because it's a python variable name. Fine though, as it's not part of the CLI's output.

[naved001]

Note to myself: Need to update the docs too since a lot of commands would have changed.

[xuhang57]

@naved001 It's up to you whether to have a CLI guide along with the docs or having two separate files. 👍

[naved001]

@xuhang57 I mean in the installation docs for developers and some place else we have a bunch of hil commands. That would need to be updated.

[naved001]

@zenhack @Izhmash @xuhang57 I think I have addressed your comments, it's ready for another pass.

[ianballou]

So I just learned that click automatically supports tab auto-completion. Do we want to add some documentation on how to allow it? All you need to do for HIL is add eval "$(_HIL_COMPLETE=source hil)" to .bashrc

Edit: actually looks like it would be better to use a separate file; apparently Git does something similar. http://click.pocoo.org/5/bashcomplete/#activation-script

[ianballou]

950 is merged now, so it's no longer blocking switch_register

[coveralls]

Pull Request Test Coverage Report for Build 1795

• 27 of 435 (6.21%) changed or added relevant lines in 11 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-4.9%) to 54.887%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
hil/cli/misc.py	0	6	0.0%
hil/cli/cli.py	0	20	0.0%
hil/commands/admin.py	27	49	55.1%
hil/cli/user.py	0	24	0.0%
hil/cli/project.py	0	30	0.0%
hil/cli/port.py	0	30	0.0%
hil/cli/client_setup.py	0	35	0.0%
hil/cli/network.py	0	38	0.0%
hil/cli/switch.py	0	43	0.0%
hil/cli/node.py	0	78	0.0%
<!--	Total:	27	435	6.21%	-->

Totals
Change from base Build 1788:	-4.9%
Covered Lines:	1999
Relevant Lines:	3642

---

💛 - Coveralls

[naved001]

@jeremyfreudberg I couldn't figure out how to always show optional arguments in usage (without typing --help). One alternative is to make those 2 optional arguments as positional arguments that aren't required.

(.venv) naved:~/click/hil$hil network create
Usage: hil network create [OPTIONS] NETWORK OWNER [ACCESS] [NET_ID]

so if they want to skip access, they can type '' for that. I think this is how the current cli handles this.

I think this is the only call that has 2 optional arguments.

[naved001]

@zenhack @xuhang57 @Izhmash addressed all your comments. Still need to add file for tab completion. And now it's passing travis checks too.

[xuhang57]

testing CLI is difficult. I don't know how we should approach this.

[naved001]

@zenhack @xuhang57

I have added some integration tests for the CLI. It's just one function doing a bunch of things.

• I could test all the remaining cli commands to it, if it doesn't feel like an overkill.
• Split that big function into multiple ones, but that would require me to use different objects in different functions because they could be run in parallel.

I'll wait for reviewers before I spend more time on that.

[naved001]

I feel like I should still include the calls for headnode functionality since it is still a part of the codebase.

[naved001]

@zenhack could you review this? Thanks!

[ianballou]

I'm happy with how this is now. After Travis is all set it'll get my +1

[naved001]

just noticed that in docs/USING.rst the curl commands with json body need to set the content-type to application/json which can be specified by -H "Content-Type: application/json". Don't know if I should include it in this PR since I am touching the file here, but then this PR is big as it is. I'll do whatever the reviewers are comfortable with. LMK.

[xuhang57]

I would recommend you just update that. It is CLI related so.

[zenhack]

Quoting Naved Ansari (2018-04-02 09:06:56)

Just for the convenience, so that I could quickly type the commands in the tests. hil('node metadata add', NODE1, 'metadata-label metadata-value') vs hil('node', 'metadata', 'add', NODE1, 'metadata-label', 'metadata-value'). If you think it's not the right way to do it, I don't mind changing it.

Yeah, I think it would be better to stick to one command argument per function argument.

[naved001]

I think it would be better to stick to one command argument per function argument.

@zenhack okay, changed it.

[zenhack]

LGTM, merged.