-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor setup.py #48
Conversation
from elasticsearch import VERSION as ES_VERSION | ||
ES_MAJOR = ES_VERSION[0] |
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.
This was never used
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) |
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.
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
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.
Telling users to utilize requirements.txt would probably be the best bet here. That way you can also use pipenv easily.
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.
Totally agree, I will remove it then - I left it just to preserve the logic
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 |
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.
Instead of this, we can use default values in argparser arguments
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.
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.
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.
Having it as CLI args would help with automated setups.
That was the main idea, see #50
|
fa341b3
to
d6c9410
Compare
@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. |
d6c9410
to
894939c
Compare
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, |
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 👍 |
We'd still need something to set up the indices, but that could be a non-interactive smaller script. |
894939c
to
3d42330
Compare
I think the indices are configured as expected.
Definitely agree 👍 |
I'll give it a final look-over later today and merge things then. |
This PR introduces some structure to setup.py by encapsulating some parts of the logic into functions and creating the main function.
3d42330
to
03d10a8
Compare
def get_user_input(msg: str, secure: bool = False): | ||
value = None | ||
while not value: | ||
value = getpass(msg) if secure else input(msg) |
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.
Nice.
@Humbedooh are we ok with moving on with this PR? |
This PR introduces some structure to setup.py by encapsulating some parts of the logic into functions and creating the main function.
This PR introduces some structure to setup.py by encapsulating some parts of the logic into functions and creating the main function.
This PR introduces some structure to setup.py by encapsulating
some parts of the logic into functions and creating the main
function.