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

Dockerized dev environment #71

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

Conversation

joserc87
Copy link

I was trying to get the project up and running in my machine, but there were quite a few things to do first, like:

  • installing postgres, as I didn't had it installed.
  • creating a user and a database
  • create a virtualenv with python 3.7 and install requirements (btw I first tried with python3.8 and the requirements failed to install).
  • etc

I thought it would be worth it to do it the "docker way", creating a docker-compose.yml and a bunch of default configuration files so that a new dev (that is me) would just need to do a docker-compose up to get the app running, without installing anything extra.

@joserc87
Copy link
Author

So far I have a postgresql + pgAdmin running on docker, with a user and a database that are automatically created. Next step is to try to create a Dockerfile to run the flask app from there, and then add it to the docker compose. It will take me many days to complete this, and it's probably overkill, but I will do it anyway. 🐳

@joserc87 joserc87 force-pushed the dockerized_dev_environment branch 2 times, most recently from 61b44d0 to 19ce0b4 Compare January 30, 2021 13:29
@joserc87 joserc87 changed the title WIP: Dockerized dev environment Dockerized dev environment Feb 17, 2021
@joserc87
Copy link
Author

I'm removing the WIP tag on this PR. It's by no means final, but it's working already (docker-compose up), and being it merged into master allows me to base some future PR on this work. If someone can verify at some point that the app works running it outside docker that would be great. Ping @Sketchy502.

Copy link
Collaborator

@Laukei Laukei left a comment

Choose a reason for hiding this comment

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

I've not had a chance to test this locally as I've not got a working local dev env at the minute and don't have the time to set one up, so this review alone shouldn't be considered an endorsement of the PR as a whole, but the changes look good and if they get a dockerized version of this up and running then that's an amazing step forward 👍

sdv/config.py.sample Outdated Show resolved Hide resolved
sdv/config.py.sample Outdated Show resolved Hide resolved
@@ -1,11 +1,32 @@
#!/usr/bin/env python

class DevelopmentConfig():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is DevelopmentConfig but I've left comments on it as though it were production as it's the only sample config we provide, so it's probably sensible to have some safeguards

sdv/config.py.sample Outdated Show resolved Hide resolved
DEBUG = True
ASSET_PATH = 'sdv/assets'
LANGUAGES = ['en']

Copy link
Collaborator

Choose a reason for hiding this comment

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

This possibly also needs

IMAGE_FOLDER = './static/images'
IMAGE_MAX_PER_FOLDER = 30000 # We use this as some filesystems have a limit on the number of subfolders per folder (I think ext2?)
RENDER_FOLDER = './static/renders'

MAIL_SERVER = None
MAIL_PORT = 465
MAIL_USE_SS = True
MAIL_DEFAULT_SENDER = ('sender name', '[email protected]')
MAIL_USERNAME = None
MAIL_PASSWORD

LANGUAGES = ['en', 'es', 'pt_BR', 'zh_Hans_CN']


API_V1_UPLOAD_ZIP_TIME_PER_USER = 3600
API_V1_UPLOAD_ZIP_LIMIT_PER_USER = 10

API_V1_PLAN_TIME = 60  # time interval (seconds)
API_V1_PLAN_LIMIT = 10  # number of successful submissions in time 
API_V1_PLAN_MAX_RENDERS = 3
API_V1_PLAN_APPROVED_SOURCES = ['stardew.info']

and may need:

BASE_DIR = os.path.dirname(os.path.realpath(__file__))

though this was used for fixing some relative directories I think that might not apply in dockerized dev

There's also a

LEGACY_ROOT_FOLDER = './sdv'

Which might not be required

@@ -1,10 +1,11 @@
import os

os.chdir(os.path.join(os.path.dirname(__file__), "sdv"))
# os.chdir(os.path.join(os.path.dirname(__file__), "sdv"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't got a local dev env set up so can't test this, but I feel like things won't work properly without this. It's possibly more of a patch for a legacy bug than needed for a new dev env though, so I'm not sure what the best approach is. Will need to test this.

Copy link
Author

Choose a reason for hiding this comment

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

I should have added a comment there. I don't remember the details but I couldn't make either the container or the test to work with this, I had to fix this by adding sdv to the PYTHONPATH variable (see docker-compose.yml), which IMHO it's a better approach. With export PYTHONPATH=.:sdv you should not have problems, but that's a preference and if we want to keep this line I'll make the PR to work around this.

@@ -52,7 +52,6 @@ six==1.11.0
speaklater==1.3
SQLAlchemy==1.2.14
termcolor==1.1.0
typing==3.6.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

What required typing? Why can we remove it? (open to believing we can, not sure why it was there in the first place though)

Copy link
Author

Choose a reason for hiding this comment

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

Again I don't remember all the details sadly, but I think it was because by enforcing the typing version black would refuse to install. And typing is a dependency of the other libraries anyway.


app = Flask(__name__)
config_name = os.environ.get("SDV_APP_SETTINGS", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly a bad idea from a security point of view? Defaulting to dev could introduce risk in a production env

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