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

Set default migrations dir in app config #12

Closed
wants to merge 3 commits into from
Closed

Set default migrations dir in app config #12

wants to merge 3 commits into from

Conversation

davidism
Copy link
Contributor

Currently, if I want a custom migrations directory, I must pass the -d option after every command. Let's take advantage of the Flask app and use it's config to set the directory.

If no directory is passed on the command line, look at a value in the config, which defaults to the old default, 'migrations'. An alternate directory can still be specified with -d.

@miguelgrinberg
Copy link
Owner

I'm not sure about this one. An application normally has more than one configuration, so you will need to set the same directory in all of them for things to work as expected. Would it work for you if I add a directory argument to the Migrate class instead? If you want to stick the directory name in the configuration you still can, but you can also hardcode the directory, which to me seems more robust.

@davidism
Copy link
Contributor Author

That makes more sense. My main use case for this is that I want to include the migrations in the package when I deploy to production so that I can run DB upgrades there.

@davidism
Copy link
Contributor Author

OK, here's an implementation where the directory is stored in the extension. I used Flask-SQLAlchemy as an example of how to store state for each init_app.

Now I set up the extension like this in __init__.py:

migrate = Migrate(directory=os.path.abspath(os.path.join(os.path.dirname(__file__), 'migrations')))
# ...
migrate.init_app(app, db)

@davidism
Copy link
Contributor Author

Note that this will require existing users to update their env.py file with the new configuration.

@miguelgrinberg
Copy link
Owner

Before I merge this change I need to think about not breaking existing users that have the old env.py file and upgrade to the latest release.

@davidism
Copy link
Contributor Author

@miguelgrinberg Any progress towards a decision on this? Is there anything else you'd like to see in order to merge?

@miguelgrinberg
Copy link
Owner

Note that your change did not merge, and also did not have doc updates and tests.

I took the idea from your solution and implemented, in a slightly simpler way. Give it a try and let me know how it works.

At some point I'll improve the new test so that it is not a dup of the original test, but since you seem to be in a rush I can live with a crappy (yet effective) test for a while.

@davidism
Copy link
Contributor Author

Thanks! Good to see a release on PyPI too.

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