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.

Sign up for GitHub

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

Jump to bottom

Create DockerClient interface #3703

Merged
merged 10 commits into from
Jul 27, 2022
Merged

Create DockerClient interface #3703

merged 10 commits into from
Jul 27, 2022

Conversation

eddumelendez
Copy link
Contributor

@eddumelendez eddumelendez commented Jul 4, 2022 •
edited

Current DockerClient is moved to CliDockerClient. This new
interface allows to add new implementations via ServiceLoader.

This PR is addressing just the public DockerClient API from #3571

@chanseokoh
Copy link
Member

Team, just remember that public-facing stuff under .api is intended to be stable once live.

@elefeint
Copy link
Contributor

elefeint commented Jul 8, 2022

Thank you! We'll review next week.

elefeint
elefeint suggested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

I apologize for the delay. The PR looks very good overall! It was easy to review because it does one specific thing.

Could you take a look at the checkstyle failure -- AnotherDockerClient needs a license header.
I would not worry about the LongConsumer sonar code smells, since they are both in preexisting code. The private constructor one may be worth addressing.

Other than that, I only have one concern echoing what @chanseokoh said -- that DockerImageDetails might be too specific to be part of the public API.

jib-core/src/main/java/com/google/cloud/tools/jib/api/DockerClient.java Outdated Show resolved Hide resolved
elefeint
elefeint suggested changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Could you rebase from master branch? We've had some infrastructural changes in macos build.

@eddumelendez
Create DockerClient interface
2fece81 
Current `DockerClient` is moved to `CliDockerClient`. This new
interface allows to add new implementations via `ServiceLoader`.
@eddumelendez
Pass docker env
981e5fa
@eddumelendez
Add license header
36f7dfe
@eddumelendez
Add private constructor
4b18976
@eddumelendez
Copy link
Contributor Author

rebase it!

@sonarcloud
Copy link

sonarcloud bot commented Jul 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.2% 80.2% Coverage
0.0% 0.0% Duplication

elefeint
elefeint approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Thank you!

@elefeint elefeint merged commit 9027c8d into GoogleContainerTools:master Jul 27, 2022
@elefeint elefeint mentioned this pull request Aug 12, 2022
Closed
@mpeddada1
Copy link
Contributor

Thank you for your contribution! jib-core 0.22.0 has been released with this change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsideup bsideup bsideup approved these changes

@elefeint elefeint elefeint approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@eddumelendez @chanseokoh @elefeint @mpeddada1 @bsideup @kokoro-team