CHE-3129 - Project can't be created without Z option #3130

snjeza · 2016-11-19T23:41:45Z

What does this PR do?

It fixes #3129

What issues does this PR fix or reference?

#3129

Signed-off-by: Snjezana Peco [email protected]

codenvy-ci · 2016-11-19T23:44:01Z

Can one of the admins verify this patch?

benoitf · 2016-11-20T10:13:39Z

ci-build

codenvy-ci · 2016-11-20T10:58:44Z

Build # 1074 - FAILED

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

tolusha · 2016-11-21T06:06:08Z

@garagatyi
Could you have a look pls. A think it is matter of che.docker.volumes_projects_options configuration not code changing.

l0rd · 2016-11-21T08:31:10Z

@tolusha I think that #3129 needs code changing (in the default config the property is set).

@snjeza I think you need do the same check for property che.docker.volumes_agent_options and if you could flag the fields/parameters with @Nullable it would be nice.

garagatyi

@snjeza Nice catch!
@tolusha No, code have to be changed in any case.
@l0rd Agree with you, Mario!

@snjeza Please consider comments of Mario and mine. Please also rename volumeOptions argument of constructor to projectsVolumeOptions

garagatyi · 2016-11-21T08:45:25Z

...main/java/org/eclipse/che/plugin/docker/machine/local/LocalCheInfrastructureProvisioner.java

