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/10/10 18:06:13 UTC
[GitHub] [kibble] turbaszek opened a new pull request #48: Refactor setup.py
turbaszek opened a new pull request #48:
URL: https://github.com/apache/kibble/pull/48
This PR introduces some structure to setup.py by encapsulating
some parts of the logic into functions and creating the main
function.
----------------------------------------------------------------
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
[GitHub] [kibble] michalslowikowski00 commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
michalslowikowski00 commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502935736
##########
File path: setup/setup.py
##########
@@ -259,60 +206,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str):
+ value = None
+ while not value:
+ value = input(msg)
+ return value
+
+
+def print_configuration(args):
+ print("Configuring Apache Kibble elasticsearch instance with the following arguments:")
+ print(f"- hostname: {args.hostname}")
+ print(f"- port: {int(args.port)}")
+ print(f"- dbname: {args.dbname}")
+ print(f"- shards: {int(args.shards)}")
+ print(f"- replicas: {int(args.replicas)}")
+ print()
+
+
+def main():
+ """
+ The main Kibble setup logic. Using users input we create:
+ - Elasticsearch indexes used by Apache Kibble app
+ - Configuration yaml file
+ """
+ parser = get_parser()
+ args = parser.parse_args()
+
+ print("Welcome to the Apache Kibble setup script!")
+ print_configuration(args)
+
+ admin_name = "admin@kibble"
+ admin_pass = "kibbleAdmin"
+ if not args.autoadmin:
+ admin_name = get_user_input("Enter an email address for the adminstrator account:")
Review comment:
```suggestion
admin_name = get_user_input("Enter an email address for the administrator account:")
```
----------------------------------------------------------------
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
[GitHub] [kibble] michalslowikowski00 commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
michalslowikowski00 commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502936254
##########
File path: setup/setup.py
##########
@@ -259,60 +206,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str):
+ value = None
+ while not value:
+ value = input(msg)
Review comment:
Here the password will be echoed during typing.
What do think about `getpass()`?
https://docs.python.org/3.5/library/getpass.html
But also email address will be hidden with `gepass()`.
```suggestion
value = getpass(msg)
```
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502895377
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
+# Arguments for non-interactive setups like docker
+def get_parser():
+ arg_parser = argparse.ArgumentParser()
+ arg_parser.add_argument(
+ "-e", "--hostname",
+ help="Pre-defined hostname for ElasticSearch (docker setups). Default: localhost",
+ default="localhost"
+ )
+ arg_parser.add_argument(
+ "-p", "--port",
+ help="Pre-defined port for ES (docker setups). Default: 9200", default=9200
+ )
+ arg_parser.add_argument(
+ "-d", "--dbname", help="Pre-defined Database prefix (docker setups). Default: kibble", default="kibble"
+ )
+ arg_parser.add_argument(
+ "-s", "--shards", help="Predefined number of ES shards (docker setups), Default: 5", default=5
+ )
+ arg_parser.add_argument(
+ "-r", "--replicas", help="Predefined number of replicas for ES (docker setups). Default: 1", default=1
+ )
+ arg_parser.add_argument(
+ "-m", "--mailhost",
+ help="Pre-defined mail server host (docker setups). Default: localhost:25",
+ default="localhost:25"
+ )
+ arg_parser.add_argument(
+ "-a", "--autoadmin",
+ action='store_true',
+ help="Generate generic admin account (docker setups). Default: False",
+ default=False
+ )
+ arg_parser.add_argument(
+ "-k", "--skiponexist",
+ action='store_true',
+ help="Skip DB creation if DBs exist (docker setups). Defaul: True", default=True
+ )
+ return arg_parser
-# Arguments for non-interactive setups like docker
-arg_parser = argparse.ArgumentParser()
-arg_parser.add_argument("-e", "--hostname", help="Pre-defined hostname for ElasticSearch (docker setups)")
-arg_parser.add_argument("-p", "--port", help="Pre-defined port for ES (docker setups)")
-arg_parser.add_argument("-d", "--dbname", help="Pre-defined Database prefix (docker setups)")
-arg_parser.add_argument("-s", "--shards", help="Predefined number of ES shards (docker setups)")
-arg_parser.add_argument("-r", "--replicas", help="Predefined number of replicas for ES (docker setups)")
-arg_parser.add_argument("-m", "--mailhost", help="Pre-defined mail server host (docker setups)")
-arg_parser.add_argument("-a", "--autoadmin", action='store_true', help="Generate generic admin account (docker setups)")
-arg_parser.add_argument("-k", "--skiponexist", action='store_true', help="Skip DB creation if DBs exist (docker setups)")
-args = arg_parser.parse_args()
-
-print("Welcome to the Apache Kibble setup script!")
-print("Let's start by determining some settings...")
-print("")
-
-
-hostname = args.hostname or ""
-port = int(args.port) if args.port else 0
-dbname = args.dbname or ""
-mlserver = args.mailhost or ""
-mldom = ""
-wc = ""
-genname = ""
-wce = False
-shards = int(args.shards) if args.shards else 0
-replicas = int(args.replicas) if args.replicas else -1
-
-while hostname == "":
- hostname = input("What is the hostname of the ElasticSearch server? [localhost]: ")
- if hostname == "":
- print("Using default; localhost")
- hostname = "localhost"
-while port < 1:
- try:
- port = input("What port is ElasticSearch listening on? [9200]: ")
- if port == "":
- print("Using default; 9200")
- port = 9200
- port = int(port)
- except ValueError:
- pass
-
-while dbname == "":
- dbname = input("What would you like to call the DB index [kibble]: ")
- if dbname == "":
- print("Using default; kibble")
- dbname = "kibble"
-
-while mlserver == "":
- mlserver = input("What is the hostname of the outgoing mailserver? [localhost:25]: ")
- if mlserver == "":
- print("Using default; localhost:25")
- mlserver = "localhost:25"
-
-while shards < 1:
- try:
- shards = input("How many shards for the ElasticSearch index? [5]:")
- if shards == "":
- print("Using default; 5")
- shards = 5
- shards = int(shards)
- except ValueError:
- pass
-
-while replicas < 0:
- try:
- replicas = input("How many replicas for each shard? [1]: ")
- if replicas == "":
- print("Using default; 1")
- replicas = 1
- replicas = int(replicas)
- except ValueError:
- pass
Review comment:
Not opposed to this :) In the longer run, people can always edit the .yaml file later on if they need to make changes. Having it as CLI args would help with automated setups.
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706681562
Generally for this change. I think we should perhaps also - and this will break the api/ dir a moment, move away from separating host, port, uriprefix and ssl parameters and just accept the standard modern URL parameter instead. That is, `http://localhost:9200/` or `https://mydb.example.com/ES/` instead of having three or four different variables that pieces it together.
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-707079717
@Humbedooh are we ok with moving on with this PR?
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706601425
```
➜ python setup/setup.py -e 0.0.0.0 -a
Welcome to the Apache Kibble setup script!
Configuring Apache Kibble server with the following arguments:
- hostname: 0.0.0.0
- port: 9200
- dbname: kibble
- shards: 5
- replicas: 1
Creating index kibble_api
Creating index kibble_ci_build
Creating index kibble_ci_queue
Creating index kibble_code_commit
Creating index kibble_code_commit_unique
Creating index kibble_code_modification
Creating index kibble_evolution
Creating index kibble_file_history
Creating index kibble_forum_post
Creating index kibble_forum_topic
Creating index kibble_ghstats
Creating index kibble_im_stats
Creating index kibble_im_ops
Creating index kibble_im_msg
Creating index kibble_issue
Creating index kibble_logstats
Creating index kibble_email
Creating index kibble_mailstats
Creating index kibble_mailtop
Creating index kibble_organisation
Creating index kibble_view
Creating index kibble_publish
Creating index kibble_source
Creating index kibble_stats
Creating index kibble_social_follow
Creating index kibble_social_followers
Creating index kibble_social_follower
Creating index kibble_social_person
Creating index kibble_uisession
Creating index kibble_useraccount
Creating index kibble_message
Creating index kibble_person
Indices created!
Creating administrator account
Account created!
/Users/tomaszurbaszek/kibble/setup/../api/yaml/kibble.yaml already exists! Writing to /Users/tomaszurbaszek/kibble/setup/../api/yaml/kibble.yaml.tmp instead
Writing Kibble config to /Users/tomaszurbaszek/kibble/setup/../api/yaml/kibble.yaml.tmp
All done, Kibble should...work now :)
```
----------------------------------------------------------------
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
[GitHub] [kibble] michalslowikowski00 commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
michalslowikowski00 commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502936254
##########
File path: setup/setup.py
##########
@@ -259,60 +206,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str):
+ value = None
+ while not value:
+ value = input(msg)
Review comment:
Here the password will be echoed during typing.
What do think about `getpass()`?
https://docs.python.org/3.5/library/getpass.html
But also email address will be hidden with `getpass()`.
```suggestion
value = getpass(msg)
```
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706682086
>That is, `http://localhost:9200/` or `https://mydb.example.com/ES/` instead of having three or four different variables that pieces it together.
I agree and as a followup I wanted to figure out something around configuration. The rough idea was to abandon the setup script in favor of providing default yaml config and documentation pointing users to change it to suits their needs.
This PR was mostly about improving code and getting familiar with the codebase 👍
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502816360
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
Review comment:
Installing dependencies for a users is a rather bad pattern in my opinion. Users should fix it by running the command that was provided:
```
pip3 install elasticsearch certifi bcrypt
```
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
Review comment:
Installing dependencies for users is a rather bad pattern in my opinion. Users should fix it by running the command that was provided:
```
pip3 install elasticsearch certifi bcrypt
```
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706683010
> We'd still need something to set up the indices, but that _could_ be a non-interactive smaller script.
I think the indices are configured as expected.
> How about we merge this, and figure out the bigger picture from there on?
Definitely agree 👍
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502895579
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
+# Arguments for non-interactive setups like docker
+def get_parser():
+ arg_parser = argparse.ArgumentParser()
+ arg_parser.add_argument(
+ "-e", "--hostname",
+ help="Pre-defined hostname for ElasticSearch (docker setups). Default: localhost",
+ default="localhost"
+ )
+ arg_parser.add_argument(
+ "-p", "--port",
+ help="Pre-defined port for ES (docker setups). Default: 9200", default=9200
+ )
+ arg_parser.add_argument(
+ "-d", "--dbname", help="Pre-defined Database prefix (docker setups). Default: kibble", default="kibble"
+ )
+ arg_parser.add_argument(
+ "-s", "--shards", help="Predefined number of ES shards (docker setups), Default: 5", default=5
+ )
+ arg_parser.add_argument(
+ "-r", "--replicas", help="Predefined number of replicas for ES (docker setups). Default: 1", default=1
+ )
+ arg_parser.add_argument(
+ "-m", "--mailhost",
+ help="Pre-defined mail server host (docker setups). Default: localhost:25",
+ default="localhost:25"
+ )
+ arg_parser.add_argument(
+ "-a", "--autoadmin",
+ action='store_true',
+ help="Generate generic admin account (docker setups). Default: False",
+ default=False
+ )
+ arg_parser.add_argument(
+ "-k", "--skiponexist",
+ action='store_true',
+ help="Skip DB creation if DBs exist (docker setups). Defaul: True", default=True
+ )
+ return arg_parser
-# Arguments for non-interactive setups like docker
-arg_parser = argparse.ArgumentParser()
-arg_parser.add_argument("-e", "--hostname", help="Pre-defined hostname for ElasticSearch (docker setups)")
-arg_parser.add_argument("-p", "--port", help="Pre-defined port for ES (docker setups)")
-arg_parser.add_argument("-d", "--dbname", help="Pre-defined Database prefix (docker setups)")
-arg_parser.add_argument("-s", "--shards", help="Predefined number of ES shards (docker setups)")
-arg_parser.add_argument("-r", "--replicas", help="Predefined number of replicas for ES (docker setups)")
-arg_parser.add_argument("-m", "--mailhost", help="Pre-defined mail server host (docker setups)")
-arg_parser.add_argument("-a", "--autoadmin", action='store_true', help="Generate generic admin account (docker setups)")
-arg_parser.add_argument("-k", "--skiponexist", action='store_true', help="Skip DB creation if DBs exist (docker setups)")
-args = arg_parser.parse_args()
-
-print("Welcome to the Apache Kibble setup script!")
-print("Let's start by determining some settings...")
-print("")
-
-
-hostname = args.hostname or ""
-port = int(args.port) if args.port else 0
-dbname = args.dbname or ""
-mlserver = args.mailhost or ""
-mldom = ""
-wc = ""
-genname = ""
-wce = False
-shards = int(args.shards) if args.shards else 0
-replicas = int(args.replicas) if args.replicas else -1
-
-while hostname == "":
- hostname = input("What is the hostname of the ElasticSearch server? [localhost]: ")
- if hostname == "":
- print("Using default; localhost")
- hostname = "localhost"
-while port < 1:
- try:
- port = input("What port is ElasticSearch listening on? [9200]: ")
- if port == "":
- print("Using default; 9200")
- port = 9200
- port = int(port)
- except ValueError:
- pass
-
-while dbname == "":
- dbname = input("What would you like to call the DB index [kibble]: ")
- if dbname == "":
- print("Using default; kibble")
- dbname = "kibble"
-
-while mlserver == "":
- mlserver = input("What is the hostname of the outgoing mailserver? [localhost:25]: ")
- if mlserver == "":
- print("Using default; localhost:25")
- mlserver = "localhost:25"
-
-while shards < 1:
- try:
- shards = input("How many shards for the ElasticSearch index? [5]:")
- if shards == "":
- print("Using default; 5")
- shards = 5
- shards = int(shards)
- except ValueError:
- pass
-
-while replicas < 0:
- try:
- replicas = input("How many replicas for each shard? [1]: ")
- if replicas == "":
- print("Using default; 1")
- replicas = 1
- replicas = int(replicas)
- except ValueError:
- pass
Review comment:
> Having it as CLI args would help with automated setups.
That was the main idea, see #50
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502816219
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
Review comment:
This was never used
----------------------------------------------------------------
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
[GitHub] [kibble] michalslowikowski00 commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
michalslowikowski00 commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502936254
##########
File path: setup/setup.py
##########
@@ -259,60 +206,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str):
+ value = None
+ while not value:
+ value = input(msg)
Review comment:
Here the password will be echoed during typing.
What do think about `getpass()`?
https://docs.python.org/3.5/library/getpass.html
But also email address will be hidden with `gepass()`.
```suggestion
value = input(msg)
```
----------------------------------------------------------------
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
[GitHub] [kibble] michalslowikowski00 commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
michalslowikowski00 commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r503063940
##########
File path: setup/setup.py
##########
@@ -259,60 +208,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str, secure: bool = False):
+ value = None
+ while not value:
+ value = getpass(msg) if secure else input(msg)
Review comment:
Nice.
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706682776
We'd still need something to set up the indices, but that _could_ be a non-interactive smaller script.
How about we merge this, and figure out the bigger picture from there on? No harm done in merging and then forgetting this file later on in favor of something new :)
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706612542
@sharanf @Humbedooh @michalslowikowski00 would love to get a review on this one. Once we are ok with the proposed changes I will adjust the documentation.
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502895222
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
Review comment:
Telling users to utilize requirements.txt would probably be the best bet here. That way you can also use pipenv easily.
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502816528
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
+# Arguments for non-interactive setups like docker
+def get_parser():
+ arg_parser = argparse.ArgumentParser()
+ arg_parser.add_argument(
+ "-e", "--hostname",
+ help="Pre-defined hostname for ElasticSearch (docker setups). Default: localhost",
+ default="localhost"
+ )
+ arg_parser.add_argument(
+ "-p", "--port",
+ help="Pre-defined port for ES (docker setups). Default: 9200", default=9200
+ )
+ arg_parser.add_argument(
+ "-d", "--dbname", help="Pre-defined Database prefix (docker setups). Default: kibble", default="kibble"
+ )
+ arg_parser.add_argument(
+ "-s", "--shards", help="Predefined number of ES shards (docker setups), Default: 5", default=5
+ )
+ arg_parser.add_argument(
+ "-r", "--replicas", help="Predefined number of replicas for ES (docker setups). Default: 1", default=1
+ )
+ arg_parser.add_argument(
+ "-m", "--mailhost",
+ help="Pre-defined mail server host (docker setups). Default: localhost:25",
+ default="localhost:25"
+ )
+ arg_parser.add_argument(
+ "-a", "--autoadmin",
+ action='store_true',
+ help="Generate generic admin account (docker setups). Default: False",
+ default=False
+ )
+ arg_parser.add_argument(
+ "-k", "--skiponexist",
+ action='store_true',
+ help="Skip DB creation if DBs exist (docker setups). Defaul: True", default=True
+ )
+ return arg_parser
-# Arguments for non-interactive setups like docker
-arg_parser = argparse.ArgumentParser()
-arg_parser.add_argument("-e", "--hostname", help="Pre-defined hostname for ElasticSearch (docker setups)")
-arg_parser.add_argument("-p", "--port", help="Pre-defined port for ES (docker setups)")
-arg_parser.add_argument("-d", "--dbname", help="Pre-defined Database prefix (docker setups)")
-arg_parser.add_argument("-s", "--shards", help="Predefined number of ES shards (docker setups)")
-arg_parser.add_argument("-r", "--replicas", help="Predefined number of replicas for ES (docker setups)")
-arg_parser.add_argument("-m", "--mailhost", help="Pre-defined mail server host (docker setups)")
-arg_parser.add_argument("-a", "--autoadmin", action='store_true', help="Generate generic admin account (docker setups)")
-arg_parser.add_argument("-k", "--skiponexist", action='store_true', help="Skip DB creation if DBs exist (docker setups)")
-args = arg_parser.parse_args()
-
-print("Welcome to the Apache Kibble setup script!")
-print("Let's start by determining some settings...")
-print("")
-
-
-hostname = args.hostname or ""
-port = int(args.port) if args.port else 0
-dbname = args.dbname or ""
-mlserver = args.mailhost or ""
-mldom = ""
-wc = ""
-genname = ""
-wce = False
-shards = int(args.shards) if args.shards else 0
-replicas = int(args.replicas) if args.replicas else -1
-
-while hostname == "":
- hostname = input("What is the hostname of the ElasticSearch server? [localhost]: ")
- if hostname == "":
- print("Using default; localhost")
- hostname = "localhost"
-while port < 1:
- try:
- port = input("What port is ElasticSearch listening on? [9200]: ")
- if port == "":
- print("Using default; 9200")
- port = 9200
- port = int(port)
- except ValueError:
- pass
-
-while dbname == "":
- dbname = input("What would you like to call the DB index [kibble]: ")
- if dbname == "":
- print("Using default; kibble")
- dbname = "kibble"
-
-while mlserver == "":
- mlserver = input("What is the hostname of the outgoing mailserver? [localhost:25]: ")
- if mlserver == "":
- print("Using default; localhost:25")
- mlserver = "localhost:25"
-
-while shards < 1:
- try:
- shards = input("How many shards for the ElasticSearch index? [5]:")
- if shards == "":
- print("Using default; 5")
- shards = 5
- shards = int(shards)
- except ValueError:
- pass
-
-while replicas < 0:
- try:
- replicas = input("How many replicas for each shard? [1]: ")
- if replicas == "":
- print("Using default; 1")
- replicas = 1
- replicas = int(replicas)
- except ValueError:
- pass
Review comment:
Instead of this, we can use default values in argparser arguments
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh merged pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh merged pull request #48:
URL: https://github.com/apache/kibble/pull/48
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502936895
##########
File path: setup/setup.py
##########
@@ -259,60 +206,133 @@ def createIndex():
'apiversion': KIBBLE_VERSION, # Log current API version
'dbversion': KIBBLE_DB_VERSION # Log the database revision we accept (might change!)
}
- es.index(index=dbname+'_useraccount', doc_type = '_doc', id = adminName, body = doc)
- es.index(index=dbname+'_api', doc_type = '_doc', id = 'current', body = dbdoc)
+ es.index(index=dbname+'_useraccount', doc_type='_doc', id=admin_name, body=doc)
+ es.index(index=dbname+'_api', doc_type='_doc', id='current', body=dbdoc)
print("Account created!")
-try:
- import logging
- # elasticsearch logs lots of warnings on retries/connection failure
- logging.getLogger("elasticsearch").setLevel(logging.ERROR)
- createIndex()
-
-
-except Exception as e:
- print("Index creation failed: %s" % e)
- sys.exit(1)
-kibble_yaml = '../api/yaml/kibble.yaml'
+def get_kibble_yaml() -> str:
+ """Resolve path to kibble config yaml"""
+ kibble_yaml = os.path.join(
+ os.path.dirname(os.path.realpath(__file__)),
+ os.pardir,
+ "api",
+ "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
-if os.path.exists(kibble_yaml):
- print("%s already exists! Writing to %s.tmp instead" % (kibble_yaml, kibble_yaml))
- kibble_yaml = kibble_yaml + ".tmp"
-
-print("Writing Kibble config (%s)" % kibble_yaml)
+def save_config(
+ mlserver: str,
+ hostname: str,
+ port: int,
+ 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
-m = mlserver.split(':')
-if len(m) == 1:
- m.append(25)
-
-myconfig = {
- 'api': {
- 'version': KIBBLE_VERSION,
- 'database': KIBBLE_DB_VERSION
- },
- 'elasticsearch': {
- 'host': hostname,
- 'port': port,
- 'ssl': False,
- 'dbname': dbname
- },
- 'mail': {
- 'mailhost': m[0],
- 'mailport': m[1],
- 'sender': 'Kibble <no...@kibble.kibble>'
- },
- 'accounts': {
- 'allowSignup': True,
- 'verify': True
+ config = {
+ 'api': {
+ 'version': KIBBLE_VERSION,
+ 'database': KIBBLE_DB_VERSION
+ },
+ 'elasticsearch': {
+ 'host': hostname,
+ 'port': port,
+ 'ssl': False,
+ 'dbname': dbname
+ },
+ 'mail': {
+ 'mailhost': mailhost,
+ 'mailport': int(mailport),
+ 'sender': 'Kibble <no...@kibble.kibble>'
+ },
+ 'accounts': {
+ 'allowSignup': True,
+ 'verify': True
+ }
}
-}
-with open(kibble_yaml, "w") as f:
- f.write(yaml.dump(myconfig, default_flow_style = False))
- f.close()
+ 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()
+
+
+def get_user_input(msg: str):
+ value = None
+ while not value:
+ value = input(msg)
Review comment:
Good one, applied
----------------------------------------------------------------
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
[GitHub] [kibble] turbaszek commented on a change in pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #48:
URL: https://github.com/apache/kibble/pull/48#discussion_r502895511
##########
File path: setup/setup.py
##########
@@ -14,143 +14,93 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
-KIBBLE_DB_VERSION = 2 # Second database revision
-
import sys
-
-if sys.version_info <= (3, 3):
- print("This script requires Python 3.4 or higher")
- sys.exit(-1)
-
import os
-import getpass
-import subprocess
import argparse
-import shutil
+import logging
import yaml
import bcrypt
import json
-mappings = json.load(open("mappings.json"))
-myyaml = yaml.load(open("kibble.yaml.sample"))
+KIBBLE_VERSION = '0.1.0' # ABI/API compat demarcation.
+KIBBLE_DB_VERSION = 2 # Second database revision
+
+if sys.version_info <= (3, 3):
+ print("This script requires Python 3.4 or higher")
+ sys.exit(-1)
-dopip = False
+# Check if Elasticsearch is installed, if not
+# prompt user with command to run
try:
from elasticsearch import Elasticsearch
- from elasticsearch import VERSION as ES_VERSION
- ES_MAJOR = ES_VERSION[0]
-except:
- dopip = True
-
-if dopip and (getpass.getuser() != "root"):
- print("It looks like you need to install some python modules first")
- print("Either run this as root to do so, or run: ")
- print("pip3 install elasticsearch certifi bcrypt")
+except ImportError:
+ print(
+ "It looks like you need to install some python modules first"
+ "Either run this as root to do so, or run: \n"
+ "pip3 install elasticsearch certifi bcrypt"
+ )
sys.exit(-1)
+
-elif dopip:
- print("Before we get started, we need to install some modules")
- print("Hang on!")
- try:
- subprocess.check_call(('pip3','install','elasticsearch', 'certifi', 'bcrypt'))
- from elasticsearch import Elasticsearch
- except:
- print("Oh dear, looks like this failed :(")
- print("Please install elasticsearch and certifi before you try again:")
- print("pip install elasticsearch certifi")
- sys.exit(-1)
Review comment:
Totally agree, I will remove it then - I left it just to preserve the logic
----------------------------------------------------------------
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
[GitHub] [kibble] Humbedooh commented on pull request #48: Refactor setup.py
Posted by GitBox <gi...@apache.org>.
Humbedooh commented on pull request #48:
URL: https://github.com/apache/kibble/pull/48#issuecomment-706683268
I'll give it a final look-over later today and merge things then.
----------------------------------------------------------------
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