-
Notifications
You must be signed in to change notification settings - Fork 918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Django 2.2 #7196
Django 2.2 #7196
Conversation
@SmileyChris based on my reading of https://circleci.com/gh/mozilla/bedrock/18651 we're getting a |
0a2211d fixed #7196 (comment) but now there's a new error in the same command. Once CirlcleCI is passing, I'll push this to the run-integration-tests branch to trigger that pipeline on our infra again. |
Now that 917efbd is passing all the CircleCI checks, I've pushed it to the |
Got the following error from the CI job above:
|
I've deployed 917efbd to https://bedrock-demo-pr7196.oregon-b.moz.works for additional testing. |
It looks like the |
@SmileyChris as I mentioned in our meeting earlier, please feel free to ping me on slack once you've got |
Status update
Chrome tests
headless tests
|
Sentry traceback from https://bedrock-integration-tests.oregon-b.moz.works/en-US/about/forums/
|
The second headless test failure is due to a redirect loop:
|
Looking at the video for the selenium failures in the chrome tests for above, the |
@jgmize the two obvious fixes are done, try again and see if selenium is still a problem. I'm out until mid next week, so if it's an issue and you want to poke the selenium bear yourself then feel free 🐻 You can push to the LL branch to keep it as part of this PR. |
Unfortunately we are still getting some selenium errors:
|
I rebased this branch on current master, resolved the merge conflicts, and force pushed to lincolnloop:django-2.2, triggering circleci #19657, and to mozilla:run-integration-tests, triggering jenkins build #329. |
After iterating over a bunch of rebase errors, replacing the broken usage of pytest.mark.viewport('mobile') in the selenium fixture with a separate selenium_mobile fixture, and some other miscellaneous cleanup, all of the CircleCI and Jenkins integration tests are now passing. |
I've rebased this branch again, resolved the merge conflicts, and pushed the results to the Edit: there was a failing CircleCI test after the last rebase, fixed in cd256f9, but Jenkins #337 passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only took a look at the functional test changes, which all look good to me barring one small nit. Nice work, excited to see this land!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a few comments but nothing major to fix I don't think. Mostly some artifacts of automated 2to3 conversion with odd imports and from __future__
additions. It's worth getting this in and then cleaning up later IMHO. I'm going to pull this and run it locally, but the code changes are looking good.
@@ -23,7 +23,7 @@ RUN gulp build --production | |||
######## | |||
# Python dependencies builder | |||
# | |||
FROM python:2-stretch AS python-builder | |||
FROM python:3-slim AS python-builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should pin a Python minor version just to be sure we have consistent builds. python:3.7-slim
e.g..
@@ -34,6 +34,7 @@ ENV PATH="/venv/bin:$PATH" | |||
COPY docker/bin/apt-install /usr/local/bin/ | |||
RUN apt-install gettext build-essential libxml2-dev libxslt1-dev libxslt1.1 | |||
|
|||
RUN pip install virtualenv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Python 3 venv is built in. You change these 2 lines to just RUN python -m venv /venv
bedrock/externalfiles/management/commands/update_externalfiles.py
Outdated
Show resolved
Hide resolved
@@ -491,8 +489,6 @@ def get_template_names(self): | |||
template = 'firefox/developer/firstrun.html' | |||
else: | |||
template = 'firefox/dev-firstrun.html' | |||
elif show_62_firstrun(version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this meant to be included in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this along the way because it appears to be redundant: the template matches the else
clause below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Yeah. Looks right. Just looked out of place in this PR. 👍
Running locally seems to work well. Just did |
Since this is such a sweeping change, I thought it is probably a good time to fix the flake8 exceptions that had been chucked in |
Ok, I've made all the changes pertaining to actual code rather than ops or decisions over whether code should have been included (looks like that one might be something missed in the recent rebase?). Let's chat tomorrow but I think we're pretty close to done now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the latest changes to the run-integration-tests
branch one last time, giving us another green pipeline. Assuming @alexgibson agrees, I'd like to merge this and deploy to production in the morning.
If all seems well no objection here 👍 |
Upgrade to Django 2.2 and Python 3
I rebased against the Django 1.11 merged branch, then merged with master - had to deal with a few conflicts but was mostly trivial.
Tests for
bedrock
andlib
are passing locally. @jgmize Let's do a more thorough test and see how things are looking...