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

Check for the DOCKER_HOST variable instead of the Docker sock file. #225

Closed
wants to merge 1 commit into from

Conversation

KoxSosen
Copy link
Collaborator

In certain scenarios, when Docker is installed the sock may be present in a different location. For as an example, when installing docker in a rootless environment, the sock location will be different based on the installation type.

This change should also maintain compatibility with installations where the sock is in the default location, as by default Docker always sets this environment variable.

In certain scenarios, when Docker is installed the sock may be present in a different location.
@A248
Copy link
Owner

A248 commented Jul 31, 2023

Unfortunately, this seems to break the Github Actions CI which runs the build. You'll notice that it ran within five minutes, which is too quick, indicating the databases weren't started: https://github.com/A248/LibertyBans/actions/runs/5712928498/job/15477346391

Fortunately, with the new infrastructure, I think we can ditch Github Actions soon. I've been wanting to, because the Actions framework is specifically designed by Github to implement vendor lock-in. At the very least, building with Github Actions should cease to be necessary, and Jenkins can finally claim its rightful due.

Note that specifying multiple activation mechanisms is worth a try, but it is a try I carried out myself, a long time ago, when I setup this build. Adding multiple activation sections requires that all of them succeed. Maven disappoints me frequently in its lack of foresight, as evidenced in MNG-4565.

@KoxSosen
Copy link
Collaborator Author

Thank you for letting me know; To be honest, I didn't notice the GitHub CI didn't run with docker enabled, which indicates that his PR does break some setups in it's current form.

When I was working on the PR, I did run a test on Jenkins, to see if my changes were OK, and it picked up Docker without any issues. I'll create a test environment shortly, and test with rooted docker as well.

It's interesting that Maven doesn't provide better control over conditional logic here, as that would greatly simplify some steps.

I'll add a solution to this PR as soon as I find one.

@A248
Copy link
Owner

A248 commented Aug 1, 2023

Ultimately, whichever docker detection mechanism we employ will have flaws. The sock file in the current source exists only for Unix, and the environment variable clearly doesn't work in all environments either. Of course, profiles can always be activated and deactivated manually, with, for example, -Pdocker-enabled or -P-docker-disabled. We may want to remove the current socket detection for consistency, or keep it for convenience. Either way is fine by me, but do tell me which setting is more welcoming to new contributors.

@KoxSosen
Copy link
Collaborator Author

KoxSosen commented Aug 1, 2023

After doing some research, I totally see your point; It seems that there isn't a concrete solution for this issue at the moment. While it's probably possible to hack something together, I see why you choose not to do something like that. So, therefore your suggestion of manually enabling profiles seems like the correct approach. For new contributors, I think the current setup is fine, as it's more likely that they'll have the docker sock in the "default" location. Thank you for your input, and help regarding debunking this issue, I'll close this PR.

@KoxSosen KoxSosen closed this Aug 1, 2023
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