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

Fixes #69 - Create migration scripts for DB [WIP] #130

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

Conversation

laghee
Copy link
Collaborator

@laghee laghee commented Nov 19, 2019

This works locally to generate scripts, but I need to change the "development" config before we merge and I run everything again locally and then on Heroku to add the migrations dir, so we either need this to:

  1. automatically detect the correct current config from FLASK_ENV (or whatever we want to call this)

or

  1. hardcode it as "production" config (as we did with the Scheduler scripts ... which in retrospect seems silly -- exporting a config variable there would have made the data loading I did at AllHands much easier 🤦‍♀ )

Which brings me to a question about the current config.py file and how it's being used, because it seems inconsistent to me. This could be me missing something, but it looks like:

we're using exporting FLASK_ENV for dev setup:

# export FLASK_ENV=development

hardcoding a production config for Heroku:

web: gunicorn "ochazuke:create_app('production')"

hardcoding a production config for the Scheduler scripts:

app = create_app('production')

setting a FLASK_CONFIG to testing on CircleCI:

FLASK_CONFIG: testing

Should we consider using FLASK_ENV or FLASK_CONFIG consistently across all these instances, @karlcow? Or is there a reason this is safer/better/necessary?

@laghee laghee added the do not merge PR in progress or dependent on other changes label Nov 19, 2019
@laghee laghee requested a review from karlcow November 19, 2019 11:31
@karlcow karlcow temporarily deployed to webcompat-metrics-pr-130 November 19, 2019 11:31 Inactive
@laghee
Copy link
Collaborator Author

laghee commented Nov 19, 2019

Ooops. Right, tests are failing because I didn't update requirements.txt...

@laghee laghee temporarily deployed to webcompat-metrics-pr-130 November 19, 2019 11:38 Inactive
@karlcow
Copy link
Member

karlcow commented Nov 20, 2019

Should we consider using FLASK_ENV or FLASK_CONFIG consistently across all these instances, @karlcow? Or is there a reason this is safer/better/necessary?

we should try to make things consistent indeed.
On webcompat.com, we switched to .flaskenv with python-dotenv to make it easier to configure locally.

Then we could block the export to heroku (so dev configs/env are only locals).

then on Heroku to add the migrations dir

for the best of my knowledge this is not possible. There is no filesystem on heroku. The migrations always happen locally on the person computer doing the DB update. Then the modifications are committed and deployed on heroku.

flask db init # locally
flask db migrate # locally
# Commit the results in the github repo 
heroku run flask db upgrade 

Which ever we do, part of this pull request should contain a thorough documentation of the step by step instructions to migrate the DB.

🙏 Thanks a lot for everything you do/have done to make this project a breeze.

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

So this is a simple comment, no merge :)

app.register_blueprint(web_blueprint)
# Views for API clients
from ochazuke.api import api_blueprint
app.register_blueprint(api_blueprint, url_prefix='/data')

app.register_blueprint(api_blueprint, url_prefix="/data")
Copy link
Member

Choose a reason for hiding this comment

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

The spaces on the 7 lines above seems to be not logical now.

What about?

   # Web views for humans
    from ochazuke.web import web_blueprint
    app.register_blueprint(web_blueprint)

    # Views for API clients
    from ochazuke.api import api_blueprint
    app.register_blueprint(api_blueprint, url_prefix='/data')

or was it the result of black?

then maybe.

    from ochazuke.web import web_blueprint
    from ochazuke.api import api_blueprint

   # Web views for humans
    app.register_blueprint(web_blueprint)

    # Views for API clients
    app.register_blueprint(api_blueprint, url_prefix='/data')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is weird. I hadn't noticed, but I think that is indeed some strange Black formatting, yes. I'll see what I can do to make it happy without the extra weirdness.

@laghee
Copy link
Collaborator Author

laghee commented Nov 20, 2019

for the best of my knowledge this is not possible. There is no filesystem on heroku. The migrations always happen locally on the person computer doing the DB update. Then the modifications are committed and deployed on heroku.

Yes, the modifications are done locally, but the files generated in the migrations directory have to be added to the repo and committed to Heroku in order to successfully run heroku run flask db upgrade .

@laghee
Copy link
Collaborator Author

laghee commented Nov 20, 2019

I will look into .flaskenv and python-dotenv. I forgot to link to the dashboard section on Heroku with the config vars, which is here. There's no file system, but we could easily set an environment variable for the configuration that way as well (which would also allow for production situations). This is currently how the dynamic database connection is made in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PR in progress or dependent on other changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants