-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
Details for different port/address added #3124
base: master
Are you sure you want to change the base?
Conversation
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 working on improving this!
I have some suggestions to limit the amount of modifications required and streamline the setup a bit.
docs/developers/devel-env.rst
Outdated
.. note:: | ||
|
||
If you are accessing your development server using a port other than ``8100``, | ||
ensure that you update the ``SERVER_ROOT_URL`` field to reflect the appropriate port number. |
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.
There are potentially quite a few more configuration lines that would need to be modified, so I wonder if a more generic advice such as "search for 8100
in the config file and replace it with the port you want to use" or something along those lines would be more suited and more future-proof.
For example, API_URL, SPOTIFY_CALLBACK_URL and other callback URLs use the port number.
Alternatively, you could modify the sample config again to replace localhost:8100
with SERVER_ROOT_URL
in an f-string like we do for MUSICBRAINZ_BASE_URL
:
OAUTH_AUTHORIZE_URL = f"{MUSICBRAINZ_BASE_URL}/new-oauth2/authorize" |
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.
Made the necessary changes by using a f-string format.
listenbrainz/config.py.sample
Outdated
@@ -22,6 +22,9 @@ SQLALCHEMY_TIMESCALE_URI = "postgresql://listenbrainz_ts:listenbrainz_ts@lb_db/l | |||
TIMESCALE_ADMIN_URI = "postgresql://postgres:postgres@lb_db/postgres" | |||
TIMESCALE_ADMIN_LB_URI = "postgresql://postgres:postgres@lb_db/listenbrainz_ts" | |||
|
|||
# Server address | |||
SERVER_NAME = "localhost:8100" |
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.
Can you explain why setting this SERVER_NAME is required?
I only see it use in the testing framework so i am wondering.
As mentioned in my previous comment, this could potentially use the value of SERVER_ROOT_URL
?
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.
In situations where generating absolute external URLs is required, such as during OAuth redirection, Flask relies on the SERVER_NAME
configuration. Without setting SERVER_NAME
, Flask defaults to localhost
, which can lead to incorrect or invalid redirect URLs. To ensure OAuth and similar features point to the correct local or production URL, it is necessary to configure SERVER_NAME
with the actual base URL of the application.
While SERVER_ROOT_URL
serves as a full external-facing URL, it is not directly usable for Flask’s purposes since it includes the protocol (http://
or https://
), which Flask does not interpret in SERVER_NAME
. Therefore, the two values, while related, cannot be interchanged directly.
You can refer to the attached Flask Documentation.
listenbrainz/config.py.sample
Outdated
@@ -211,4 +214,4 @@ REJECT_LISTENS_WITHOUT_USER_EMAIL = False | |||
REJECT_NEW_USERS_WITHOUT_EMAIL = False | |||
|
|||
# base directory for user data exports | |||
USER_DATA_EXPORT_BASE_DIR = "/code/listenbrainz/exports/" | |||
USER_DATA_EXPORT_BASE_DIR = "/code/listenbrainz/exports/" |
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.
Newline deleted automatically? You might need to adjust your code editor settings to not remove them automatically.
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.
Will look into it.
…enbrainz-server into config-docs-update
Problem
The documentation lacked information about necessary configuration changes for users working with a port other than
8100
or a host other thanlocalhost
.Solution
Updated the configuration file to include details for working with different ports or hosts. Added a note in the server documentation for user assistance regarding the same:
8100
, update theSERVER_ROOT_URL
field with the correct port number.SERVER_NAME
andSERVER_ROOT_URL
fields with the appropriate host details.Action