You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kibble.apache.org by GitBox <gi...@apache.org> on 2020/11/03 13:42:24 UTC
[GitHub] [kibble] turbaszek commented on a change in pull request #89: Kibble cli init
turbaszek commented on a change in pull request #89:
URL: https://github.com/apache/kibble/pull/89#discussion_r516170129
##########
File path: setup.py
##########
@@ -38,6 +38,7 @@
"python-dateutil==2.8.1",
"PyYAML==5.3.1",
"tenacity==6.2.0",
+ "click==7.1.2",
Review comment:
Let's keep this list in alphabetical order 👌
##########
File path: kibble/__main__.py
##########
@@ -14,10 +14,104 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+import yaml
+import click
+from kibble.version import kibble_version
+from kibble.configuration import KibbleConfigParser
+
+from kibble.settings import KIBBLE_YAML
+
+KIBBLE_VERSION = "0.1.0" # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = KIBBLE_YAML
+ if os.path.exists(kibble_yaml):
+ print(f"{kibble_yaml} already exists! Writing to {kibble_yaml}.tmp instead")
+ kibble_yaml = kibble_yaml + ".tmp"
+ return kibble_yaml
+
+
+def save_config(mlserver: str, conn_uri: str, dbname: str):
+ """Save kibble config to yaml file"""
+ if ":" in mlserver:
+ try:
+ mailhost, mailport = mlserver.split(":")
+ except ValueError:
+ raise ValueError(
+ "mailhost argument must be in form of `host:port` or `host`"
+ )
+ else:
+ mailhost = mlserver
+ mailport = 25
+
+ if ":" in conn_uri:
+ try:
+ hostname, port = conn_uri.split(":")
+ except ValueError:
+ raise ValueError(
+ "conn_uri argument must be in form of `host:port` or `host`"
+ )
+ else:
+ hostname = elasticsearch
+ port = 98200
+
+ config = {
+ "api": {"version": KIBBLE_VERSION, "database": KIBBLE_DB_VERSION},
+ "elasticsearch": {
+ "host": hostname,
+ "port": int(port),
+ "ssl": False,
+ "dbname": dbname,
+ },
+ "mail": {
+ "mailhost": mailhost,
+ "mailport": int(mailport),
+ "sender": "Kibble <no...@kibble.kibble>",
+ },
+ "accounts": {"allowSignup": True, "verify": True},
+ }
+
+ kibble_yaml = get_kibble_yaml()
+ print(f"Writing Kibble config to {kibble_yaml}")
+ with open(kibble_yaml, "w") as f:
+ f.write(yaml.dump(config, default_flow_style=False))
+ f.close()
+
+
+@click.group()
+def cli():
+ """A simple command line tool for kibble"""
+
+
+@cli.command("version", short_help="displays the current kibble version")
+def version():
+ click.echo(kibble_version)
+
+
+@cli.command("setup", short_help="starts the setup process for kibble")
+def setup():
+ click.echo("Welcome to the Apache Kibble setup script!")
+ conf = KibbleConfigParser()
+ conf.read("kibble.ini")
+ conn_uri = conf.get("elasticsearch", "conn_uri")
+ click.echo(f"Elasticsearch: {conn_uri}")
+
+ # Create Kibble configuration file
+ save_config(
+ mlserver=conf.get("mail", "mailhost"),
+ conn_uri=conn_uri,
+ dbname=conf.get("elasticsearch", "dbname"),
+ )
Review comment:
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 👍
##########
File path: kibble/__main__.py
##########
@@ -14,10 +14,104 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+import yaml
+import click
+from kibble.version import kibble_version
+from kibble.configuration import KibbleConfigParser
+
+from kibble.settings import KIBBLE_YAML
+
+KIBBLE_VERSION = "0.1.0" # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = KIBBLE_YAML
+ if os.path.exists(kibble_yaml):
+ print(f"{kibble_yaml} already exists! Writing to {kibble_yaml}.tmp instead")
+ kibble_yaml = kibble_yaml + ".tmp"
+ return kibble_yaml
+
+
+def save_config(mlserver: str, conn_uri: str, dbname: str):
+ """Save kibble config to yaml file"""
+ if ":" in mlserver:
+ try:
+ mailhost, mailport = mlserver.split(":")
+ except ValueError:
+ raise ValueError(
+ "mailhost argument must be in form of `host:port` or `host`"
+ )
+ else:
+ mailhost = mlserver
+ mailport = 25
+
+ if ":" in conn_uri:
+ try:
+ hostname, port = conn_uri.split(":")
+ except ValueError:
+ raise ValueError(
+ "conn_uri argument must be in form of `host:port` or `host`"
+ )
+ else:
+ hostname = elasticsearch
+ port = 98200
+
+ config = {
+ "api": {"version": KIBBLE_VERSION, "database": KIBBLE_DB_VERSION},
+ "elasticsearch": {
+ "host": hostname,
+ "port": int(port),
+ "ssl": False,
+ "dbname": dbname,
+ },
+ "mail": {
+ "mailhost": mailhost,
+ "mailport": int(mailport),
+ "sender": "Kibble <no...@kibble.kibble>",
+ },
+ "accounts": {"allowSignup": True, "verify": True},
+ }
+
+ kibble_yaml = get_kibble_yaml()
+ print(f"Writing Kibble config to {kibble_yaml}")
+ with open(kibble_yaml, "w") as f:
+ f.write(yaml.dump(config, default_flow_style=False))
+ f.close()
+
+
+@click.group()
+def cli():
+ """A simple command line tool for kibble"""
+
+
+@cli.command("version", short_help="displays the current kibble version")
+def version():
+ click.echo(kibble_version)
+
+
+@cli.command("setup", short_help="starts the setup process for kibble")
+def setup():
+ click.echo("Welcome to the Apache Kibble setup script!")
+ conf = KibbleConfigParser()
+ conf.read("kibble.ini")
+ conn_uri = conf.get("elasticsearch", "conn_uri")
+ click.echo(f"Elasticsearch: {conn_uri}")
+
+ # Create Kibble configuration file
+ save_config(
+ mlserver=conf.get("mail", "mailhost"),
+ conn_uri=conn_uri,
+ dbname=conf.get("elasticsearch", "dbname"),
+ )
Review comment:
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
##########
File path: kibble/__main__.py
##########
@@ -14,10 +14,104 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import os
+import yaml
+import click
+from kibble.version import kibble_version
+from kibble.configuration import KibbleConfigParser
+
+from kibble.settings import KIBBLE_YAML
+
+KIBBLE_VERSION = "0.1.0" # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = KIBBLE_YAML
+ if os.path.exists(kibble_yaml):
+ print(f"{kibble_yaml} already exists! Writing to {kibble_yaml}.tmp instead")
+ kibble_yaml = kibble_yaml + ".tmp"
+ return kibble_yaml
+
+
+def save_config(mlserver: str, conn_uri: str, dbname: str):
+ """Save kibble config to yaml file"""
+ if ":" in mlserver:
+ try:
+ mailhost, mailport = mlserver.split(":")
+ except ValueError:
+ raise ValueError(
+ "mailhost argument must be in form of `host:port` or `host`"
+ )
+ else:
+ mailhost = mlserver
+ mailport = 25
+
+ if ":" in conn_uri:
+ try:
+ hostname, port = conn_uri.split(":")
+ except ValueError:
+ raise ValueError(
+ "conn_uri argument must be in form of `host:port` or `host`"
+ )
+ else:
+ hostname = elasticsearch
+ port = 98200
+
+ config = {
+ "api": {"version": KIBBLE_VERSION, "database": KIBBLE_DB_VERSION},
+ "elasticsearch": {
+ "host": hostname,
+ "port": int(port),
+ "ssl": False,
+ "dbname": dbname,
+ },
+ "mail": {
+ "mailhost": mailhost,
+ "mailport": int(mailport),
+ "sender": "Kibble <no...@kibble.kibble>",
+ },
+ "accounts": {"allowSignup": True, "verify": True},
+ }
+
+ kibble_yaml = get_kibble_yaml()
+ print(f"Writing Kibble config to {kibble_yaml}")
+ with open(kibble_yaml, "w") as f:
+ f.write(yaml.dump(config, default_flow_style=False))
+ f.close()
+
+
+@click.group()
+def cli():
+ """A simple command line tool for kibble"""
+
+
+@cli.command("version", short_help="displays the current kibble version")
+def version():
+ click.echo(kibble_version)
+
+
+@cli.command("setup", short_help="starts the setup process for kibble")
+def setup():
+ click.echo("Welcome to the Apache Kibble setup script!")
+ conf = KibbleConfigParser()
+ conf.read("kibble.ini")
+ conn_uri = conf.get("elasticsearch", "conn_uri")
+ click.echo(f"Elasticsearch: {conn_uri}")
+
+ # Create Kibble configuration file
+ save_config(
+ mlserver=conf.get("mail", "mailhost"),
+ conn_uri=conn_uri,
+ dbname=conf.get("elasticsearch", "dbname"),
+ )
Review comment:
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org