implement SCL3 httpd redirects in Django#4215

bookshelfdave · 2017-05-04T20:40:32Z

This PR uses @pmac's https://github.com/pmac/django-redirect-urls lib to implement SCL3 Apache httpd redirects in Python/Django. Original rewrite rules are saved as comments to each Django redirect. Test layout is similar to the Bedrock redirect tests.

Some (or all) of these redirects may be unusued/leftover from A Long Time Ago.

The only rewrite rule that isn't implemented in this PR is as follows:

RewriteCond %{SERVER_NAME} ^developer\.mozilla\.com$
RewriteRule (.*) http://developer.mozilla.org$1 [R=301,L]

which redirects requests to developer.mozilla.com/* --> developer.mozilla.org/*

Tracked in this infra issue

cc @jgmize @pmac @jwhitlock @escattone

This site was a great help when working with mod_rewrite.

jgmize · 2017-05-04T21:10:15Z

kuma/settings/common.py


    # other
    'kuma.humans',
+    'redirect_urls',


Is this needed? I would think that the kuma.redirects app added above should be sufficient.

It is needed. This loads the machinery that then goes to find the redirects.py files in the other apps.

pmac · 2017-05-04T21:11:13Z

kuma/redirects/redirects.py

+redirectpatterns = [
+    # RewriteRule ^/media/(redesign/)?css/(.*)-min.css$
+    # /static/build/styles/$2.css [L,R=301]
+    redirect(r'^media/(?:redesign/)?css/(?P<doc>.*)-min.css$',


Might want the prepend_locale=False arg on some/most of these. Probably works without, but more regex than you need.

jwhitlock

An empty file is needed at tests/redirects/__init__.py Update - was tests_redirects.py, which makes no sense

After adding that file, I was able to run py.test tests/redirects/test_redirects.py, and get 6 passing tests. I'll have to read a little more of tests/redirects/base.py to see what that means...

There's a few more items, noted in the diff, but this is the first 5 min feedback.

jwhitlock · 2017-05-04T21:25:28Z

requirements/dev.txt

 Werkzeug==0.11.4 \
    --hash=sha256:7db3cb2d4725be0680abf64a45b18229186f03ad8b9989abbe053f9357b17b37 \
 --hash=sha256:e48fb7e3f2bb5a740dd9a666624699a4d83e2e028555f9c46bcc8ecfc2cd8c32
+django-redirect-urls==0.1 \


dev.txt includes default.txt, so this should be removed

jwhitlock · 2017-05-04T21:27:40Z

requirements/test.txt

 pytest-base-url==1.1.0 \
    --hash=sha256:90a1ce6a00a558231117b5aee32300edecbc0c2fd701c13e8cec62177900d28c \
    --hash=sha256:001c1b2678dae82c2891db415251361b29ed6824cb2085e4821635119499071e
+pytest-django==3.1.2 \


Is pytest-django actually needed in functional tests?

I couldn't test redirects without it.

jwhitlock · 2017-05-04T21:28:54Z

tests/redirects/map_301.py

@@ -0,0 +1,34 @@
+import pytest


pytest imported but unused

jwhitlock · 2017-05-04T21:29:05Z

tests/redirects/map_301.py

@@ -0,0 +1,34 @@
+import pytest
+from redirect_urls import redirect


redirect imported but unused

jwhitlock · 2017-05-04T21:29:51Z

tests/redirects/test_redirects.py

@@ -0,0 +1,14 @@
+from __future__ import absolute_import
+from operator import itemgetter


itemgetter imported but unused

jwhitlock · 2017-05-04T21:30:42Z

kuma/redirects/models.py

@@ -0,0 +1 @@
+from django.db import models


can this file be omitted?

I believe the file is required for django to consider it an "app" (that could be old info though). It can be empty though. Looks like the default models.py when you use manage.py startapp.

jwhitlock · 2017-05-04T21:31:07Z

requirements/test.txt

+pytest-django==3.1.2 \
+    --hash=sha256:038ccc5a9daa1b1b0eb739ab7dce54e495811eca5ea3af4815a2a3ac45152309 \
+    --hash=sha256:00995c2999b884a38ae9cd30a8c00ed32b3d38c1041250ea84caf18085589662
+django-redirect-urls==0.1 \


I think this is unused in functional tests

jwhitlock · 2017-05-04T21:42:31Z

Also, moving tests/utils/urls.py to tests/redirects/base.py will break functional (Selenium) tests. Instead, it should be imported like tests/function/test_home.py does: from utils.urls import assert_valid_url

codecov-io · 2017-05-05T12:09:42Z

Codecov Report

Merging #4215 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4215      +/-   ##
==========================================
+ Coverage   86.31%   86.31%   +<.01%     
==========================================
  Files         147      148       +1     
  Lines        9103     9105       +2     
  Branches     1222     1222              
==========================================
+ Hits         7857     7859       +2     
  Misses       1005     1005              
  Partials      241      241

Impacted Files	Coverage Δ
kuma/settings/common.py	`93.33% <ø> (ø)`	⬆️
kuma/redirects/redirects.py	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 368b6f3...83d97d6. Read the comment docs.

bookshelfdave · 2017-05-05T15:12:07Z

going to split this into 2 PRs: one for requirements so the base Docker image can be updated, and another with the redirects impl.

implement SCL3 httpd redirects in Django

9269e61

bookshelfdave mentioned this pull request May 4, 2017

Analysis of current SCL3 Apache config, and possible conversion to Python mozmeao/infra#180

Closed

fix formatting

d285f14

jgmize reviewed May 4, 2017

View reviewed changes

pmac reviewed May 4, 2017

View reviewed changes

jwhitlock suggested changes May 4, 2017

View reviewed changes

bookshelfdave added the not ready label May 5, 2017

bookshelfdave added 3 commits May 5, 2017 07:50

additional redirects

6b4e06a

prepend_locale=False

6942cba

cleanup imports

efcf6ce

bookshelfdave added 2 commits May 5, 2017 10:26

fixed mozillademos tests

9e0afc7

all your test are belong to us, will squash/rebase

83d97d6

bookshelfdave closed this May 5, 2017

bookshelfdave mentioned this pull request May 5, 2017

implement SCL3 httpd redirects in Django #4220

Merged

		@@ -0,0 +1,34 @@
		import pytest
		from redirect_urls import redirect

		@@ -0,0 +1,14 @@
		from __future__ import absolute_import
		from operator import itemgetter

Conversation

bookshelfdave commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwhitlock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwhitlock commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bookshelfdave commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bookshelfdave commented May 4, 2017 •

edited

Loading

jwhitlock left a comment •

edited

Loading

jwhitlock commented May 4, 2017 •

edited

Loading

codecov-io commented May 5, 2017 •

edited

Loading