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

Add an ability to match machine configuration to OpenShift container by annotations #7391

Merged
merged 3 commits into from
Nov 16, 2017

Conversation

akorneta
Copy link
Contributor

@akorneta akorneta commented Nov 15, 2017

What does this PR do?

Provides a way to implement converter from docker image recipes into OpenShift recipes.
Provides an ability to use simple (not composite) machine names in OpenShift recipes.
Fix provisioning of Che debug property in deploy_che.sh.
Adds logs filtering for foreign containers while workspace start.

What issues does this PR fix or reference?

#7308
#7367

Release Notes

n/a

@akorneta akorneta added kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. target/che6 labels Nov 15, 2017
@akorneta akorneta self-assigned this Nov 15, 2017
@akorneta akorneta requested a review from benoitf as a code owner November 15, 2017 13:24
*/
public static String podName(String machineName) {
return machineName.split("/")[0];
public static String machineName(Pod pod, Container container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You dont use anything from container except the name. Maybe leave param containerName?

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave Container as it gives us an opportunity to use something else from container later.


@BeforeMethod
public void setup() throws Exception {
MockitoAnnotations.initMocks(this);
internalRuntime =
new OpenShiftInternalRuntime(
internalRuntimeSpy =
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that you need spy here?

* Returns pod name that is extracted from machine name.
*
* @see #machineName(String, String)
* Returns machine name that has the following format `{POD_NAME}/{CONTAINER_NAME}` or machine
Copy link
Member

Choose a reason for hiding this comment

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

Please rewrite java doc here to make clear that there are three possible ways in which machine name can be constructed.
I mean

  1. MACHINE_NAME_ANNOTATION_FMT
  2. CHE_ORIGINAL_NAME_LABEL
  3. Names of objects

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Looks good to me. A special thank for repairing che debug mode! 👍

@akorneta akorneta merged commit 2b56807 into che6 Nov 16, 2017
@akorneta akorneta deleted the CHE-7308 branch November 16, 2017 16:10
@akorneta akorneta removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 16, 2017
@benoitf benoitf added this to the 6.0.0-M2 milestone Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants