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

implement SCL3 httpd redirects in Django #4220

Merged
merged 3 commits into from May 8, 2017

Conversation

Projects
None yet
3 participants
@metadave
Member

metadave commented May 5, 2017

previously: #4215

This PR uses @pmac's https://github.com/pmac/django-redirect-urls lib to implement SCL3 Apache httpd redirects in Python/Django. The complex rewrites retain original mod_rewrite rules as comments, while the remaining are uncommented simple static redirects. Test layout is similar to the Bedrock redirect tests.

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

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 5, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4220      +/-   ##
==========================================
+ 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 7c59c5e...4c9e6af. Read the comment docs.

codecov-io commented May 5, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4220      +/-   ##
==========================================
+ 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 7c59c5e...4c9e6af. Read the comment docs.

@jwhitlock

I think we need the base_url fixture, so that you can verify that the new Kuma code matches the behavior of production. The [url12] test of /media/uploads/demos/foobar looks to be a real bug.

As for the foo//bar tests. 😕 Maybe spend 15 minutes trying to fix localhost, and then comment out those tests for now or mark them as XFAIL if that's possible. We may need WebOps to sync an Apache config change from prod to staging.

Show outdated Hide outdated tests/redirects/test_redirects.py
@pytest.mark.nondestructive
@pytest.mark.parametrize('url', REDIRECT_URLS)
def test_redirects(url):
url['base_url'] = "https://developer.mozilla.org"

This comment has been minimized.

@jwhitlock

jwhitlock May 5, 2017

Member

This should match bedrock, using the base_url py.test fixture:

def test_redirects(url, base_url):
    url['base_url'] = base_url

You can then test it against staging:

py.test tests/redirects -vv --base-url https://developer.allizom.org --pdb

or the local Docker:

py.test tests/redirects -vv --base-url http://localhost:8000 --pdb

The --pdb drops you into the Python debugger, where you can print out the expected and actual values ("q" and Enter to quit). If we were on py.test 3.x, the assets may be more useful (see PR #4210), but w/ py.test 2.x, you have to do your own test debugging.

@jwhitlock

jwhitlock May 5, 2017

Member

This should match bedrock, using the base_url py.test fixture:

def test_redirects(url, base_url):
    url['base_url'] = base_url

You can then test it against staging:

py.test tests/redirects -vv --base-url https://developer.allizom.org --pdb

or the local Docker:

py.test tests/redirects -vv --base-url http://localhost:8000 --pdb

The --pdb drops you into the Python debugger, where you can print out the expected and actual values ("q" and Enter to quit). If we were on py.test 3.x, the assets may be more useful (see PR #4210), but w/ py.test 2.x, you have to do your own test debugging.

url_test("/media/uploads/demos/foobar123",
"/docs/Web/Demos_of_open_web_technologies/",
status_code=requests.codes.found),

This comment has been minimized.

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base-url=http://localhost:8000. It expects a 302 Found, but gets a 301 Moved Permanently.

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base-url=http://localhost:8000. It expects a 302 Found, but gets a 301 Moved Permanently.

Show outdated Hide outdated tests/redirects/map_301.py
"/docs/Web/Demos_of_open_web_technologies/",
status_code=requests.codes.found),
url_test("/foo//bar", "/foo_bar"),

This comment has been minimized.

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base_url=localhost:8000, getting http://localhost:8000/foo//foo_bar.

It also fails against --base_url=https://developer.allizom.org, getting https://en-US/foo/bar. I think the staging server's Apache config is wrong.

I also question the purpose of this pattern, but I guess it was a problem at some point (see bug 779501)

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base_url=localhost:8000, getting http://localhost:8000/foo//foo_bar.

It also fails against --base_url=https://developer.allizom.org, getting https://en-US/foo/bar. I think the staging server's Apache config is wrong.

I also question the purpose of this pattern, but I guess it was a problem at some point (see bug 779501)

Show outdated Hide outdated tests/redirects/map_301.py
status_code=requests.codes.found),
url_test("/foo//bar", "/foo_bar"),
url_test("/foo//bar//baz", "/foo_bar_baz"),

This comment has been minimized.

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base-url=http://localhost:8000, getting http://localhost:8000/bar_baz

It also fails against --base-url=https://developer.allizom.org, getting https://developer.allizom.org/f/en-US/foo/bar/baz.

WEIRD.

@jwhitlock

jwhitlock May 5, 2017

Member

This fails against --base-url=http://localhost:8000, getting http://localhost:8000/bar_baz

It also fails against --base-url=https://developer.allizom.org, getting https://developer.allizom.org/f/en-US/foo/bar/baz.

WEIRD.

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock May 8, 2017

Member

There's some missing stuff:

  • 3 Tests that pass in developer.mozilla.org but fail in developer.allizom.org or Docker dev
  • The .htaccess redirects
  • Tests are not integrated into acceptance tests

but, I'm happy with what is here so far. It's up to @metadave if he wants to keep working in this PR, or merge and add further refinements in a future PR.

Member

jwhitlock commented May 8, 2017

There's some missing stuff:

  • 3 Tests that pass in developer.mozilla.org but fail in developer.allizom.org or Docker dev
  • The .htaccess redirects
  • Tests are not integrated into acceptance tests

but, I'm happy with what is here so far. It's up to @metadave if he wants to keep working in this PR, or merge and add further refinements in a future PR.

@metadave

This comment has been minimized.

Show comment
Hide comment
@metadave

metadave May 8, 2017

Member

@jwhitlock I pushed some fixes to get all the tests to pass on http://localhost:8000 and https://developer.mozilla.org, however there are 2 remaining failures on https://developer.allizom.org related to the following tests:

    url_test("/docs/Mozilla/Projects/NSPR/Reference/I//O_Functions", "/docs/Mozilla/Projects/NSPR/Reference/I_O_Functions"),
    url_test("/docs/Mozilla/Projects/NSPR/Reference/I//O//Functions", "/docs/Mozilla/Projects/NSPR/Reference/I_O_Functions"),

which correspond to the following rewrites:

RewriteRule ^(.*)//(.*)//(.*)$ $1_$2_$3 [R=301,L,NC]
RewriteRule ^(.*)//(.*)$ $1_$2 [R=301,L,NC]
# Legend:
#   R = redirect to a specific http response (301)
#   L = don't try any more rewrite rules
#   NC = case insensitive

I'd prefer to submit * the .htaccess redirects in a separate PR.

I'll wait for Travis to turn green and merge.

Member

metadave commented May 8, 2017

@jwhitlock I pushed some fixes to get all the tests to pass on http://localhost:8000 and https://developer.mozilla.org, however there are 2 remaining failures on https://developer.allizom.org related to the following tests:

    url_test("/docs/Mozilla/Projects/NSPR/Reference/I//O_Functions", "/docs/Mozilla/Projects/NSPR/Reference/I_O_Functions"),
    url_test("/docs/Mozilla/Projects/NSPR/Reference/I//O//Functions", "/docs/Mozilla/Projects/NSPR/Reference/I_O_Functions"),

which correspond to the following rewrites:

RewriteRule ^(.*)//(.*)//(.*)$ $1_$2_$3 [R=301,L,NC]
RewriteRule ^(.*)//(.*)$ $1_$2 [R=301,L,NC]
# Legend:
#   R = redirect to a specific http response (301)
#   L = don't try any more rewrite rules
#   NC = case insensitive

I'd prefer to submit * the .htaccess redirects in a separate PR.

I'll wait for Travis to turn green and merge.

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock May 8, 2017

Member

Nice work @metadave. I'll diff the staging and production configuration, and see why there is a difference for those URL patterns.

Member

jwhitlock commented May 8, 2017

Nice work @metadave. I'll diff the staging and production configuration, and see why there is a difference for those URL patterns.

@jwhitlock jwhitlock merged commit 624ad69 into master May 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jwhitlock jwhitlock deleted the dp_httpd_redirects branch May 8, 2017

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock May 17, 2017

Member

I've looked at the staging and production Apache configuration, and the two given rewrite rules are missing from staging. I've opened infra bug 1365774 to request syncing the two configurations.

Member

jwhitlock commented May 17, 2017

I've looked at the staging and production Apache configuration, and the two given rewrite rules are missing from staging. I've opened infra bug 1365774 to request syncing the two configurations.

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock May 23, 2017

Member

The configurations have been synced, and the tests now pass against staging: py.test tests/redirects/. I think we're ready to add these tests to the integration test suite.

Member

jwhitlock commented May 23, 2017

The configurations have been synced, and the tests now pass against staging: py.test tests/redirects/. I think we're ready to add these tests to the integration test suite.

@metadave metadave referenced this pull request Jul 21, 2017

Closed

MDN migration #253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment