-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[bitnami/openldap] Correct naming and location for some organization units #51192
Conversation
Signed-off-by: barsikus007 <[email protected]>
Signed-off-by: barsikus007 <[email protected]>
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
hey? |
Oh I get it |
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
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.
Hi @barsikus007,
Thank you very much for your contribution, but I'm worried about how this change would affect existing users.
Do you think you could make this change backward-compatible? For example, instead of removing it, you could set LDAP_USER_OU/LDAP_GROUP_OU
to LDAP_USER_DC
if it was set and warn users about that environment variable being deprecated in favor of the new ones.
That way, the image functionality won't be affected and users will have time to modify their deployments, so we can safely remove the deprecated setting in a future release.
Okay, but how to notify users about changes? |
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.
Please take a look at my suggestions
export LDAP_USER_OU="${LDAP_USER_OU:-users}" | ||
export LDAP_GROUP_OU="${LDAP_GROUP_OU:-groups}" |
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 in my suggestion, please keep the value LDAP_USER_DC
but remove its default value.
Then, for LDAP_USER_OU
and LDAP_GROUP_OU
it will have the following priorities:
- Value provided in
LDAP_USER_OU
/LDAP_GROUP_OU
- If no value provided and
LDAP_USER_DC
, use value inLDAP_USER_DC
. - If neither
LDAP_USER_OU
/LDAP_GROUP_OU
orLDAP_USER_DC
provided, then use default valuesuser/group
.
export LDAP_USER_OU="${LDAP_USER_OU:-users}" | |
export LDAP_GROUP_OU="${LDAP_GROUP_OU:-groups}" | |
export LDAP_USER_DC="${LDAP_USER_DC:-}" | |
export LDAP_USER_OU="${LDAP_USER_OU:-${LDAP_USER_DC:-users}}" | |
export LDAP_GROUP_OU="${LDAP_GROUP_OU:-${LDAP_USER_DC:-groups}}" |
Then, around line 174, inside ldap_validate
add the following message:
if [[ -n "$LDAP_USER_DC" ]]; then
warn "The env variable 'LDAP_USER_DC' has been deprecated and will be removed in a future release. Please use 'LDAP_USER_OU' and 'LDAP_GROUP_OU' instead."
fi
* `LDAP_USER_OU`: Name for the user's organizational unit. Default: **users** | ||
* `LDAP_GROUP_OU`: Name for the group's organizational unit. Default: **groups** |
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.
Finally, in the README.md, you could also warn about the deprecation:
* `LDAP_USER_OU`: Name for the user's organizational unit. Default: **users**
* `LDAP_GROUP_OU`: Name for the group's organizational unit. Default: **groups**
* `LDAP_USER_DC`: DC for the users' organizational unit. **DEPRECATED** Please use `LDAP_USER_OU` and `LDAP_GROUP_OU` instead.
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary. |
Description of the change
Fix of #19716 issue
Benefits
Correct and predictable location of groups organization unit (E.g. https://hub.docker.com/r/wheelybird/ldap-user-manager image expect to see groups in separate ou than users)
Possible drawbacks
It could break some configs, which is targeted to previous schema and env variables
Applicable issues
Additional information