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

Refactor inspectContainer() and add commit(), removeImage() #4085

Merged
merged 2 commits into from Feb 11, 2017
Merged

Refactor inspectContainer() and add commit(), removeImage() #4085

merged 2 commits into from Feb 11, 2017

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Feb 9, 2017

Adds commit() and removeImage() implementations to OpenShiftConnector. This requires some refactoring of existing methods (mostly refactoring repeated processes into their own methods -- e.g. creating an ImageStreamTag and getting image info from a tag).

Additionally, refactors inspectContainer() method to remove a call to DockerConnector, instead obtaining the same information from what's available through the OpenShift API. This fixes an issue where the IP address of a workspace was unavailable from the ContainerInfo returned by DockerConnector.

The benefit of these changes is that -- except for exec-related methods -- most uses of DockerConnector are removed when running on OpenShift. Note however that custom stacks are still not supported.

What does this PR do?

Refactors inspectContainer() to use ImageStreamTags instead of Docker, and adds commit() and removeImage() methods.

Changelog

Improved OpenShift connector independence from DockerConnector

Release Notes

(Internal changes -- not ready for public release)

Docs PR

Issue tracking work on OpenShift integration docs: https://github.com/eclipse/che-docs/issues/76.

@amisevsk
Copy link
Contributor Author

/cc @ibuziuk @l0rd Please review when you have time.

@codenvy-ci
Copy link

Can one of the admins verify this patch?

@l0rd l0rd self-requested a review February 10, 2017 11:22
l0rd
l0rd reviewed Feb 10, 2017
View reviewed changes
...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java Outdated
return null;
}

String podPullSpec = pod.getSpec().getContainers().get(0).getImage();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be more defensive here. I would check if the num of containers returned by getContainers() is > 0 and if the value returned by getImage() is nullOrEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java Outdated
String sourceImage = params.getImage(); // e.g. eclipse/ubuntu_jdk8
String targetImage = params.getRepository(); // e.g. eclipse-che/<identifier>
String tag = params.getTag();
if (tag == null || tag.isEmpty()) {
if (sourceImage.contains(":")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You split images string into tag and image name multiple times in the code. To avoid duplication you can probably extract that logic in a new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, there's a few methods in KubernetesStringUtils that can do most of the work now.

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java Outdated

@Override
public ImageInfo inspectImage(InspectImageParams params) throws IOException {

String image = params.getImage();
if (image.contains(":")) { // Image to inspect passed in with tag, which we don't support
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicated in removeImage too. You may want to extract it to a new method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, this was an oversight by me -- the method already existed.

@l0rd
Copy link
Contributor

l0rd commented Feb 10, 2017

@amisevsk you should rebase your PR

@l0rd
Copy link
Contributor

l0rd commented Feb 10, 2017

@amisevsk that's not related to this PR but you could have a look at that: in method getImageStreamTagFromRepo() the size to truncate imageStreamTagName is 30 but shouldn't it be 63 (aka MAX_CHARS)?

@amisevsk
Copy link
Contributor Author

I've rebased the PR and added some test cases.

@l0rd For the getImageStreamTagFromRepo() issue, the tag name has to be truncated since, as far as I can tell, OpenShift limits the length of the ImageStream name + tag to 63, not the tag by itself. This means that if the original repo name is long, the entire length of the ImageStream tag won't fit in that field.

Another option I considered (instead of truncating) was to do a startsWith() comparison, but I feel like this might be less readable, since we'd have to manipulate the ImageStreamTag name to get just the tag.

@amisevsk
Refactor inspectContainer and add commit(), removeImage()
4bb4269 
Adds commit() and removeImage() implementations to
OpenShiftConnector. This requires some refactoring of existing
methods (mostly refactoring repeated processes into their own
methods -- e.g. creating an ImageStreamTag and getting image
info from a tag).

Additionally, refactors inspectContainer() method to remove
a call to DockerConnector, instead obtaining the same information
from what's available through the OpenShift API. This fixes an
issue where the IP address of a workspace was unavailable from
the ContainerInfo returned by DockerConnector.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Add test cases for KubernetesStringUtils
29f4f30 
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Feb 11, 2017
@l0rd l0rd added this to the 5.3.0 milestone Feb 11, 2017
@l0rd l0rd merged commit ad318b0 into eclipse-che:master Feb 11, 2017
@JamesDrummond JamesDrummond mentioned this pull request Feb 17, 2017
Closed
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
@amisevsk @l0rd
Refactor inspectContainer() and add commit(), removeImage() (eclipse-…
c0560e8 
…che#4085)

Adds commit() and removeImage() implementations to
OpenShiftConnector. This requires some refactoring of existing
methods (mostly refactoring repeated processes into their own
methods -- e.g. creating an ImageStreamTag and getting image
info from a tag).

Additionally, refactors inspectContainer() method to remove
a call to DockerConnector, instead obtaining the same information
from what's available through the OpenShift API. This fixes an
issue where the IP address of a workspace was unavailable from
the ContainerInfo returned by DockerConnector.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l0rd l0rd l0rd approved these changes

Assignees
No one assigned
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@amisevsk @codenvy-ci @l0rd