-
Notifications
You must be signed in to change notification settings - Fork 26
Kibble cli init #89
Kibble cli init #89
Conversation
@skekre98 it would be awesome to rewrite it to using click 👍 |
|
Also the click library should be added to required dependencies in Adding this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current changes don't work. Please see the comments. Once fixed it should look good!
➜ kibble
Usage: kibble [OPTIONS] COMMAND [ARGS]...
A simple command line tool for kibble
Options:
--help Show this message and exit.
Commands:
setup starts the setup process for kibble
version displays the current kibble versio
|
Hi @turbaszek, how are you building the actual cli? I was testing by running |
Here are basic information: You need to install |
|
@turbaszek apologies for the delay. I've been able to successfully migrate |
|
This is the current output with the cli: (home) Sharvils-MacBook-Pro:kibble sharvilkekre$ kibble
Usage: kibble [OPTIONS] COMMAND [ARGS]...
A simple command line tool for kibble
Options:
--help Show this message and exit.
Commands:
setup starts the setup process for kibble
version displays the current kibble version
(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ kibble version
1.0.0dev
(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ kibble setup
Welcome to the Apache Kibble setup script!
Elasticsearch: elasticsearch:9200
~/projects/kibble/kibble/api/yaml/kibble.yaml already exists! Writing to ~/projects/kibble/kibble/api/yaml/kibble.yaml.tmp instead
Writing Kibble config to ~/projects/kibble/kibble/api/yaml/kibble.yaml.tmp
All done, Kibble should...work now :)This is currently only missing |
setup.py
Outdated
| "python-dateutil==2.8.1", | ||
| "PyYAML==5.3.1", | ||
| "tenacity==6.2.0", | ||
| "click==7.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this list in alphabetical order 👌
|
@skekre98 could you please rebase? |
kibble/__main__.py
Outdated
| save_config( | ||
| mlserver=conf.get("mail", "mailhost"), | ||
| conn_uri=conn_uri, | ||
| dbname=conf.get("elasticsearch", "dbname"), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup command should do much more than this (especially that after #83 we can abandon saving the config). The setup command should perform whole logic of:
https://github.com/apache/kibble/blob/6959f3c51a594941abc08face115cf3057aaed88/kibble/setup/setup.py#L233-L276
Also it should parse the arguments using click not argparse - that's the main point of using click 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so is it taking in more arguments than what is in kibble.ini?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argparser is implementing a cli. But we would like to migrate to click as it's easier to use. So we need to preserve whole logic of this argument parser (cli):
https://github.com/apache/kibble/blob/6959f3c51a594941abc08face115cf3057aaed88/kibble/setup/setup.py#L41-L87
So we need to use @click.option decorators for every option in existing cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh got it. So we are not trying to use KibbleConfigParser? Apologies for the misunderstanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skekre98 we are using it to set default values like that:
https://github.com/apache/kibble/blob/6959f3c51a594941abc08face115cf3057aaed88/kibble/setup/setup.py#L47
We don't have to create KibbleConfigParser instance every time we want to access it. It's enough to use the conf object:
https://github.com/apache/kibble/blob/6959f3c51a594941abc08face115cf3057aaed88/kibble/configuration.py#L32-L33
|
Hey @skekre98 any update on this one? |
|
Hi @turbaszek, this is still in progress. I have been caught up with work the past few days. |
|
Got it, I've noticed this PR has quite a lot of fluff so I'm going to go ahead and close this one and open a new one with the correct changes. Apologies for the misunderstanding 😅 |
I have currently initialized the cli with
versionandsetup. I have currently left the setup function open since I believe the set up workflow is a work in progress. Let me know how you'd like me to handle this and what else to build out.