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

(#23) Implemented RemoteDocker #64

Merged
merged 4 commits into from Mar 26, 2018
Merged

(#23) Implemented RemoteDocker #64

merged 4 commits into from Mar 26, 2018

Conversation

llorllale
Copy link
Contributor

@llorllale llorllale commented Mar 23, 2018

This PR:

Note:
The implementation of SslHttpClient is very minimal. I think we can just implement its operations as their need arises. It's also the reason why I added very few tests for it. It's the major reason why coverage decreases with this PR.

* New 'RemoteDocker' implementation of Docker
* New 'DefaultHttpClient' implementing connection pooling
@0crat
Copy link
Collaborator

0crat commented Mar 23, 2018

Job #64 is now in scope, role is REV

@llorllale llorllale changed the title (#53) Implemented RemoteDocker (#23) Implemented RemoteDocker Mar 23, 2018
@coveralls
Copy link

coveralls commented Mar 23, 2018

Pull Request Test Coverage Report for Build 109

  • 8 of 25 (32.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-10.7%) to 85.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/com/amihaiemil/docker/RemoteDocker.java 4 6 66.67%
src/main/java/com/amihaiemil/docker/SslHttpClient.java 4 19 21.05%
Totals Coverage Status
Change from base Build 103: -10.7%
Covered Lines: 119
Relevant Lines: 140

💛 - Coveralls

Copy link
Owner

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

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

@llorllale looks nice; just one comment

* @since 0.0.1
* @checkstyle ParameterNumber (150 lines)
*/
final class DefaultHttpClient implements HttpClient {
Copy link
Owner

Choose a reason for hiding this comment

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

@llorllale rename this to SSLHttpClient (since it's going to use the certificates to encrypt the requests) and leave a puzzle for exactly that: registering the http/https protocol using the provided certificates -- it's going to be similar to how unix is registered in UnixHttpClient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per PR review:
* Renamed DefaultHttpClient -> SslHttpClient
@llorllale
Copy link
Contributor Author

@amihaiemil done, please review

Forgot to rename DefaultHttpClientTestCase -> SslHttpClientTestCase
@amihaiemil
Copy link
Owner

@rultor merge pls

@rultor
Copy link
Collaborator

rultor commented Mar 26, 2018

@rultor merge pls

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 3574c4c into amihaiemil:master Mar 26, 2018
@rultor
Copy link
Collaborator

rultor commented Mar 26, 2018

@rultor merge pls

@amihaiemil Done! FYI, the full log is here (took me 2min)

@0crat
Copy link
Collaborator

0crat commented Mar 26, 2018

The job #64 is now out of scope

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.

None yet

5 participants