Skip to content

Conversation

@Kelketek
Copy link
Contributor

@openedx-webhooks
Copy link

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=11295

Kelketek added a commit that referenced this pull request Jan 21, 2016
@Kelketek Kelketek merged commit 3cc11d1 into master Jan 21, 2016
@Kelketek Kelketek deleted the bump-problem-builder branch January 21, 2016 20:32
@feanil
Copy link
Contributor

feanil commented Jan 22, 2016

Hello, we're seeing issues in our CI environment as a result of this merging to master. Because the migrations were already run. This is also the case in our production environment so we need to figure out how to fix this.

@nedbat @doctoryes @edx/devops Is this because we didn't run a fake-initial for this model that was migrated?

@feanil
Copy link
Contributor

feanil commented Jan 22, 2016

If we don't have a resolution in the next 3 hours, I would like to revert this PR until we know what the operational procedure should be for this change.

@adampalay FYI, we should reach a resolution on this before you cut the next RC.

@doctoryes
Copy link
Contributor

Yes - since the table already exists, a --fake-initial migration would need to be run for this model to get all the migration accounting correct without actually trying to create the table. Can this be tried somewhere?

@feanil
Copy link
Contributor

feanil commented Jan 22, 2016

@doctoryes, it looks like that resolved the issue in the CI environment so we just need to do the same during our deploys. @maxrothman @jibsheet FYI, you'll need to run migrations manually and run with --fake-initial to deal with the migration of the problem_builder_answer table from south to django migrations.

@adampalay
Copy link
Contributor

Are we cool with this resolution? Will we run into problems down the line with future deploys? Is this something that can — or should — be fixed in code?

@nedbat
Copy link
Contributor

nedbat commented Jan 22, 2016

@Kelketek we definitely needed some devops thumbs on this change.

@adampalay
Copy link
Contributor

I put in a PR to revert this until we sort it out: https://github.com/edx/edx-platform/pull/11308

@jzoldak
Copy link
Contributor

jzoldak commented Jan 22, 2016

My vote is for revert while we sort out the breakage implications. I am concerned that this will cause problems when switching back and forth between branches on devstack and jenkins.

@Kelketek
Copy link
Contributor Author

@nedbat @adampalay @jzoldak My apologies. The normal handling of hash updates, as I understand it, allows for one to merge in once the subordinate PR is finished. As the test build appeared to pass, and I had given instructions in a devops ticket on how to migrate the repo at https://openedx.atlassian.net/browse/DEVOPS-3555 , and I thought that it would not be on the staging server until someone from devops knew about it (since it was acknowledged), I thought I was in the clear.

Apparently not. Next time I'll get an explicit +1. @antoviaque @bradenmacdonald heads up here.

@nedbat
Copy link
Contributor

nedbat commented Jan 22, 2016

@Kelketek i think the thing here that required extra caution was the migration aspect, and especially the 1.8 transition complications.

@nedbat
Copy link
Contributor

nedbat commented Jan 22, 2016

But thumbs-up on edx-platform is always required.

@Kelketek
Copy link
Contributor Author

@nedbat Yes, I see that now. I will do better in the future, and my apologies again. And good to know on the procedure front there. That was a point of confusion.

@jzoldak
Copy link
Contributor

jzoldak commented Jan 22, 2016

@Kelketek No problem. We've had a bunch of migration issues that we've needed to deal with after the django 1.8 upgrade, and are all building an understanding of all the flavors that they might come in.

We'll revert this PR, and then have you put in another one that contains an updated bok-choy cache with the initial migration applied.

@nedbat
Copy link
Contributor

nedbat commented Jul 6, 2016

@doctoryes @feanil The Eucalyptus notes say that this change might require a fake-initial: https://openedx.atlassian.net/wiki/display/COMM/Eucalyptus. Can you help me understand what that means?

@doctoryes
Copy link
Contributor

--fake-initial is used if a table has already been created but its initial migration (0001_initial) that creates the table has not yet been run:
https://docs.djangoproject.com/en/1.8/topics/migrations/#adding-migrations-to-apps
I suppose this situation can happen if you perform a syncdb or the like. I don't quite understand how the table got created in this case without running/applying the migrations. The question to answer:

  • Did Dogwood have an Answers table for this XBlock? Was it created somehow?

If so, then a --fake-initial is needed to get the migration accounting correct.

@bradenmacdonald
Copy link
Contributor

@doctoryes problem-builder used to have South migrations but converted to Django migrations. For older installations that had run the South migration, fake-initial is required, or else the new Django migration will try to re-create tables already created by the South migration. I thought this happened pre-Dogwood, but perhaps it happened after.

However, I don't think this will concern most Open edX users, since problem-builder is only in edx-private.txt and as such is not installed by default.

@nedbat
Copy link
Contributor

nedbat commented Jul 8, 2016

Ah, edx-private is a key factoid here! Thanks.

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.

9 participants