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

CHE-253: Refactor Che docker client #952

Merged
merged 1 commit into from
May 4, 2016
Merged

CHE-253: Refactor Che docker client #952

merged 1 commit into from
May 4, 2016

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Apr 1, 2016

_2 Upvotes_ Refactor Che docker client to be able to add new parameters without breaking code dependent on docker client

private String repository;
/** authentication configuration for private registries. Can be null */
private AuthConfigs authConfigs;
/** */
Copy link
Member

Choose a reason for hiding this comment

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

take a look

*
* @author Mykola Morhun
*/
public class GetSystemInfoParams {
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 this new unused empty class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@mmorhun mmorhun force-pushed the CHE-253 branch 3 times, most recently from cddca49 to 1a69c1f Compare April 4, 2016 12:44
@mmorhun
Copy link
Contributor Author

mmorhun commented Apr 4, 2016

@garagatyi check please

@@ -106,12 +107,12 @@ public void setNetworkSettings(NetworkSettings networkSettings) {
this.networkSettings = networkSettings;
}

public String getResolvConfPath() {
return resolvConfPath;
public String getResolveConfPath() {

Choose a reason for hiding this comment

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

Why you've changed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed typo

Choose a reason for hiding this comment

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

It is not a typo. We used the same wording Docker use itself. You can check it by inspecting docker container.

@mmorhun
Copy link
Contributor Author

mmorhun commented Apr 4, 2016

@garagatyi I corrected the code according to your comments


/**
* @param container
* id of container

Choose a reason for hiding this comment

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

are you sure that only id is valid value?

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Apr 20, 2016

+1 for

params.withTag(tag)
params.getTag() 

@garagatyi
Copy link

+0

@AndrienkoAleksandr
Copy link
Contributor

+1 for
params.withTag(tag)
params.getTag()

@garagatyi garagatyi mentioned this pull request Apr 21, 2016
@mmorhun
Copy link
Contributor Author

mmorhun commented Apr 21, 2016

@evoevodin @skabashnyuk check please

final DockerResponse response = connection.request();
final int status = response.getStatus();
if (!(NO_CONTENT.getStatusCode() == status || NOT_MODIFIED.getStatusCode() == status)) {
if (NO_CONTENT.getStatusCode() != status && NOT_MODIFIED.getStatusCode() != status) {
Copy link

@garagatyi garagatyi Apr 23, 2016

Choose a reason for hiding this comment

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

I think that status / 100 != 2 will be better in that case

@mmorhun
Copy link
Contributor Author

mmorhun commented Apr 26, 2016

@garagatyi ?

@garagatyi
Copy link

Code looks really good!
But we still need tests.

@mmorhun mmorhun force-pushed the CHE-253 branch 7 times, most recently from dca4980 to 1a8971f Compare April 29, 2016 14:14
@garagatyi
Copy link

LGTM
👍

…without breaking code dependent on docker client

Signed-off-by: Mykola Morhun <mmorhun@codenvy.com>
@mmorhun mmorhun merged commit 00b26db into master May 4, 2016
@mmorhun mmorhun deleted the CHE-253 branch May 4, 2016 06:34
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

6 participants