-
Notifications
You must be signed in to change notification settings - Fork 398
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
Redirect /android?something to /android/?something #8117
Conversation
Codecov Report
@@ Coverage Diff @@
## master mozilla/addons-frontend#8117 +/- ##
=========================================
+ Coverage 98.09% 98.1% +<.01%
=========================================
Files 260 260
Lines 7363 7373 +10
Branches 1331 1332 +1
=========================================
+ Hits 7223 7233 +10
Misses 126 126
Partials 14 14
Continue to review full report at Codecov.
|
I think this should be fixed in the prefix middleware rather than introduce yet another place for redirects. |
That makes sense, although I was not able to fix it today.. I'll give it another try. This patch is needed anyway (without the android redirect) because we want to add new redirects in the very near future (and likely move some permanent redirects from nginx to this codebase) |
I've seen the discussion in the issue and it sounds like I should hold off reviewing this as possibly the approach is going to change? |
The approach to fix mozilla/addons#13207, yes. You can probably reuse this patch for #8105 though :) |
@willdurand In case it helps, I've proposed an independent prefix middleware patch to fix mozilla/addons#13207 over here: #8120 |
Fixes mozilla/addons#13207
This fixes mozilla/addons#13207 by introducing a server redirect instead of trying to patch the prefix middleware. This also adds a framework to add other server redirects since it seems we do not want them in Nginx.
Adding a new redirect rule with this framework can be done by:
SERVER_REDIRECTS
map (with a short and meaningful key)each()
array of the test case