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.

Sign up for GitHub

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

Jump to bottom

fix!: Allow protocol in SASLAUTHD_LDAP_SERVER & adjust SASLAUTHD_LDAP_ default values #1989

Merged
merged 9 commits into from May 22, 2021
Merged

fix!: Allow protocol in SASLAUTHD_LDAP_SERVER & adjust SASLAUTHD_LDAP_ default values #1989

merged 9 commits into from May 22, 2021

Conversation

moqmar
Copy link
Contributor

@moqmar moqmar commented May 20, 2021

Description

Deprecate SASLAUTHD_LDAP_SSL by allowing the protocol in SASLAUTHD_LDAP_SERVER & use more sane defaults for some SASLAUTHD_LDAP variables.

Fixes #1983

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project (I used the style of the code around my changes, I think there are some inconsistencies with the default values in the saslauthd section compared to the other code)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (will have to test this tomorrow when I have a better internet connection)

@moqmar moqmar force-pushed the bugfix/saslauthd-ldap-ssl branch 2 times, most recently from e1719ad to 09dd7b6 Compare May 20, 2021 08:55
@georglauterbach georglauterbach mentioned this pull request May 20, 2021
Merged
9 tasks
@georglauterbach georglauterbach added this to the v10.0.0 milestone May 20, 2021
@moqmar moqmar force-pushed the bugfix/saslauthd-ldap-ssl branch from 09dd7b6 to 02f233d Compare May 20, 2021 09:25
@georglauterbach
Copy link
Member

georglauterbach commented May 20, 2021 •
edited

Just FYI

This PR is to be merged before the release of v10 as it contains breaking changes:)

@georglauterbach
Copy link
Member

@moqmar please reach back to us when this PR is ready to be reviewed, so we can take action:)

@moqmar
chore(test): Test SASLAUTHD_LDAP_SERVER with protocol and ..._SSL=0, …
e7d975d 
…as well as with default bind credentials

Note that there are currently no regression tests for this as there's only one setup_file, so that would require big changes to the testing methodology.
@moqmar
Copy link
Contributor Author

moqmar commented May 20, 2021

Alright, I'd say if the checks run fine now then this is done!

Note that there are currently no regression tests for this as there's only one setup_file, so that would require big changes to the testing methodology.

moqmar added a commit to moqmar/docker-mailserver that referenced this pull request May 20, 2021
@moqmar
docs(ldap): Remove superfluous environment variables as of docker-mai…
68fa2fe 
…lserver#1989
polarathene added a commit that referenced this pull request May 21, 2021
@moqmar @wernerfred @polarathene
docs: Improve LDAP documentation (#1921)
44622e6 
* docs(ldap): Make DOVECOT_PASS_FILTER clearer and add a small DOVECOT_AUTH_BIND section

* docs(ldap): Remove superfluous environment variables as of #1989

* docs(ldap): Document defaults for DOVECOT_*_ATTRS/FILTER

* docs(ldap): Add documentation for LDAP with TLS and StartTLS

Co-authored-by: Frederic Werner <20406381+wernerfred@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
wernerfred
wernerfred approved these changes May 22, 2021
View reviewed changes
Copy link
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Anything else needed from your side @moqmar?

If not @docker-mailserver/maintainers the next one to approve pls merge this so we can start feature freeze and testing for v10.0.0

@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: a4c9271

@georglauterbach
Copy link
Member

georglauterbach commented May 22, 2021 •
edited

LGTM 👍🏻

Anything else needed from your side @moqmar?

If not @docker-mailserver/maintainers the next one to approve pls merge this so we can start feature freeze and testing for v10.0.0

I have opened #1996 which does not change anything else than the layer count. After this PR is merged, we can begin with the feature freeze. We should merge #1996 before releasing v10 since its a safe improvement, see the PR description. #1996 should be the last PR to really be merged before the release of v10.

@georglauterbach georglauterbach mentioned this pull request May 22, 2021
Closed
11 tasks
@wernerfred
Copy link
Member

We currently have a failing CI on master. The merged PR check passed though. I retriggered the check, maybe wait with merge until it passes

@georglauterbach
Copy link
Member

We currently have a failing CI on master. The merged PR check passed though. I retriggered the check, maybe wait with merge until it passes

Just in time, I was about to merge this

@wernerfred
Copy link
Member

wernerfred commented May 22, 2021 •
edited

Tests passed. Now build and publish is in Progress. You can merge imho.
Seems Like the First Run had a Timing/CI Environment issue

@georglauterbach
Copy link
Member

Tests passed. Now build and publish is in Progress. You can merge imho.
Seems Like the First Run had a Timing/CI Environment issue

I will merge this when CI has fully passed :D

@georglauterbach georglauterbach merged commit 66bc157 into docker-mailserver:master May 22, 2021
@polarathene polarathene mentioned this pull request Oct 19, 2021
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polarathene polarathene polarathene left review comments

@wernerfred wernerfred wernerfred approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

@casperklein casperklein Awaiting requested review from casperklein

Assignees

@moqmar moqmar

Labels
area/documentation area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/high service/ldap
Projects
None yet
Milestone
v10.0.0
Development

Successfully merging this pull request may close these issues.

[BUG] SASLAUTHD_LDAP_SSL=1 does not set ldaps://
4 participants
@moqmar @georglauterbach @wernerfred @polarathene