Remove logic for guessing slug from an unregistered domain #4730
Conversation
| self.middleware.process_request(request) | ||
| self.assertEqual(request.urlconf, self.urlconf_subdomain) | ||
| self.assertEqual(request.cname, True) | ||
| self.assertEqual(request.rtdheader, True) | ||
| self.assertEqual(request.slug, 'pip') | ||
|
|
||
| @override_settings(USE_SUBDOMAIN=True) |
There was a problem hiding this comment.
We already put this on the class
stsewd
changed the title
|
@stsewd I'm a bit worried this might break our users CNAME's if they aren't properly configured. Can we put together a PR that upgrades the |
|
(It can be another PR, so we can merge this when we're ready) |
|
I opened #4769 |
|
Looks like travis is still busy p: |
Codecov Report
@@ Coverage Diff @@
## master #4730 +/- ##
==========================================
- Coverage 76.85% 76.29% -0.56%
==========================================
Files 158 158
Lines 9989 10007 +18
Branches 1254 1262 +8
==========================================
- Hits 7677 7635 -42
- Misses 1978 2029 +51
- Partials 334 343 +9
|
I agree with @ericholscher that we need to do something with these people before deploying this. It's fine to have a good documentation saying how users should configure their domains, but we know there are many people that didn't follow those instructions properly (from the Cloudflare feature) and now we should probably create their Domain objects for their projects or at least warn these users. On the other hand, I'm personally interested in merging this PR because it removes the I'm marking this PR as blocked because of this. |
|
We were talking about it would be good to make a plan to deprecate this:
So, in the near future, we will have all these projects properly configured and also the new ones, because our new docs are correct but also because they will fail otherwise. At the moment, after the deploy, Eric already found at least one project with this issue. |
|
Looks like codecov isn't happy with the coverage from the diff 😢 I'll need to figure out how to increase the coverage here... |
|
We hit issues with producing Domain objects for requests for projects without a Domain setup in the past. It lead to projects overtaking known slugs. For example, I think the case we hit was a well known project didn't have a Domain, and some rando domain was resolving to the project. We then assigned the rando Domain to the project as canonical. 👍 on adding a feature flag with migration that changes the behavior going forward. I'm 👎 on adding magic logic to continue adding new domains for domain-less projects. New projects should have a domain set up correctly, so I'm not worried about these projects being misconfigured. I do think we should get away from using magic to set up projects that are not configured well. Explicit is better than magic from a code maintenance perspective. |
|
If I understand correctly, the actionable part of this PR is this #4730 (comment) together with #4730 (comment). Can we unblock it? |
|
We should have some logs from this actually #4769 |
|
A quick update here:
|
Those are new projects
Should we take manual action?
|
|
What timeframe are those stats for? I think if it's <100, we can probably just create them manually. I do think we'll still end up breaking some subset of users, but if we use the last week or so of queries, that should get everything in real use. |
|
Those numbers come from parsing logs from |
|
So that's from the last 10 days on web02? |
|
Yes |
|
I messed up trying to solve the conflicts (wrong option in the merge command). I opened #5143. Closing here. |
Close #1991