-
-
Notifications
You must be signed in to change notification settings - Fork 732
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
feat: PostgresProvider - Add POSTGRES_URL_OVERRIDE #3395
feat: PostgresProvider - Add POSTGRES_URL_OVERRIDE #3395
Conversation
docs/docs/documentation/getting-started/installation/postgres.md
Outdated
Show resolved
Hide resolved
docs/docs/documentation/getting-started/installation/postgres.md
Outdated
Show resolved
Hide resolved
docs/docs/documentation/getting-started/installation/postgres.md
Outdated
Show resolved
Hide resolved
docs/docs/documentation/getting-started/installation/postgres.md
Outdated
Show resolved
Hide resolved
…STGRES_URL_OVERRIDE
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.
LGTM, will defer to @boc-the-git for merge
@@ -33,6 +33,7 @@ | |||
| POSTGRES_SERVER | postgres | Postgres database server address | | |||
| POSTGRES_PORT | 5432 | Postgres database port | | |||
| POSTGRES_DB | mealie | Postgres database name | | |||
| POSTGRES_URL_OVERRIDE | None | Postgres URL override. Must be used with DB_ENGINE: postgres | |
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 don't like the wording "Must" here - it implies it's mandatory when in fact it's optional.
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.
It's not optional if using the variable. You must define that or it will not work because the provider is created based on DB_ENGINE.
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.
Yea, I understand the message you're trying to communicate, but I don't agree that the words achieve that. Otherwise that same message should be on all the POSTGRES_x
env vars?
Something that talks to what it replaces would make more sense, e.g. (don't quote me, I've given this very little thought!!) "Postgres URL override. Used in place of POSTGRES_x/POSTGRES_y/... if you want to specify the full connection URI"
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 is what I came up with: "Optional Postgres URL override to use instead of POSTGRES_* variables. Must be used in conjunction with DB_ENGINE: postgres to have an effect."
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.
It's absolutely an improvement, I just still find this a weird addition Must be used in conjunction with DB_ENGINE: postgres to have an effect.
, per previous comments.
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'd be nervous to not add something similar as this is a quirk of the implementation.
The URL includes the dialect already, so its quite weird to have it specific to postgres if I am honest here.
If you don't specify it must still be used in conjunction with DB_ENGINE: postgres, someone may think that specifying DB_ENGINE is redundant, and remove it, only to find the instance has gone back to sqlite.
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.
Pretend this new variable doesn't exist..
If a user does not specify DB_ENGINE: postgres
, they get a sqlite DB already, right?
It looks that way to me, based on https://github.com/mealie-recipes/mealie/blob/mealie-next/mealie/core/settings/settings.py#L215
I'm all for the docs being improved, but adding this exact message, to this one environment variable, adds more confusion rather than simplifying things.
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.
Changed.
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.
One doco change needed, but otherwise happy for this to be merged - even ahead of the security release if that's how the timing works out. I don't see this as being a risky change given it's a new env var being introduced.
Oh, and I'm crowd sourcing an opinion to the change of how timezone is set in the sample files.. that's on Discord.
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.
👍
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3392
Testing
1). Installed postgres locally into a mealie devcontainer.
2). Created a database for mealie
3). Tested multiple values of POSTGRES_URL (omitted, specified using default config, specified using file socket). using task dev:services and task py:postgres for more complete testing.