-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Remove unnecessary limitation for allowed hosts in development environments #2137
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Technically this could just be done in the docker settings because I think it's only related to running with docker due to the docker network.
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.
I agree with @marksweb , what is the purpose of changing this part?
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.
The purpose is to remove a broken setting that creating extra complexity with no benefit; refactoring.
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.
0.0.0.0 is not related to the Docker network.
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.
Have you run the site with this configuration, accessed all of the different domains listed in the file, and confirmed that they display the appropriate site?
If the proper host name is not getting through to Django, it seems like the problem needs to be fixed somewhere else.
Uh oh!
There was an error while loading. Please reload this page.
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.
About foreman, not sure why you think it's wrong but accessing a local project over 0.0.0.0, 127.0.0.1 or localhost is common practice, which I see no issue in having mentioned in a doc. Having the mention of foreman, and the procfile in the repo, is something I'd like to discuss but that would be a separate refactoring discussion.
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.
As I said before, my recommendation is to update the README. This is not a workaround; these are the actual domains you need to use to access the development site, as listed in the README, the
ALLOWED_HOSTS
setting, thedev_sites
fixture, and the django-hosts configuration (djangoproject/hosts.py
). They're also the domains used by the test suite. It's the same for the non-Docker setup, and theALLOWED_HOSTS
settings helps you see that you're not using the development server as it was intended.If you would like to propose a change to allow the main site to be access on
localhost:8000
instead of or in addition towww.djangoproject.localhost:8000
, the change deserves its own issue where the @django/django-website team and others who work on the site can discuss and decide how to move forward.Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks for the clarification. So, we need developers to access the project locally via specific hostnames 👍🏻 Blocking undesired hosts via
ALLOWED_HOSTS
provides only a partial solution for that need and creates further confusion. It blocks access in particular scenarios, but it doesn't help to find the right entry points or provide any information that the developer is doing something undesired. I think a better solution could be to apply the changes in this PR, and then follow up with forwarding requests from common entry points, such as "0.0.0.0" and "127.0.0.1", to the desired hostnames. I'll create an issue to have a healthier discussion 🌻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.
Yeah I don't think that's what I was getting at.
The zero IP allows you to bind all interfaces. It's an easy "allow all" type approach.
Looking back at the screenshots, it's not something you'd then use to connect from the browser.
Uh oh!
There was an error while loading. Please reload this page.
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.
This can be updated/improved, but I don't have the energy left to start a discussion about another arbitrary choice.
This is not correct. Hosts config doesn't check for the domain:
So django-hosts doesn't care if you access it via
docs.0.0.0.0
,docs.localhost
ordocs.djangoproject.localhost