Skip to content
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

Remove dirty hax, fix dependencies #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove dirty hax, fix dependencies #5

wants to merge 3 commits into from

Conversation

SuperPrower
Copy link

Hi. Several steps were preventing me from running a server. I attempted to fix them in this commit, including fixing missing and broken requirements and removing need for meddling with system environment.

@SuperPrower
Copy link
Author

Noticed I made a small mistake: folder name in run.sh script is not required. I'll fix this plus few more things.

@SuperPrower
Copy link
Author

Should be OK to go now. Works out of the box, without meddling with .bashrc and stuff.

@iharshulhan iharshulhan self-requested a review January 23, 2019 20:39
Copy link
Collaborator

@iharshulhan iharshulhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment variables are used to separate production environment and local for testing. It uses different servers and protocols. Removing them is not appropriate.

The issue with requirements.txt was correct 👍



if __name__ == "__main__":
app.run()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already achieved in app.py

#!/usr/bin/env bash

SCRIPT_PATH=`dirname "$0"`
export INNOMETRICS_BACKEND_PATH="${SCRIPT_PATH}/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script may be executed from other folder and environment variable will wrong in that case

@SuperPrower
Copy link
Author

SuperPrower commented Jan 24, 2019

  1. You don't have to set environmental variables in .bashrc for them to work. You can set them in flask.sh script. Separating development and deployment versions can and should be done using Flaks Configuration, and even then, you can use isolated variable in shell script as opposed to global one in .bashrc.
  2. The purpose of serv.py is to avoid adding your directories to global python PATH. With this, your application will see both api and db directories just like that, because they are in same directory as serv.py.
  3. dirname command solves the issue - it returns the location of the script, although relative. _BACKEND_PATH would be set appropriately using it.

@iharshulhan
Copy link
Collaborator

I agree that you don't have to add them to bashrc. The line is wrong, one just needs to set environment variables in a convenient way. Flask configuration won't do, as WGSI server with SSL encryption is used for production version, which requires different code execution controlled by environment variable. Anyway, there is already config.ini which stores necessary variables and config_production.ini on production server, but in order to share the same code both on local and production machines, environment variables were used to load a right version of configuration and start a different server. If you can add your flask.sh script to deployment script and change the message, I would gladly accept the pull request.

I see the point of serv.py, but to make it more generic, please, create a function in app.py responsible for starting the application with all appropriate logic. Then you can call that function in serv.py. I would still keep a line with PYTHONPATH, but in that case you can add a comment that it is optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants