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

fix: error already existing instance due to case sensitive Path #1847

Merged
merged 7 commits into from
Jan 10, 2025

Conversation

Amitrei
Copy link
Contributor

@Amitrei Amitrei commented Jan 1, 2025

Description

We are using a per-module builder implementation, where the module name is used as the builder name:
<builderName> ${project.artifactId} </builderName>
When specifying a buildx builder name with capital letters, for example:
<builderName> myBuilder </builderName>,
an error occurs if the builder already exists:
ERROR: existing instance for "myBuilder" but no append mode, specify --node to make changes for existing instances

Cause

The docker buildx create command stores the builder configuration at the following path:
$DOCKER_CONFIG/buildx/instances/mybuilder (Note: the builder name is stored in lowercase).

According to the Docker buildx implementation, docker buildx always stores the builder configuration file name in lowercase.

In the Maven Docker plugin, the following code checks if a builder exists by looking for the builder name inside the instances folder. If the builder doesn't exist, it attempts to create it:


...
Path builderPath = configPath.resolve(Paths.get("buildx", "instances", builderName));
if (Files.notExists(builderPath)) {
    List<String> cmds = new ArrayList<>(buildX);
    append(cmds, "create", "--driver", "docker-container", "--name", builderName);
}
...

While some file system types are case-insensitive, others are not. This results in the plugin mistakenly attempting to create a builder that already exists. On my macOS, the file system ignores case sensitivity, so the code works fine. However, on our Linux-based CI runners, the "already exists" error is triggered due to case sensitivity.

The Fix

Simplify the builder name lookup by converting the builderName to lowercase when checking the instances directory. This aligns with the behavior of the docker buildx create command, which resolves the issue of case sensitivity.

@Amitrei Amitrei changed the title fix: Fixed error already existing instance due to case sensitive Path… fix: error already existing instance due to case sensitive Path… Jan 1, 2025
@rohanKanojia
Copy link
Member

@Amitrei : Thanks for the PR! Is it possible to add a unit test to verify path is getting converted to lowercase as expected?

@Amitrei
Copy link
Contributor Author

Amitrei commented Jan 4, 2025

Hi, as requested I added a test making sure the builder name is lower cased when looking up for a builder config in the buildx/instances folder.

Edit: fixed sonarqube issue related to the test.

Removed unnecessary eq on verify method due to SonarQube issue
@Amitrei Amitrei changed the title fix: error already existing instance due to case sensitive Path… fix: error already existing instance due to case sensitive Path Jan 8, 2025
@rohanKanojia
Copy link
Member

@Amitrei : Thanks, your changes look good to me. could you please add a line regarding this change to doc/changelog.md ?

@Amitrei
Copy link
Contributor Author

Amitrei commented Jan 10, 2025

Hi Rohan,
Thanks for the approval. I've added a note regarding the change in the doc/changelog.md file. Please let me know if you need anything else from me.

Thanks again!

@rohanKanojia rohanKanojia merged commit 4df03f3 into fabric8io:master Jan 10, 2025
18 of 23 checks passed
@rohanKanojia
Copy link
Member

@Amitrei : Thanks a lot!

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