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

Add missing migrations #3608

Closed
wants to merge 8 commits into from
Closed

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 14, 2018

There are some missing migrations on the project, this make a little difficult to isolate migrations for other recent changes.

@humitos
Copy link
Member

humitos commented Feb 14, 2018

Good! I noticed this some time ago and I wasn't sure why this was happening. Do you know why these migrations are needed?

I supposed that it could be some help_text that changed or choices so a new migration is needed but in the migration files it seems there are CreateModel calls. So, I'm confused. How the project could work without those models created? Aren't those models necessary for the project? Should we remove them?

@safwanrahman
Copy link
Member

So this models were added by #2764
@agjohnson Do you have any idea why the migration was not added? and how that is working now in production?

@stsewd
Copy link
Member Author

stsewd commented Feb 15, 2018

@humitos @safwanrahman maybe something to do with the python manage.py loaddata test_data step (just trying to guess here)?

And I'm not sure about the production db.

@humitos
Copy link
Member

humitos commented Feb 15, 2018

@stsewd the loaddata command doesn't affect the models, it just add data to the database without touching its structure. So, that shouldn't be the issue.

@humitos
Copy link
Member

humitos commented Mar 15, 2018

@ericholscher can you take a quick look at this PR? This is causing problems in other PRs to generate the proper migrations files.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I believe most of these are due to linting and other cleanup, but the integration proxy models might be either a missed migration or a difference in django versions or something? I believe we need those migrations either way.

I will block on the migration names. As I usually bring up in migration PRs, I prefer migrations to be named, not autonamed. If there was a way to auto enforce this, I'd be 👍 on that as well.

@stsewd stsewd force-pushed the missing-migrations branch from 0becf0f to befa4c8 Compare March 16, 2018 02:00
@stsewd
Copy link
Member Author

stsewd commented Mar 16, 2018

but the integration proxy models might be either a missed migration or a difference in django versions or something?

@agjohnson I did a grep and didn't find any previous migration about that models on the project :/

I updated the names, I'm not sure if I chosen the more descriptive name for some migrations, let me know if you want me to change some names.

@ericholscher
Copy link
Member

This looks good, and will remove the need to run migration warnings locally. Managing migration names is hard, but I'd say we merge it this week along w/ our other "cleanup" PR's, so we can test it locally and then deploy next week.

Will wait on @agjohnson's re-review to merge.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

but the integration proxy models might be either a missed migration or a difference in django versions or something? I believe we need those migrations either way.

I'd say that it's because the proxy = True. There is a better explanation here: https://stackoverflow.com/questions/37988914/why-does-django-create-migration-files-for-proxy-models

I just left some suggestions regarding the naming of the files.

@@ -0,0 +1,20 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Name suggestion: 0002_alter_choices_in_level_model.py

@@ -0,0 +1,56 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Name suggestion: 0003_create_proxy_webhook_models.py

@@ -0,0 +1,50 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Name suggestion: 0024_alter_and_project_models.py

Copy link
Member Author

Choose a reason for hiding this comment

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

...alter_and domain and project, right?

@stsewd stsewd force-pushed the missing-migrations branch from befa4c8 to c379bfb Compare April 4, 2018 17:24
@stsewd stsewd force-pushed the missing-migrations branch from c379bfb to 24cbc42 Compare April 30, 2018 22:47
@ericholscher
Copy link
Member

This is outdated. Closing this, we should just run this right before doing a deploy, since any PR will likely conflict or go stale.

@stsewd stsewd deleted the missing-migrations branch May 24, 2018 22:43
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.

6 participants