Skip to content
This repository has been archived by the owner on Jan 21, 2021. It is now read-only.

upgrade gunicorn and django #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

upgrade gunicorn and django #40

wants to merge 2 commits into from

Conversation

Ali-D-Akbar
Copy link

@Ali-D-Akbar Ali-D-Akbar commented Jan 6, 2021

PROD-333
PROD-239

Upgrading gunicorn above 19.5.0 which resolved potential vulnerability in process_headers and also upgrading django above 1.11.19 which resolved uncontrolled memory consumption

requirements/base.txt Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
Django >= 1.4, < 1.5
Django

Choose a reason for hiding this comment

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

Do we need a constraint or max version specified to prevent us from installing a major version bump?

Copy link
Author

@Ali-D-Akbar Ali-D-Akbar Jan 11, 2021

Choose a reason for hiding this comment

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

I saw one PR that seems to be a safe upgrade for django to me.
#39

I'm still not 100% sure about this as I have no idea about this repository.

Copy link
Member

Choose a reason for hiding this comment

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

When removing the version constraint from base.in, you should instead pin the version in the constraints.txt file. This is the method that we use in most of the edX repos. Not adding the upper constraint on Django version will update it to the latest version whenever django-nose constraint is updated.

@Ali-D-Akbar Ali-D-Akbar requested a review from stvstnfrd January 12, 2021 12:35
@ormsbee
Copy link
Contributor

ormsbee commented Jan 15, 2021

I'll defer to @stvstnfrd, since I think he's more up to speed with recent xserver happenings (wasn't it slated to be on the chopping block soon?). That being said, how was this tested? Going from Django 1.4 -> 1.11 is a really big leap, and it's hard to believe that everything still works with just the requirement bump (even if it is a small repo).

@Ali-D-Akbar
Copy link
Author

I haven't tested this thoroughly nor do I have knowledge about the repo. All I knew was that, this repo is in Python 2 and it needs to be on the last supported django version. I had to go through Django release notes and 1.11.x series is the last version to support Python 2. If someone with a know-how of this repo can take a good look at the changes, that'd be great.

Copy link

@stvstnfrd stvstnfrd left a comment

Choose a reason for hiding this comment

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

Going from Django 1.4 -> 1.11 is a really big leap, and it's hard to believe that everything still works with just the requirement bump (even if it is a small repo).

@ormsbee Good flag. Interestingly enough, I think this should be fine...

unless this package's requirements are shared by the same venv used to process the grader (and thus having an implicit dependency in the grader code); but I have no idea how this code actually runs and/or is sandboxed...

TL;DR: I don't think Django is actually used, at least not directly...

A grep for django [1] only turns up

  • lines in various requirements files (both Django and Django-nose)
  • comments; comments which ironically say "NOT Django" :)
    • # Not django, but do something similar
    • # Not django (for now), but use the same settings format anyway

@cpennington Explicitly removed the Django dependency back in August 2012 😬 .
However, it was reintroduced in a monolithic/cribbing commit a few months later, perhaps naively?

  • 3c4d5f3
    • "Stolen from xqueue, should be a reasonable base."

So if Django is being used anywhere, it'd have to be via django-nose, right?
Buuut... It doesn't even look like the test suite is hooked up to run against the repo via CI.
And save a single import tweak 3+ years ago, this test suite hasn't been touched since Victor/2012.
Aaand looking at the tests, they function by actually making an HTTP request to a locally running server, so I don't think this is as easy as just saying, "let's run them on Github Actions"; they'd need refactoring.

I suspect we could remove the django and django-nose dependencies entirely, but I'm not sure how you'd test it, short of just trying it...

  • [1] grep -ri django ./

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants