-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Proxito: move "canonicalizing" logic to docs view #10027
Conversation
This only applies to doc serving, so no need to have it at the middleware level, unless we want to enforce this for all views.
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.
Looks good to me! I left some questions and comments.
self.assertEqual(request.canonicalize, 'noncanonical-cname') | ||
resp = self.client.get(self.url, HTTP_HOST=cname) | ||
self.assertEqual(resp.status_code, 302) | ||
self.assertEqual(resp["X-RTD-Redirect"], "noncanonical-cname") |
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.
Shouldn't we create a RedirectType.to_non_canonical_domain
and use it here?
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.
We aren't setting that value from anywhere, this test is marked as fail too. This looks like a feature we wanted to implement at some point, I'm +1 on just deleting these tests.
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.
Requests to a custom domain should redirect to the public domain or canonical domain if not canonical.
I guess that's when users don't have any custom domain set as canonical
we want to redirect to the public domain?
@ericholscher may have more information about this "lost feature"
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.
#6905 (comment) I think is the conversation. I think it's mostly just awkward to redirect people away from custom domains. There are use cases where people might want to have docs on a custom domain, but not have it be canonical.
So I'm +1 on deleting this I think.
I'm merging this now, but @ericholscher if you have some insight about #10027 (comment), so we can remove those tests or open a new issue for that feature if needed. |
This only applies to doc serving, so no need to have it at the middleware level, unless we want to enforce this for all views.