Skip to content

Conversation

@le-lenn
Copy link
Contributor

@le-lenn le-lenn commented Dec 21, 2025

The test sandbox was mounting ${HOME}/.docker/config.json into the manager/worker containers. That made the tests depend on my local Docker config, which was not compatible with the test suite.

This change sets DOCKER_CONFIG=/tmp/docker-config and mounts an empty named volume, keeping the test environment hermetic and consistent across machines.

@m90
Copy link
Member

m90 commented Dec 22, 2025

The idea behind the mount is to achieve the opposite of an isolated config: in CI the host's credentials need to propagate into the container (which is running Docker in Docker) so that pulls are authenticated and builds don't get rate limited (which happens when Dependabot opens multiple PRs at once). Merging this PR would break that behavior.

If you wanted to make this more versatile, it would still need to support the option of mounting the config file as it's done right now.

@le-lenn le-lenn marked this pull request as draft December 23, 2025 13:25
@le-lenn
Copy link
Contributor Author

le-lenn commented Dec 23, 2025

This PR now adds the option to overwrite the docker config. I am not 100% happy with the approach, but I think its a good start for a discussion

@m90
Copy link
Member

m90 commented Dec 26, 2025

I was wondering if it's simpler to flip the logic, i.e. default to not mounting the host config and only do that when requested (it's mostly needed for CI in any case). There's no hard requirement to keep the existing behavior, it's fine to add configuration to CI get what is needed.

In that case, one could introduce a DOCKER_CONFIG_FILE var which defaults to an empty JSON file next to the compose file, and CI will instead set it to the desired location?

@le-lenn
Copy link
Contributor Author

le-lenn commented Dec 26, 2025

That sounds like a great approach and would make the project easier for new developers to navigate and contribute to.

Do you want to take over or should I implement this?

@m90
Copy link
Member

m90 commented Dec 26, 2025

Do you want to take over or should I implement this?

Whatever you prefer. I won't get around to doing it before next year though, but I guess that's fine too.

@le-lenn le-lenn force-pushed the improve-test-suite-2 branch from 0d5fedb to 38d0749 Compare December 27, 2025 10:53
@le-lenn le-lenn force-pushed the improve-test-suite-2 branch from 38d0749 to 14b0e1e Compare December 27, 2025 10:58
@le-lenn le-lenn marked this pull request as ready for review December 27, 2025 10:59
@m90 m90 force-pushed the improve-test-suite-2 branch from 63996ec to eecaae1 Compare December 28, 2025 16:39
@m90
Copy link
Member

m90 commented Dec 28, 2025

I added another commit that applies the correct value in CI still, which seems to work. Thanks again.

@m90 m90 merged commit 80c2c13 into offen:main Dec 28, 2025
3 checks passed
@le-lenn
Copy link
Contributor Author

le-lenn commented Dec 28, 2025

Number 700 🥳

@le-lenn le-lenn deleted the improve-test-suite-2 branch December 28, 2025 19:24
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