Skip to content
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

Fixed #35335 -- Updated "Enabling the sites framework" docs to reiterate usage of get_current_site. #17977

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sdarwin
Copy link

@sdarwin sdarwin commented Mar 14, 2024

Hi,
When learning the sites framework, a few questions came to mind and needed to be researched. The idea with this pull request is to address those topics. Feedback is welcome.

  • When considering adding "sites" to our Django site in order to enable a feature of Allauth, it was not clear if the typical implementation of "sites" could be done with one application server (ideally) or must be scaled across multiple servers, each with their own SITE_ID (which would be problematic). It seems the original design would require that. Thus, the sentences about "multiple app servers" to explain the point. I think it's interesting information.

  • added the word "optional" to the directive that you must add a SITE_ID in the settings.

  • added a mention about what is "recommended". If you believe that is going too far, and it's not recommended, it could be edited.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@mahiuddin-dev
Copy link

Yes. I like your point

@nessita
Copy link
Contributor

nessita commented Mar 27, 2024

As mentioned in the ticket, I think the changes should focus only on updating the "Enabling the sites framework" to reiterate the ability to use get_current_site.

@nessita nessita changed the title Docs: Updated content of "sites" framework page. Fixed #35335 -- Updated "Enabling the sites framework" docs to reiterate usage of get_current_site. Mar 27, 2024
@sdarwin
Copy link
Author

sdarwin commented Mar 27, 2024

Hi Nessita,

I see that you are taking issue with the word "server".

To revisit that particular point, in the original implementation of "sites", where you would hard-code the SITE_ID in the settings file, then in that case... imagine if you would like to have varying behavior for multiple so-called "sites". One nginx vhost, in a docker container, isn't enough. Horizontally scaled replicas of that same container with the same files isn't enough. You really have to do something "extra". And for that I am calling them "servers", to emphasis the point. Most features of Django will "just work" on your one server, when you update the python code. But not this. Not a static SITE_ID. So perhaps the wording could be altered, and I am open to suggestions. "server" conveyed the idea. Perhaps "instances" or "vhosts" or "deployment" instead. In reality, in the world of k8s, you'd probably have multiple deployments. Or multiple VMs.

By the way, pytest is failing on the Boost website if SITE_ID is missing. :-) But per this very issue here, it should be ok , to not set that variable.

@sdarwin sdarwin force-pushed the sites branch 2 times, most recently from a6db09e to 3ee0702 Compare April 10, 2024 15:03
@sdarwin
Copy link
Author

sdarwin commented Apr 10, 2024

After having completed a "sites" framework deployment, and thinking about the situation further, I see that it's more involved and nuanced than just recommending get_current_site(request). Even though get_current_site(request) has more features the most common installations of Django will not use that functionality or multiple domains. They are better off coding a fixed SITE_ID.

With that in mind, I have updated the PR.

Also, the words "server" and "recommended" have been removed.

One way to propose an update, although not required, is to copy-paste a whole section into this github conversation, and modify the text directly. That makes it clear what the modification is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants