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

Replacing deprecated docker-client with docker-java #922

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

samueleresca
Copy link
Member

@samueleresca samueleresca commented Jan 9, 2024

Changes

Replacing deprecated docker-client with docker-java. See: #806

Testing

  • AsyncDnsResolverIntegrationSpec and DnsDiscoverySpec tests are successful.

client.pull(image)
client
.pullImageCmd(image)
.start()
} catch {
case NonFatal(_) =>
log.warning(s"Failed to pull docker image [$image], is docker running?")
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to close the client in finally block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a finally block here would close the connection pool of the client before executing the subsequent instruction. I proceeded by adding the client.close() instruction in the afterTermination() override


abstract class DockerBindDnsService(config: Config) extends PekkoSpec(config) with Eventually {
val client = DefaultDockerClient.fromEnv().build()

val dockerConfig: DockerClientConfig = DefaultDockerClientConfig.createDefaultConfigBuilder().build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultDockerClient.fromEnv() -> DefaultDockerClientConfig.createDefaultConfigBuilder()
This change seems to bring some changes to the configuration method, it can't configration by env.

DockerClientConfig custom = DefaultDockerClientConfig.createDefaultConfigBuilder()
    .withDockerHost("tcp://docker.somewhere.tld:2376")
    .withDockerTlsVerify(true)
    .withDockerCertPath("/home/user/.docker")
    .withRegistryUsername(registryUser)
    .withRegistryPassword(registryPass)
    .withRegistryEmail(registryMail)
    .withRegistryUrl(registryUrl)
    .build();

for testContainer, we can configration by env
https://java.testcontainers.org/supported_docker_environment/
please consider the question: How can I customize the Docker configuration,
maybe docker-java supportes it allready.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laglangyue from my understanding the createDefaultConfigBuilder fetch the configuration from env, unless overwritten, see: https://github.com/docker-java/docker-java/blob/main/docs/getting_started.md#system-environment

Please let me know if I am missing something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine,it's ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is necessary to add a document or a link to tell users how to configure it.

@samueleresca
Copy link
Member Author

Both AsyncDnsResolverIntegrationSpec and DnsDiscoverySpec tests are passing locally. I'm investigating why they are failing in build

@samueleresca
Copy link
Member Author

Both AsyncDnsResolverIntegrationSpec and DnsDiscoverySpec tests are passing locally. I'm investigating why they are failing in build

Now the tests are passing - the code wasn't awaiting for the imagePullCmd. The reason why it was passing locally was that the image was already in my local repo

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm - I'm travelling but this looks good from a quick perusal.

@laglangyue
Copy link
Contributor

LGTM

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@samueleresca samueleresca merged commit b7be586 into apache:main Jan 12, 2024
18 checks passed
@samueleresca samueleresca deleted the replacing-docker-client branch January 12, 2024 16:12
pjfanning pushed a commit to pjfanning/incubator-pekko that referenced this pull request Apr 12, 2024
* Replacing deprecated docker-client with docker-java.

* Addressing feedback.

* Awaiting for image pull.

---------

Co-authored-by: Samuele Resca <sr7@ad.datcon.co.uk>
pjfanning pushed a commit that referenced this pull request Apr 12, 2024
* Replacing deprecated docker-client with docker-java.

* Addressing feedback.

* Awaiting for image pull.

---------

Co-authored-by: Samuele Resca <sr7@ad.datcon.co.uk>
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.

4 participants