Added pacing field Course API #11985
Conversation
clintonb
force-pushed
the
clintonb/course-api-pacing
branch
from
c4b66da to
9cdac9a
Compare
|
@nasthagiri @peter-fogg please review |
clintonb
force-pushed
the
clintonb/course-api-pacing
branch
2 times, most recently
from
3e5f143 to
6fc384c
Compare
| migrations.RemoveField( | ||
| model_name='courseoverview', | ||
| name='facebook_url', | ||
| ), |
There was a problem hiding this comment.
I don't see the code change associated with this operation. Should it have been committed previously?
There was a problem hiding this comment.
It appears the followup work for #11673 was not completed. @nasthagiri is there a ticket for this?
There was a problem hiding this comment.
I'm a bit confused about this. The original PR did have migration code for it. However, since then, the devOps team made a change to revert it in order to support production deployment to multiple workers, concurrently running different versions of the code. @jibsheet At this point, can we go ahead and re-add the removal of this field in the migration file?
There was a problem hiding this comment.
Correct, the original PR had migration code in it to remove that field from the table without previously having removed it from the model, which would have broken production. That RemoveField calls was removed. You can look at 0008 and 0009 for details.
Now that we've removed the model field in a release, it should be safe to remove the column in another release. 👍 from me, provided we've done the usual work when migrating this table to deal with row versions.
|
👍 aside from the question about migrations. |
| name='self_paced', | ||
| field=models.BooleanField(default=False), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Why is this added to an existing migration file?
There was a problem hiding this comment.
This is a new migration file. See my comment about #11673.
ECOM-3994
clintonb
force-pushed
the
clintonb/course-api-pacing
branch
from
6fc384c to
1185fff
Compare
|
👍 Once you have confirmation from devOps on the migration change. |
|
Drat. This is probably because the migration was removed during a release. It might be easiest to reset migrations for the course_overviews app: |
|
Yeah, we had to do shenanigans because 0008 (to drop) existed for a while, but the point of my changes was to have 0009 detect and put back the field for weird folks in @sanfordstudent's state. What's the current migration plan for that table? You can feel free to bring over your devstack and show me if that's easier. |
|
Yep - Unfortunately, Sandy had already fixed his local state before I could examine it. If we see this again, I'd like to see the table states etc. I tested a few things against master just now and was unable to replicate. Will wait for further reports of failure before looking any further. |
ECOM-3994