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

[WIP] Hackathon merge #3013

Merged
merged 30 commits into from
Nov 11, 2016
Merged

[WIP] Hackathon merge #3013

merged 30 commits into from
Nov 11, 2016

Conversation

garagatyi
Copy link

What does this PR do?

  • Refactors workspace bootstrapping code:
    • Addition of projects volumes to ws-machines and applying of agents moved to infrastructure provisioner. It can be overriden in any implementation of Che and will be responsible for providing all infrastructure needed for Che implementatipn functioning.
    • Allow to configure environment network driver.
    • Allow to add Agents from the code instead of special files. These agents can override agents configured in files if identifiers are equal.
    • Adds possibility to host agents binaries from CHE master
    • Reworks agents in a way when they can be downloaded from CHE master if they are not found on FS.
    • Adds workspace ID to internal representation of environment to allow to use it for internal purposes of machine bootstrapping.
  • Renames some properties.
  • Refactors AgentConfigApplier to incapsulate additional code that was used with this class before.

benoitf and others added 16 commits October 16, 2016 10:20
Change-Id: I8383847bbba3b8932e4a10492642c67d37d4efb8
Signed-off-by: Florent BENOIT <[email protected]>
Change-Id: Ie21c57bf60bd482b2752e00bc1a8b08db5e71afd
Signed-off-by: Florent BENOIT <[email protected]>
…r "mounted" path

Change-Id: I5004b5344a708dc6985e76ded80edd033a747fc6
Signed-off-by: Florent BENOIT <[email protected]>
Change-Id: I4f1c3474387fe90f07a30a1af036773883e6c67e
Signed-off-by: Florent BENOIT <[email protected]>
Signed-off-by: Oleksii Kurinnyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
@garagatyi garagatyi changed the title Hackathon merge [WIP] Hackathon merge Nov 8, 2016
import static java.lang.String.format;

/**
* @author Alexander Garagatyi
Copy link
Contributor

Choose a reason for hiding this comment

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

There is nothing about what this class does

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'll fix that. It is because this PR is still in WIP

@@ -44,10 +44,10 @@ public AgentImpl(String id,
this.name = name;
this.version = version;
this.description = description;
this.dependencies = dependencies;
this.properties = properties;
this.dependencies = dependencies != null ? dependencies : new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

getters are doing this check so it looks odd to have it at both places
if it's immutable it should be Collection.emptyList()

Copy link
Contributor

Choose a reason for hiding this comment

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

firstNotNull?

Copy link
Author

Choose a reason for hiding this comment

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

Getters have a bug, I'll rewrite them. The bug is that they return mutable collection that doesn't influence their state.

@codenvy-ci
Copy link

Build # 951 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/951/ to view the results.

@garagatyi garagatyi self-assigned this Nov 8, 2016
@benoitf benoitf added the status/in-progress This issue has been taken by an engineer and is under active development. label Nov 8, 2016
@garagatyi
Copy link
Author

@skabashnyuk @evoevodin @mshaposhnik @sleshchenko Please review

// check and replace only occurrence of ":" after disk label on Windows host (e.g. C:/)
// but keep other occurrences it can be marker for docker mount volumes
// (e.g. /path/dir/from/host:/name/of/dir/in/container )
esc = path.replaceFirst(":", "").replace('\\', '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't like File.separator :) ?

Copy link
Author

@garagatyi garagatyi Nov 8, 2016

Choose a reason for hiding this comment

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

It's not my code. And I don't want to introduce here additional changes in this PR because it will delay merge significantly.

CHE-2454: add replacing of double underscores

Add replacing of double underscores in environment variable name
with single underscore in che application variable name.
Signed-off-by: Alexander Garagatyi <[email protected]>
@@ -139,6 +139,10 @@
<Host name="localhost" appBase="webapps"
unpackWARs="true" autoDeploy="false">


<!-- Provide Che Agent binaries (like workspace agent, terminal) -->
<Context docBase="${che.home}/lib" path="/agent-binaries" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to use separate folder for that. Let's call it /agent-binaries

Copy link
Contributor

Choose a reason for hiding this comment

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

separate folder by duplicating the existing binaries ? because agents are inside CHE_HOME/lib for now

Copy link
Author

Choose a reason for hiding this comment

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

I suppose that we can move agents binaries to any folder, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to move agents away from /lib

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok but it might take some time to validate this change no ?

Copy link
Author

Choose a reason for hiding this comment

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

@skabashnyuk are you ok if we postpone that and do after merging that PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

package org.eclipse.che.commons.lang.os;

/**
* Escapes Windows path to unix-style path.
Copy link
Contributor

Choose a reason for hiding this comment

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

what for?

Copy link
Author

Choose a reason for hiding this comment

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

It is used in several places in around docker usage in both Che and Codenvy. I think it is time to move that code in a single place since it is bad practice to use the same private methods in several classes.

@@ -44,10 +44,10 @@ public AgentImpl(String id,
this.name = name;
this.version = version;
this.description = description;
this.dependencies = dependencies;
this.properties = properties;
this.dependencies = dependencies != null ? dependencies : new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

firstNotNull?

@codenvy-ci
Copy link

@codenvy-ci
Copy link

Build # 962 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/962/ to view the results.

Alexander Garagatyi and others added 2 commits November 9, 2016 13:01
Signed-off-by: Alexander Garagatyi <[email protected]>
@codenvy-ci
Copy link

@codenvy-ci
Copy link

Build # 976 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/976/ to view the results.

Alexander Garagatyi added 3 commits November 10, 2016 09:49
fxi
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
Signed-off-by: Alexander Garagatyi <[email protected]>
@codenvy-ci
Copy link

Signed-off-by: Alexander Garagatyi <[email protected]>
@codenvy-ci
Copy link

fix
Signed-off-by: Alexander Garagatyi <[email protected]>
@garagatyi garagatyi merged commit 0c81935 into master Nov 11, 2016
@garagatyi garagatyi deleted the hackathon-merge branch November 11, 2016 16:21
@codenvy-ci
Copy link

Build # 997 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/997/ to view the results.

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
Refactors workspace bootstrapping code:
Addition of projects volumes to ws-machines and 
applying of agents moved to infrastructure provisioner. 
It can be overwritten in any implementation of Che and 
will be responsible for providing all infrastructure needed for Che implementation functioning.
Allow to configure environment network driver.
Allow to add Agents from the code instead of special files. 
These agents can override agents configured in files if identifiers are equal.
Adds possibility to host agents binaries from CHE master.
Reworks agents in a way when they can be downloaded from CHE master if they are not found on FS.
Adds workspace ID to internal representation of environment to allow to use it for internal purposes of machine bootstrapping.
Renames some properties.
Refactors AgentConfigApplier to encapsulate additional code that was used with this class before.
Signed-off-by: Alexander Garagatyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress This issue has been taken by an engineer and is under active development.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants