Thank you for starting this PR! Adding LDAP integration tests to the already complex CI set-up of Symfony is a really brave task :)

Let's start with a big disclaimer: I've never used LDAP before, so my knowledge about it is very weak.

Can you maybe share why a custom docker image was used (instead of semi-official ones like bitnami/openldap)?
I think it would be good to explore how we can remove this "dependency" on a custom image, in order to reduce maintenance work (and depending on images maintained by a single author is a bit more "risky" than depending on an image maintained by a community or company).

Well, honestly I'm not into LDAP too. I've just used it for authentication. But I've made a PR which led to a bug that would have been avoided if I had been more accurate with tests. So... this is my way to ask forgiveness..

You're absolutely right: introducing a dependency on a custom image would absolutely be risky. I needed to do it because I needed to push some initial data to the LDAP server, namely, those in the test env. And it is precisely what I do in my custom Dockerfile: I create a folder where I store the data and copy it there. Then, with the env, I tell the image to load it.

I think, the best aproach would be to use the standard image and then, add a step in the action, that pushes the data to the server. In order to do that I should use some command line program like ldapadd. And thats where I'm stuck...

Another way would be to create a custom action, which would lead to tha same problem as having a custom docker image.

If we really need a custom image (which I'd like to avoid), we should move its maintenance this this repository or at least a repository within the Symfony organization.

If we really need a custom image (which I'd like to avoid), we should move its maintenance this this repository or at least a repository within the Symfony organization.

we even don't need a docker image. Github Action can run Dockerfile directly (https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/creating-a-docker-container-action)

We can do this without a custom image I think. The following docker compose config (docker compose isn't needed here, it's just more readable than a docker run command imho) sets up the openldap image with our fixture data (as far as I can see):

version: '3.0'

services:
    ldap:
        image: osixia/openldap:1.4.0
        ports:
            - 3389:389
        volumes:
            - ./src/Symfony/Component/Ldap/Tests/Fixtures/data/fixtures.ldif:/container/service/slapd/assets/config/bootstrap/ldif/50-bootstrap.ldif
        command: --copy-service --loglevel debug
        environment:
            LDAP_DOMAIN: symfony.com
            LDAP_BASE_DN: dc=symfony,dc=com
            LDAP_ADMIN_PASSWORD: 'symfony'

Please note that I can't make the tests pass with this config, so something isn't quite right. But it at least is a start to get custom data in the openldap image without using a custom image.

@wouterj I've tryed your suggestion. seemd promising! I've mounted a volume with the GHA:

openldap:
    image: osixia/openldap:1.4.0
    options: -v ${{ github.workspace }}/src/Symfony/Component/Ldap/Tests/Fixtures/data/fixtures.ldif:/ldif-data
    ports:
        - 3389:389
    env:
        LDAP_DOMAIN: "symfony.com"
        LDAP_ADMIN_PASSWORD: symfony
        LDAP_BASE_DN: "dc=symfony,dc=com"
        LDAP_SEED_INTERNAL_LDIF_PATH: "/ldif-data"

But in the Checkout phase, GHA tryes to clean-up the workspace and, beeing the volume tied to the image, breaks the pipeline.

I'll try some alternatives and read the article pointed from @jderusse

we even don't need a docker image. Github Action can run Dockerfile directly (https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/creating-a-docker-container-action)

At a first impression, that would move the problem. In the sense that, instead of maintaing a docker image, there should be a GHA to maintain.

Hmm, according to actions/checkout#211 (comment) the solution would be to run this command before the checkout action:

sudo chown -R $USER:$USER /home/runner/work/symfony/symfony/src/Symfony/Component/Ldap/Tests/Fixtures/data/fixtures.ldif

we even don't need a docker image. Github Action can run Dockerfile directly (https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/creating-a-docker-container-action)

My bad @jderusse ! Indeed, we can use a local action and not depenend on an external one.

I squashed your commits @lucasaba rebased, and puh a commit that fix the CI.
Please, pull --rebase before pushing another commit

PR is ready for review @derrabus @wouterj

Rebase went wrong? It seems that an unrelated commit landed in this PR 3510b67

Thanks @jderusse ! I didn't think about using actions in that way! Thank you very much!
The erros on Travis and appveyour seems unrelated.

@chalasr seems ok now. I had some error too. Maybe some cache on github....

@chalasr is right, that does not look right. The content of that commit looks okay, but message and author are from a different PR.

Rebase went wrong? It seems that an unrelated commit landed in this PR 3510b67

Yeah, the commit was there in the original PR (lucasaba@9de6771) I can reword it or squash it if you want @derrabus

Right now, the commit message is misleading and I'm the author of that commit. I think, squashing it into the second commit and removing me as author would be good. 😃

derstand anything about what's happening here, but it seems to make the LDAP

For the record. This PR update the CI to:

• starts an openldap server as a service (like redis/memcache/...)
• load fixtures
• deletes the user created by the bitnami/openldap (I didn't found a way to start with basic root setup, BUT WITHOUT demo users generated by LDAP_USERS env variable)
• remove LDAP calls from travis

Then it fixes tests:

• restore states of database after successful/failed tests (otherwise, you can't run tests twice)
• fix SSL bug in connection to LDAP
• fix wrong exception expected

unrelated, but I'm a bit scared by amount of integration tests that are skipped on our CI

So do I.. Shall we run tests with option -vv to see the reason of skipped tests?

So do I.. Shall we run tests with option -vv to see the reason of skipped tests?

Yes, that would be good. Integration tests are often skipped if an external system is not available. If we have a CI run dedicated to integration tests, skipped tests are likely an accident.

So do I.. Shall we run tests with option -vv to see the reason of skipped tests?

Yes, that would be good. Integration tests are often skipped if an external system is not available. If we have a CI run dedicated to integration tests, skipped tests are likely an accident.

Done. We have ~200 skipped tests for the following reasons:

• Testing expiration slows down the test suite
• Not a pruneable cache pool.
• Memcached cannot clear by prefix
• REDIS_SENTINEL_HOSTS env var is not defined.
• Memcached expects a TTL greater than 1 sec. Simulating a slow network is too hard

I'm checking for the REDIS_SENTINEL test.
Other skipped tests sounds legit to me.

All right, none of them is related to LDAP, so this PR should be good.

(fwiw, let's move the conversation of the skipped tests to another PR/issue, so we can merge this asap)

Thanks @lucasaba.

Thank you all guys. As usual, has been a pleasure and a privilege. Learned a lot!

I'll merge this up to 5.1 now. Let's hope, the tests stay green. 😃

🤞🏻 https://github.com/symfony/symfony/runs/1407291010

There was 1 error:

1) Symfony\Component\Ldap\Tests\Adapter\ExtLdap\LdapManagerTest::testUpdateOperationsThrowsExceptionWhenAddedDuplicatedValue
Undefined variable: con

/home/runner/work/symfony/symfony/src/Symfony/Component/Ldap/Adapter/ExtLdap/EntryManager.php:155
/home/runner/work/symfony/symfony/src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php:369

Anyone up for a quick fix before I move on to 5.2? 😁

$con should be $this->getConnectionResource(). Or we should do $con = $this->getConnectionResource(); (just like in other methods of this class)

That makes sense. I've extracted that variable on 4.4 already, in order to keep the diff small. #39094

The LDAP integration tests are now green on 5.1, 5.2 and 5.x. 🎉

thank you for your help @lucasaba .

It helped to detect issue in 5.1 if I understand well, and would have prevent bugs in previous PRs. 🎉

Ok, now I'm really flattered! You've made my day!!!!! ❤️