Adds support for ALLOW_EMPTY_PASSWORD
in bitnami/postgresql
.
Comment From: wilkinsona
Thanks for the PR. I think this is really doing four separate things:
- Correcting the environment variable names for the Postgres username and password. This is a bug that affects Spring Boot 3.3.x and later.
- Adding support for
ALLOW_EMPTY_PASSWORD
with Postgres. This is arguably an enhancement or could, perhaps, be treated as a bug of omission. - Correcting the Clickhouse support when checking the value of
ALLOW_EMPTY_PASSWORD
. This is a bug that affects Spring Boot 3.4.x and later. - Changing how the MariaDB and MySQL support handles the
ALLOW_EMPTY_PASSWORD
environment variable. It's debatable that this is needed as checking for the presence ofALLOW_EMPTY_PASSWORD
is a reasonable approximation. If we want to tighten things up, we could do so as a task in 3.3.x and later.
Can you please split your PR up into four new PRs, one for each of the above? If you'd prefer to avoid that overhead, please let us know and we can open issues and take care of things. Either way, thanks again for this PR and for bringing the need for some changes to our attention.
Comment From: hezean
@wilkinsona I appreciate your reply and am willing to take any necessary action :)
Could you kindly provide some guidance on how can I split this PR, as point 2, 3, 4 you mentioned share the same isBooleanYes
method. I'm considering the following approach:
- Open a new PR to fix the
POSTGRESQL_USERNAME
andPOSTGRESQL_DATABASE
issues - Force push to this PR, leaving only the
BitnamiUtils
and the fix for Clickhouse - After this PR is merged, open two new PRs: for Postgres, and for MySQL/Maria
Comment From: wilkinsona
Rather than introducing BitnamiUtils
, which has to be public API due to it being used in multiple packages, I think I'd repeat the logic in each place that it's needed. We can either just look for ALLOW_EMPTY_PASSWORD
and ignore its value (Bitnami will still check it), or we can support yes
as that appears to be the value that's used in the various READMEs for the Bitnami images and is recommended in the error message (see here for example) when an empty password is detected and ALLOW_EMPTY_PASSWORD
hasn't been set. ClickHouse may need to be an exception to this in case someone's already relying on true
working.
Comment From: hezean
Noted, that way I will put multiple isBooleanYes
as private methods for each service connection :)
ClickHouse may need to be an exception to this in case someone's already relying on
true
working.
Please note that Bitnami treats 1
, true
, and yes
as true value, refer to their impl, so we are not break anything.
Comment From: wilkinsona
For simplicity, I don't think we should support 1
, true
, and yes
. It appears to only be yes
that is documented. ClickHouse is a possible exception to this as the logic implemented in Boot currently only supports true
:
https://github.com/spring-projects/spring-boot/blob/c847ce4a8b82fddcc80823040c04e2549d530fb6/spring-boot-project/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/service/connection/clickhouse/ClickHouseEnvironment.java#L44
We might want to support both true
and yes
in this one case to avoid a breaking change.
That said, I'd prefer that we just consistently look for ALLOW_EMPTY_PASSWORD
being set to any value and allow Bitnami to do the actual checking. That isolates Boot from any changes to what Bitnami supports and/or documents in the future. This isolation is important as Boot doesn't control the version of any Bitnami container that's used so we don't control when a user may try to use a container with different rules for the value of ALLOW_EMPTY_PASSWORD
.
Comment From: hezean
Blocked by #43787 for parsing the username and database
Comment From: hezean
For simplicity, I don't think we should support
1
,true
, andyes
. It appears to only beyes
that is documented. ClickHouse is a possible exception to this as the logic implemented in Boot currently only supportstrue
:https://github.com/spring-projects/spring-boot/blob/c847ce4a8b82fddcc80823040c04e2549d530fb6/spring-boot-project/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/service/connection/clickhouse/ClickHouseEnvironment.java#L44
We might want to support both
true
andyes
in this one case to avoid a breaking change.That said, I'd prefer that we just consistently look for
ALLOW_EMPTY_PASSWORD
being set to any value and allow Bitnami to do the actual checking. That isolates Boot from any changes to what Bitnami supports and/or documents in the future. This isolation is important as Boot doesn't control the version of any Bitnami container that's used so we don't control when a user may try to use a container with different rules for the value ofALLOW_EMPTY_PASSWORD
.
Then, IMHO, simply changing the current logic for ClickHouse to containsKey
makes the code more consistent