Skip to content

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Jun 6, 2013

r?

@rlr
Copy link
Contributor Author

rlr commented Jun 6, 2013

This also includes a commit for moving lib/ into kitsune module.

@rlr
Copy link
Contributor Author

rlr commented Jun 6, 2013

The code changes are pretty much just changes in the import path.

@willkg
Copy link
Member

willkg commented Jun 6, 2013

The tests pass for me. Jenkins kicks up this interesting message for all three failing tests:

Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave.

I have no clue what that means. It might as well say:

Unsafe statement written to the binary log, then taken out, then put in, and shaken all about. You do the HOKEY_POKEY and turn yourself about. That's what it's all about.

Anyhow, tests pass for me. I spent some time tooling around on my local instance taking care to have deleted the apps/ and lib/ directories before doing so. Everything looks ok to me as far as I can tell.

I have some thoughts:

  1. If anything does serialization of objects, I'm pretty sure these code changes will hose those. That might include celery tasks that haven't run and items in cache. After pushing this, we should probably expect to see a bunch of failures related to that. If there's a way to automatically flush the cache or invalidate it, then we should do that.
  2. Hrm... I had a second thought, but I forgot it while writing the first one. For now, this list item is a placeholder.

r+

@willkg
Copy link
Member

willkg commented Jun 6, 2013

Oh! I remember. This has a somewhat sad effect of making it slightly harder to run tests for a specific app.

Before we could do:

./manage.py test search

That doesn't work anymore. I'm not sure offhand how to do it now. It should take a Django app name in which case, this should work:

./manage.py test kitsune.search

But it doesn't run any tests.

@willkg
Copy link
Member

willkg commented Jun 6, 2013

@rlr pointed out that doing:

./manage.py test kitsune/search

works super. That's good enough for me.

@rlr rlr merged commit c2348e1 into mozilla:master Jun 6, 2013
@rlr
Copy link
Contributor Author

rlr commented Jun 6, 2013

260a570...c2348e1

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