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.

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

Include ldap support in docker image #205

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Conversation

pixil98
Copy link
Contributor

@pixil98 pixil98 commented Jul 9, 2024

Fixes #204

Includes the postfix-ldap package in both debian and alpine docker images.

Aaron Reisman and others added 2 commits July 9, 2024 13:12
@pixil98 pixil98 marked this pull request as draft July 9, 2024 18:14
@bokysan
Copy link
Owner

bokysan commented Jul 10, 2024

Hi, looks OK. Tests are successful.

I would suggest:

  • updating documentation
  • writing some tests
  • moving from DRAFT to FINAL so I can merge.

Cheers,
B

@pixil98
Copy link
Contributor Author

pixil98 commented Jul 10, 2024

Will do, thanks. I've been sidetracked this week with some unrelated database issues that I need to track down. Once I get that figured out (hopefully tonight) I'll finish this up. In addition I want to fully test that ldap is working with only that package being installed before I undraft it. Last I left it, I was getting an error binding, but I'm pretty sure that's just my config.

@pixil98
Copy link
Contributor Author

pixil98 commented Jul 19, 2024

I should have some time to work on this over the weekend. What kind of tests are you looking for? It should be easy enough to test that when an ldap map is configured it doesn't give the error that ldap isn't supported. Doing anything beyond that would require setting up and configuring an ldap server for the test.

@pixil98
Copy link
Contributor Author

pixil98 commented Aug 5, 2024

@bokysan I added some integration tests, but I'm not sure that they're actually meaningful. When I run them locally, via integration-tests.sh, they all always seem to pass. Even when I try to break an existing integration test and run it, it still passes. Is there something in particular I need to do to run them locally?

I'd be happy to update documentation if you think there is something meaningful to add, but this is just enabling a standard type of map in postfix. There's no config needed to enable support for them and it would be better if users went to postfix's documentation on how to use ldap maps.

@pixil98 pixil98 marked this pull request as ready for review August 5, 2024 04:57
@pixil98
Copy link
Contributor Author

pixil98 commented Aug 5, 2024

You can semi ignore that previous comment, I rewrote the ldap tests to use postmap instead of trying to send an email and I verified that it actually fails if ldap isn't working. I still always get successes on the other tests, but that's less of a concern to me since I didn't touch their functionality.

@pixil98
Copy link
Contributor Author

pixil98 commented Aug 26, 2024

@bokysan can we get this merged in soon?

@bokysan bokysan merged commit 11cc00c into bokysan:master Aug 26, 2024
1 check passed
@bokysan
Copy link
Owner

bokysan commented Aug 26, 2024

Yeah, missed it. There we go.

I'll test it out a bit and make a release this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP lookup map support
2 participants