Skip to content

Fix collection compatibility and preserve LDAP URI between roles - #76

Merged
lvps merged 28 commits into
lvps:masterfrom
Dataloh:stop-depending-on-filter-plugin
Aug 26, 2026
Merged

lvps merged 28 commits into
lvps:masterfrom
Dataloh:stop-depending-on-filter-plugin

Conversation

@Dataloh

@Dataloh Dataloh commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary
This pull request contains two fixes required after moving the roles into the lvps.ldap_389ds collection.

Changes

  • Replace the removed quote_plus filter plugin with Ansible’s built-in urlencode filter and encode path separators explicitly.
  • Preserve the configured dirsrv_server_uri when LDAPI is disabled.
  • Only switch to the generated LDAPI URI when LDAPI is both enabled and actively configured.

Motivation
The first change fixes a failure in the new lvps.ldap_389ds collection where the removed filter plugin could no longer be loaded.

The second change ensures that lvps.ldap_389ds.replication can run correctly after the server role in the same playbook. Previously, the server role could overwrite dirsrv_server_uri with the LDAPI URI even when LDAPI was disabled, preventing the replication role from connecting using the configured LDAP endpoint.

@lvps

lvps commented Aug 25, 2026

Copy link
Copy Markdown
Owner

This all makes sense and I'm willing to merge your changes, but I just want to be sure: have you tested them?

It seems a bit strange to do regex_replace("/", "%2F"), which is equivalent to the previous quote_plus but now I have doubts on that too, and adding | bool seems redundant, even if one of the molecule tests does that for some reason.

This also compounds with the fact that the molecule tests haven't been working for a while (#68): if you're willing to dedicate a few tokens (from your comments it seems pretty obvious that you're running everything through an LLM), I would really appreciate if also try to fix that and it would make it easier to test any further changes.

@Dataloh

Dataloh commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

It took a little while longer than I expected, but I managed to get the CI pipeline to work. I ended up removing centos:7 from the molecule tests, but I think I should try to readd and fix whatever issues there was with that version, since I guess that this is the test that covers 389ds-base version 1.3.

Here is my thoughts on some of the changes that were made and some of the issues I ran into.

  • Removed the systemctl python replacement, since systemctl works just fine in Docker with the right settings.
  • Added some variables to converge.yml that are defaults in the role, but cannot be inferred since the steps afterwards are not part of the role.
  • Addded user and group dirsrv no matter what. I ran into issues if I did not and I figured that it would not be a problem anyways to always add it.
  • Loading the role from {{ playbook_dir }}/../.. this way it is faster to test local changes without having to reinstall local changes.
  • Removed the redundant bool that you mentioned.

I look forward to hear your thoughts on the suggested changes. And feel free to question anything. There was a human in the loop, so I should be able to argue for most of the changes ;)

@lvps
lvps merged commit 9ba8567 into lvps:master Aug 26, 2026
1 check passed
@lvps

lvps commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Thanks! I added a test for the LDAPI part and it passes, too, so I merged your changes.

I also tried to run all tests on Rocky 9, which works perfectly on my machine with Python 3.10 and all the packages from requirements.txt, but fails for no discernible reason on the GitHub Actions runner - but I'll open an issue for that.

@lvps lvps mentioned this pull request Aug 26, 2026
2 of 4 tasks
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.

2 participants