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

Fix local embedded database issues #172

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

Conversation

EMachin3
Copy link

This pull request includes a few changes that I made while trying to get trackman to run on my local machine for testing purposes. After implementing these changes, in addition to adding a config.json file and a service_account file as well as creating an OAuth application within the WUVT domain, I was able to run a trackman instance on localhost.

Copy link
Member

@mutantmonkey mutantmonkey left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I think using the embedded DB by default in the docker-compose file is a good idea; I just want to make sure that the "sketchy" database initialization doesn't happen outside of dev environments.

@@ -172,6 +172,9 @@ def init_app():
'CACHE_TYPE': "RedisCache",
'CACHE_REDIS_URL': cache_redis_url,
})


#this command is sketchy, but it made running trackman locally work for me
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to have this triggered by an environment variable...perhaps DB_INIT_ON_BOOT or something like that.

Also, for PEP8 compliance, please add a space between # and the rest of the comment. It might also be good to add some newlines to clearly separate the "sketchy" bit out, but I don't know that that's strictly necessary.

@@ -7,7 +7,7 @@ services:
dockerfile: Dockerfile.dev
environment:
- APP_CONFIG_PATH=/data/config/config.json
- USE_EMBEDDED_DB
- USE_EMBEDDED_DB=1 #DON'T USE FOR PRODUCTION
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space between # and the rest of the comment to improve readability.

@@ -4,7 +4,7 @@
from . import app, db_utils, lib, pubsub, tasks


@app.cli.command()
@app.cli.command('init_embedded_db')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is necessary. It should use the function name by default, no?

Copy link
Author

Choose a reason for hiding this comment

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

It didn't on my machine for some reason.

@jbellerb
Copy link
Member

The more I think about this, the more I feel like the embedded database approach for development doesn't make much sense. If you're using it in the main container, it also needs to be enabled in the scheduler, but then you need to set up a shared volume between the containers, and at that point having the same Postgres database used in production running with docker-compose is easier so why even bother?

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.

3 participants