@@ -69,9 +71,15 @@ public void provision(Environment envConfig, CheServicesEnvironmentImpl internal
        // add bind-mount volume for projects in a workspace
        String projectFolderVolume;
        try {
-            projectFolderVolume = format("%s:%s:%s",
+            if (Strings.isNullOrEmpty(volumesOptions)) {


Let's form suffix in constructor. Then you won't need this clause in main code.

snjeza · 2016-11-21T11:48:14Z

I have updated the patch.

garagatyi · 2016-11-21T12:13:19Z

...main/java/org/eclipse/che/plugin/docker/machine/local/LocalCheInfrastructureProvisioner.java

@@ -35,19 +38,23 @@
    private final WorkspaceFolderPathProvider workspaceFolderPathProvider;
    private final WindowsPathEscaper          pathEscaper;
    private final String                      projectFolderPath;
-	private final String                      volumesOptions;
+	private final String                      projectsVolumeOptions;


Please replace tab character with spaces. Someone put it here but it is not allowed for indentation in Che.

garagatyi · 2016-11-21T12:19:00Z

...src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/TerminalVolumeProvider.java

@@ -64,7 +67,11 @@ public String get() {
    }

    private String getTargetOptions(final String path) {
-        return path + CONTAINER_TARGET + ":" + volumeOptions;
+        if (!Strings.isNullOrEmpty(agentVolumeOptions)) {


Please create constructor and put this code there as you did in LocalCheInfrastructureProvisioner

garagatyi · 2016-11-21T12:19:14Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/WsAgentVolumeProvider.java

@@ -68,6 +71,10 @@ public String get() {
    }

    private String getTargetOptions(final String path) {
-        return path + CONTAINER_TARGET + ":" + volumeOptions;
+        if (!Strings.isNullOrEmpty(agentVolumeOptions)) {


Please create constructor and put this code there as you did in LocalCheInfrastructureProvisioner

snjeza · 2016-11-21T13:12:49Z

The patch updated.

garagatyi

Sorry that still asking you to change your PR, but there are few minor changes to be made.

garagatyi · 2016-11-21T13:21:46Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

+    private String agentVolumeOptions;
+
+    public DockerExtConfBindingProvider() {
+        if (!Strings.isNullOrEmpty(agentVolumeOptions)) {


Constructor is called before field injection in Guice. So here we will have null value always. Can you replace field injection with a constructor injection.

garagatyi · 2016-11-21T13:21:56Z

...src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/TerminalVolumeProvider.java

+    private String agentVolumeOptions;
+
+    public TerminalVolumeProvider() {
+        if (!Strings.isNullOrEmpty(agentVolumeOptions)) {


Constructor is called before field injection in Guice. So here we will have null value always. Can you replace field injection with a constructor injection.

garagatyi · 2016-11-21T13:22:04Z

.../src/main/java/org/eclipse/che/plugin/docker/machine/ext/provider/WsAgentVolumeProvider.java

+    private String agentVolumeOptions;
+
+    public WsAgentVolumeProvider() {
+        if (!Strings.isNullOrEmpty(agentVolumeOptions)) {


Constructor is called before field injection in Guice. So here we will have null value always. Can you replace field injection with a constructor injection.

Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2016-11-21T16:07:39Z

The patch updated.
I wasn't able to inject the che.docker.volumes_agent_options option to the DockerExtConfBindingProvider constructor because DockerExtServerModule created it with the empty constructor.
Che doesn't start when I try to bind DockerExtConfBindingProvider to DockerExtServerModule.

garagatyi

Good job, Snjezana!

garagatyi · 2016-11-22T07:56:28Z

...in/java/org/eclipse/che/plugin/docker/machine/ext/provider/DockerExtConfBindingProvider.java

@@ -43,8 +46,8 @@
    private static final Logger LOG              = LoggerFactory.getLogger(DockerExtConfBindingProvider.class);

    @Inject
-    @Named("che.docker.volumes_agent_options")
-    private String volumeOptions;
+    @Nullable @Named("che.docker.volumes_agent_options")


OK, then leave it as it is now, I'll fix that in one of my next PRs.

sleshchenko · 2016-11-22T08:40:40Z

Looks like DockerExtConfBindingProvider shouldn't work correctly because instance is created with default constructor and requestInjection method is not invoked for it. Please make sure that it works correctly

snjeza · 2016-11-22T13:24:22Z

Looks like DockerExtConfBindingProvider shouldn't work correctly because instance is created
with default constructor and requestInjection method is not invoked for it. Please make sure
that it works correctly

That's why I haven't injected the che.docker.volumes_agent_options option to the DockerExtConfBindingProvider constructor.
I have tested the build with and without the Z option. It works properly.

garagatyi · 2016-11-22T13:27:31Z

If instance of class is created manually and injection is not requested it should not work if I understand how it works. Can you describe how did you test that?

snjeza · 2016-11-22T13:32:07Z

I have created che-server, started it with different che.properies, created a project, reviewed mounted volumes using "docker inspect". Everything works as expected.

garagatyi · 2016-11-22T13:41:47Z

@sleshchenko correct me if I'm mistaken but looks like ws-agent configuration works in a different way.

snjeza · 2016-11-22T14:09:05Z

When excluding the Z option, the volumes are mounted in the following way:

 "Mounts": [
            {
                "Source": "/data/workspaces/wksp-8t9p",
                "Destination": "/projects",
                "Mode": "",
                "RW": true,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/data/lib/linux_amd64/terminal",
                "Destination": "/mnt/che/terminal",
                "Mode": "ro",
                "RW": false,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/data/lib/ws-agent.tar.gz",
                "Destination": "/mnt/che/ws-agent.tar.gz",
                "Mode": "ro",
                "RW": false,
                "Propagation": "rprivate"
            }
        ],

sleshchenko · 2016-11-22T14:13:01Z

@snjeza Please create folder plugin-conf in CHE_LOCAL_CONF_DIR. As far as I understand it can be empty. And then mounts should include extra one with "Destination": /mnt/che/conf"". Please make ensure that it is mounted with 'Z' suffix when configuration contains it. If no, please try to add requestInjection(extConfBindingProvider); in DockerExtServerModule

snjeza · 2016-11-22T14:31:11Z

The following are the mounted volumes:

 "Mounts": [
            {
                "Source": "/data/workspaces/wksp-8t9p",
                "Destination": "/projects",
                "Mode": "",
                "RW": true,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/data/lib/linux_amd64/terminal",
                "Destination": "/mnt/che/terminal",
                "Mode": "ro",
                "RW": false,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/data/lib/ws-agent.tar.gz",
                "Destination": "/mnt/che/ws-agent.tar.gz",
                "Mode": "ro",
                "RW": false,
                "Propagation": "rprivate"
            },
            {
                "Source": "/conf/plugin-conf",
                "Destination": "/mnt/che/conf",
                "Mode": "ro",
                "RW": false,
                "Propagation": "rprivate"
            }
        ],

with the Z option

"Mounts": [
            {
                "Source": "/data/workspaces/wksp-jvuc",
                "Destination": "/projects",
                "Mode": "Z",
                "RW": true,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/lib/linux_amd64/terminal",
                "Destination": "/mnt/che/terminal",
                "Mode": "ro,Z",
                "RW": false,
                "Propagation": "rprivate"
            },
            {
                "Source": "/data/conf/plugin-conf",
                "Destination": "/mnt/che/conf",
                "Mode": "ro,Z",
                "RW": false,
                "Propagation": "rprivate"
            },
            {
                "Source": "/tmp/lib/ws-agent.tar.gz",
                "Destination": "/mnt/che/ws-agent.tar.gz",
                "Mode": "ro,Z",
                "RW": false,
                "Propagation": "rprivate"
            }
        ],

garagatyi · 2016-11-22T15:20:19Z

It is interesting. We debugged DockerExtConfBindingProvider usage and guice injected dependencies between 1st and 2nd calls of get method of this class. Sorry that we asked you to check more and more but we missed that Guice behavior with singleton classes.

garagatyi · 2016-11-24T12:12:29Z

ci-build

codenvy-ci · 2016-11-24T16:20:05Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1111/

Signed-off-by: Snjezana Peco <[email protected]>

garagatyi suggested changes Nov 21, 2016

View reviewed changes

snjeza force-pushed the issue3129 branch from 6b06b59 to ccb20ce Compare November 21, 2016 11:47

garagatyi suggested changes Nov 21, 2016

View reviewed changes

snjeza force-pushed the issue3129 branch from ccb20ce to cb9de5b Compare November 21, 2016 13:12

garagatyi suggested changes Nov 21, 2016

View reviewed changes

CHE-3129 - Project can't be created without Z option

2f8e8a2

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue3129 branch from cb9de5b to 2f8e8a2 Compare November 21, 2016 16:06

garagatyi approved these changes Nov 22, 2016

View reviewed changes

sleshchenko approved these changes Nov 22, 2016

View reviewed changes

garagatyi added the status/pending-merge label Nov 25, 2016

garagatyi added this to the 5.0.0-M9 milestone Nov 25, 2016

vparfonov approved these changes Dec 15, 2016

View reviewed changes

vparfonov merged commit 9f5a7fb into eclipse-che:master Dec 15, 2016

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

CHE-3129 - Project can't be created without Z option (eclipse-che#3130)

8d178f1

Signed-off-by: Snjezana Peco <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHE-3129 - Project can't be created without Z option #3130

CHE-3129 - Project can't be created without Z option #3130

snjeza commented Nov 19, 2016

codenvy-ci commented Nov 19, 2016

benoitf commented Nov 20, 2016

codenvy-ci commented Nov 20, 2016

tolusha commented Nov 21, 2016

l0rd commented Nov 21, 2016

garagatyi left a comment

garagatyi Nov 21, 2016

snjeza commented Nov 21, 2016

garagatyi Nov 21, 2016

garagatyi Nov 21, 2016

garagatyi Nov 21, 2016

snjeza commented Nov 21, 2016

garagatyi left a comment

garagatyi Nov 21, 2016

garagatyi Nov 21, 2016

garagatyi Nov 21, 2016

snjeza commented Nov 21, 2016

garagatyi left a comment

garagatyi Nov 22, 2016

sleshchenko commented Nov 22, 2016

snjeza commented Nov 22, 2016

garagatyi commented Nov 22, 2016

snjeza commented Nov 22, 2016

garagatyi commented Nov 22, 2016

snjeza commented Nov 22, 2016

sleshchenko commented Nov 22, 2016

snjeza commented Nov 22, 2016 •

edited

Loading

garagatyi commented Nov 22, 2016

garagatyi commented Nov 24, 2016

codenvy-ci commented Nov 24, 2016

CHE-3129 - Project can't be created without Z option #3130

CHE-3129 - Project can't be created without Z option #3130

Conversation

snjeza commented Nov 19, 2016

What does this PR do?

What issues does this PR fix or reference?

codenvy-ci commented Nov 19, 2016

benoitf commented Nov 20, 2016

codenvy-ci commented Nov 20, 2016

tolusha commented Nov 21, 2016

l0rd commented Nov 21, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Nov 21, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Nov 21, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Nov 22, 2016

snjeza commented Nov 22, 2016

garagatyi commented Nov 22, 2016

snjeza commented Nov 22, 2016

garagatyi commented Nov 22, 2016

snjeza commented Nov 22, 2016

sleshchenko commented Nov 22, 2016

snjeza commented Nov 22, 2016 • edited Loading

garagatyi commented Nov 22, 2016

garagatyi commented Nov 24, 2016

codenvy-ci commented Nov 24, 2016

snjeza commented Nov 22, 2016 •

edited

Loading