Update requirements and dependency badge

[itsjeyd]

cf. OC-2024

Notes

• I could not update PyYAML, selenium, and Werkzeug to the most recent versions available, since Jasmine currently still depends on slightly older versions.

• Do not ignore overridden View.get_view_name() in breadcrumbs encode/django-rest-framework#3273 changed the way breadcrumbs are generated for browsable API pages: Before, DRT would call a generic get_view_name function to obtain view names to use for individual breadcrumbs. That function only uses information that it can obtain from the view class, and does not depend on the action attribute being set on the view. Now, the code that generates breadcrumbs instantiates a view based on the view class and calls the (overridden) get_view_name function on it. This broke a couple of tests for the instance and app server APIs (OpenEdXInstanceAPITestCase.test_view_name and OpenEdXAppServerAPITestCase.test_view_name), because our custom implementations of get_view_name (InstanceViewSet.get_view_name and OpenEdXAppServerViewSet.get_view_name) depended on the action attribute being set. DRT will only set the .action attribute on a view in the context of a request, so when the breadcrumbs code called get_view_name on a newly instantiated InstanceViewSet or OpenEdXAppServerViewSet, that call would fail with AttributeError: 'ViewSet' object has no attribute 'action'..

  faa6214 fixes this issue by using getattr to retrieve the value of the action field, allowing the breadcrumbs code to finish successfully. However, a small problem remains: When generating breadcrumbs using the new method, DRT itself can not distinguish between list and detail views of an InstanceViewSet/OpenEdXAppServerViewSet (since suffix is None). This results in breadcrumbs being displayed as

  Api Root / Instance None / Instance None

  on the details page of an instance. On master (which uses an older version of DRT that precedes Do not ignore overridden View.get_view_name() in breadcrumbs encode/django-rest-framework#3273) we instead got:

  Api Root / Instance List / Instance Instance

  It's possible that I'm missing something, but given that we can only use the value of the action attribute to determine an appropriate view name in the context of a request, I'm not sure how we could go about determining an appropriate view name in the context of breadcrumbs generation.

  There's always the option of sticking with an older version of DRT, or using a custom fork that reverts the changes from Do not ignore overridden View.get_view_name() in breadcrumbs encode/django-rest-framework#3273; but at this point, that would mean using a significantly older version of DRT (3.3.3 vs. 3.5.3), or introducing additional overhead for maintaining a separate fork.

  Update: Custom get_view_name methods have been removed completely, cf. relevant discussion starting with Update requirements and dependency badge #153 (comment). Keeping this note for future reference.

Test instructions

• Observe green build on this PR.
• Run make shell locally to see that it starts up successfully.
• Run the development server. Observe that there are no deprecation warnings in the output. Check that you can access the instance manager in the browser (shouldn't raise a DisallowedHost exception). Note that if you run management commands with PYTHONWARNINGS=all, you'll still see some warnings, but they concern code installed into the venv; they are not triggered by instance manager code.

Reviewers

• @smarnach

[antoviaque]

If I understand correctly, this new line creates the DB if it doesn't exist, right? Why is this needed here?

[itsjeyd]

@antoviaque Yup, that's what it does. As for why it's necessary, cf. ac9be4f: After the updates, makemigrations fails with "OperationalError: database ... does not exist" if the target database does not exist (this is actually what I would expect). I first noticed this on CircleCI, and then also verified it locally, by changing DATABASE_URL in .env.test to point to a database that didn't exist on my Vagrant box and running make test_migrations_missing. Didn't get any errors with the old dependencies/master, and saw the error mentioned above after switching back to the new dependencies/this branch.

[smarnach]

@itsjeyd Any idea why this is happening now, and didn't happen in the past? It looks like this should always have been an issue. I can't figure out by what mechanism the DB was created on CircleCI before this PR.

In any case, the make target test_migrations_missing seems like a weird place for the implicit creation of the database. If this is indeed necessary, it can be simplified to

createdb --encoding utf-8 --template template0 opencraft 2> /dev/null || true

[itsjeyd]

@smarnach I don't know why the behavior changed, but I'm assuming it has something to do with the (Django) upgrade. As mentioned earlier, it's possible to reproduce the before/after behavior that CircleCI showed locally: In .env.test, set DATABASE_URL to point to a database that doesn't exist (e.g., DATABASE_URL='postgres://localhost/foo'). Then try running make test_migrations_missing

• on a Vagrant snapshot with dependencies from master installed
• on a Vagrant snapshot with dependencies from this branch (update-dependencies) installed

In the first instance, you won't get any errors. In the second instance, the command will error out with

django.db.utils.OperationalError: FATAL: database "foo" does not exist

This (combined with the fact that I also couldn't find any code that would create an "opencraft" database on CircleCI) makes me think that there never was an "opencraft" database on CircleCI, and that we didn't notice because make test_migrations_missing succeeded anyway.

I simplified the command according to your suggestions.

[antoviaque]

To understand this better, why do we change the view name depending on the action? Is there a broader fix/refactoring which would allow to prevent burying ourselves further here?

[itsjeyd]

@antoviaque We change the view name to get a better/more specific heading displayed in the browsable API. When overriding get_view_name, we get "Instance List" on /api/v1/instance/, and "Instance Details" on /api/v1/instance/{pk}/:

• 
• 

When not overriding get_view_name, we get "Instance List" on api/v1/instance/ (same as before), but on /api/v1/instance/{pk}/ we get "Instance Instance":

• 
• 

I'm not that familiar with DRT, so I'm not sure if there is a broader fix/refactoring that we could apply here (if there is, it would probably also take care of the breadcrumbs issue that I laid out in the PR description).

EDIT: Of course, assuming that we don't want to expose the browsable API to end users, we could also decide that we don't care about "Instance Details" vs. "Instance Instance", and just remove any custom get_view_name implementations.

[smarnach]

I'm in favour of removing the get_view_name() methods. They don't seem to be worth the trouble.

[itsjeyd]

@smarnach I agree -- removed those methods in a7a0712.

CC @antoviaque

[antoviaque]

+1 for the solution - thanks!

[antoviaque]

The duplication of the hacky fix & its explanation comment make me double down on my question in instance/api/instance.py

[antoviaque]

Isn't env.json() already supposed to deserialize the variable string?

[itsjeyd]

@antoviaque You'd think that it would, and the docs are terse; but I double-checked this in the shell and found that it does indeed return a string:

In [9]: import environ
In [11]: env = environ.Env()
In [12]: bar = env.json('BAR', default='[]')
In [13]: bar
Out[13]: '[]'
In [14]: type(bar)
Out[14]: str

[smarnach]

It returns a string because the default value is a string. Please remove the quotes from the default value, and remove the call to json.loads().

[itsjeyd]

Rebased from 85a9426.

[smarnach]

@itsjeyd I've done a first pass over this PR. I'll manually test once the comments are addressed.

[itsjeyd]

Thanks @smarnach! I'll address your comments later on today (have been working on fixing additional warnings that came up when running commands with PYTHONWARNINGS=all).

[itsjeyd]

@smarnach Thanks for the review! I addressed your comments, please have another look.

[antoviaque]

Can this be put in its own create_db rule, and the rule added as a dependency for test_migrations_missing? Or, better, rather than always get test_migrations_missing to execute it, add make create_db just for circleci in

opencraft/circle.yml

Line 15 in 8572619

pre:

and replace https://github.com/open-craft/opencraft/blob/master/bin/bootstrap#L52-L53 to factorize.

[antoviaque]

+1 for the solution - thanks!

[antoviaque]

Thanks for the changes! One last remark for the DB creation for CircleCI in the Makefile, but all good on my side once this is addressed.

[smarnach]

@itsjeyd @antoviaque I've looked into the database creation issue. CircleCI should figure out what database to create from the DATABASE_URL environment variable. This seems to be still working, since the unit tests, which also use the database, are passing fine before the test_migrations_missing target fails. I'm currently looking into alternative solutions for this problem, but will go for the change Xavier suggested if I can't come up with anything cleaner.

I also noticed that we still have a TODO in the Vagrantfile to use the official Ubuntu image. I think we should address this TODO as part of this PR, and I started working on it.

[antoviaque]

@smarnach As far as I can tell the official Ubuntu image is still broken - at least Ned was having issues with it last week (cf the ops channel). Imho this is outside the scope of this issue, I'd rather just make sure this doesn't spillover.

[smarnach]

@antoviaque OK, fine with me. I've already taken a quick look, and it doesn't seem to be straight-forward to switch to the official image, so I'll just leave it as it is.

[smarnach]

Actually, ./manage.py makemigrations has no reason to access the database at all. All it should do is compare the generated migrations with the models in the .py files, independent of what's in the DB. This might be a bug in Django 1.10. I haven't figured out a way around it, so I will just use the approach suggested by Tim and Xavier.

[smarnach]

I couldn't figure out why the database is working for the unit tests, but not working for testing for missing migrations. I tried several approaches, but the only thing that works is what Xavier suggested. The build is green now, and the only change I made was addressing Xavier's comment, so I'm merging this PR now.