O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) #296

kb019 · 2024-09-28T03:57:37Z

Description of what I changed

When setting up openmrs sdk by running mvn org.openmrs.maven.plugins:openmrs-sdk-maven-plugin:5.14.0-SNAPSHOT:setup i get a bunch of errors.

The first one is protocol unsupported error → This is because we use http instead of tcp to talk to docker daemon

Once the protocol error is tackeled we get the mysql image not found error ->The withImageFilterCommand doesn't seem to work. Reference With Docker 20.10.0 listImages filter no longer works · Issue #1516 · docker-java/docker-java

Finally we get the db name is incorrect error → I think we are again performing a slice on a already sliced db name

Openmrs Talk Link - OpenMRS SDK installation_BUILD FAILURE_Failed to setup server

Issue I worked on

https://openmrs.atlassian.net/browse/SDK-339

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean install right before creating this pull request and
added all formatting changes to my commit.

No? -> execute the above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

Video.1.mp4

Video.2.1.mp4

co-credit - @chibongho

chibongho · 2024-09-28T12:16:08Z

Thanks @kb019 . This code base is out of my depth. I'm adding @wikumChamith for this PR; feel free to add others along.

The changes are simple enough, but we want to make sure it doesn't break other systems.

@kb019 can you list out which platform (Windows 11?) / java version / mysql version (mysql v8.4.1?) you tested this change on?

kb019 · 2024-09-28T12:29:04Z

platform

I Tested this on windows 11, using jdk 1.8. The mysql version is automatically setup by the jdk,I think its 8.4.1. Thanks

wikumChamith

@kb019, thank you for the PR! Could you please clarify why the changes related to the database name are necessary? I tested it on Windows and didn't encounter any issues with the database name.

cc: @dkayiwa, @ibacher

kb019 · 2024-09-30T03:38:59Z

Hi @wikumChamith . Thanks. I made the change because i was getting the below error when installing sdk. I think what your thinking is right. I will recheck on this change and get back to you. Thanks

wikumChamith · 2024-09-30T03:45:16Z

Hi @wikumChamith . Thanks. I made the change because i was getting the below error when installing sdk. I think what your thinking is right. I will recheck on this change and get back to you. Thanks

What is the server name you are using?

kb019 · 2024-09-30T03:58:39Z

Hi @wikumChamith . Thanks. I made the change because i was getting the below error when installing sdk. I think what your thinking is right. I will recheck on this change and get back to you. Thanks

What is the server name you are using?

Oh k got it. I was giving it as 1.Its mistake on my side.I will update the commit. Thanks.
Also should i add some condition there while giving server name only if numeric input is present.

wikumChamith

Could you please update the PR and commit titles to reflect the ticket name?

https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips

wikumChamith · 2024-09-30T04:51:58Z

docker-maven-plugin/src/main/java/org/openmrs/maven/plugins/CreateMySql.java

@@ -62,7 +64,9 @@ public void executeTask() throws MojoExecutionException {
    }

    private boolean noMySqlImage(DockerClient docker) {
-        List<Image> mysql = docker.listImagesCmd().withImageNameFilter(MYSQL_8_4_1).exec();
+        ListImagesCmd listImagesCmd = docker.listImagesCmd();


Do we need these changes?

Yes @wikumChamith . The withImageNameFilter doesnt seem to work.It returns all the images instead of only mysql images. The reference is here .Thanks

wikumChamith · 2024-09-30T04:52:10Z

maven-plugin/src/main/java/org/openmrs/maven/plugins/Setup.java

@@ -627,7 +627,7 @@ public String determineDbName(String uri, String serverId) throws MojoExecutionE
 				}

 				dbName = dbName.substring(1);
-
+				


Remove this extra space

ibacher · 2024-09-30T14:17:03Z

sdk-commons/src/main/java/org/openmrs/maven/plugins/utility/DockerHelper.java

@@ -32,7 +32,7 @@ public class DockerHelper {
    public static final String DOCKER_MYSQL_PASSWORD = "Admin123";
    private static final String DOCKER_HOST_KEY = "dockerHost";
    public static final String DEFAULT_DOCKER_HOST_UNIX_SOCKET = "unix:///var/run/docker.sock";
-    public static final String DEFAULT_HOST_DOCKER_FOR_WINDOWS = "http://127.0.0.1:2375/";
+    public static final String DEFAULT_HOST_DOCKER_FOR_WINDOWS = "tcp://127.0.0.1:2375/";


We need to be careful with this. It isn't universally true (i.e., the default value here is accurate for some Docker setups, depending on the version of Docker installed). Better would probably be to switch the Docker library to auto-detecting as much as possible.

Ok @ibacher .I will look into that. Thanks

chibongho requested a review from wikumChamith September 28, 2024 12:13

wikumChamith reviewed Sep 29, 2024

View reviewed changes

kb019 force-pushed the fix/docker-fix branch 3 times, most recently from 560f445 to aa553c5 Compare September 30, 2024 04:04

wikumChamith requested changes Sep 30, 2024

View reviewed changes

ibacher reviewed Sep 30, 2024

View reviewed changes

kb019 force-pushed the fix/docker-fix branch from aa553c5 to 4538e2b Compare September 30, 2024 15:59

kb019 closed this Sep 30, 2024

kb019 force-pushed the fix/docker-fix branch from 4538e2b to 7e10aaf Compare September 30, 2024 16:10

kb019 changed the title ~~O3-339: Fix auto Docker setup in Sdk~~ O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) #296

O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) #296

kb019 commented Sep 28, 2024 •

edited

Loading

chibongho commented Sep 28, 2024

kb019 commented Sep 28, 2024

wikumChamith left a comment

kb019 commented Sep 30, 2024

wikumChamith commented Sep 30, 2024

kb019 commented Sep 30, 2024 •

edited

Loading

wikumChamith left a comment

wikumChamith Sep 30, 2024

kb019 Sep 30, 2024 •

edited

Loading

wikumChamith Sep 30, 2024

ibacher Sep 30, 2024

kb019 Sep 30, 2024

		@@ -627,7 +627,7 @@ public String determineDbName(String uri, String serverId) throws MojoExecutionE
		}

		dbName = dbName.substring(1);

O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) #296

O3-339: Fix auto Docker setup in Sdk (Sorry Currently lost the commit in reabse. Trying to undo the commit) #296

Conversation

kb019 commented Sep 28, 2024 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

chibongho commented Sep 28, 2024

kb019 commented Sep 28, 2024

wikumChamith left a comment

Choose a reason for hiding this comment

kb019 commented Sep 30, 2024

wikumChamith commented Sep 30, 2024

kb019 commented Sep 30, 2024 • edited Loading

wikumChamith left a comment

Choose a reason for hiding this comment

wikumChamith Sep 30, 2024

Choose a reason for hiding this comment

kb019 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

wikumChamith Sep 30, 2024

Choose a reason for hiding this comment

ibacher Sep 30, 2024

Choose a reason for hiding this comment

kb019 Sep 30, 2024

Choose a reason for hiding this comment

kb019 commented Sep 28, 2024 •

edited

Loading

kb019 commented Sep 30, 2024 •

edited

Loading

kb019 Sep 30, 2024 •

edited

Loading