Workspace loader #8838

vitaliy-guliy · 2018-02-19T16:01:02Z

What does this PR do?

Adds an application for starting a workspace and loading corresponding IDE.

What issues does this PR fix or reference?

#8556
#8682

Release Notes

Application to start a workspace and open predefined IDE

benoitf

I don't see unit tests of the loader ?

benoitf · 2018-02-19T16:02:55Z

assembly/assembly-workspace-loader-war/src/main/webapp/WEB-INF/web.xml

+        <url-pattern>/loader/*</url-pattern>
+    </servlet-mapping>
+
+    <security-role>


I suppose we could remove security-role

benoitf · 2018-02-19T16:35:11Z

workspace-loader/tsconfig.json

+    // "experimentalDecorators": true,        /* Enables experimental support for ES7 decorators. */
+    // "emitDecoratorMetadata": true,         /* Enables experimental support for emitting type metadata for decorators. */
+  }
+}


Could we have more strict checking ? like variables not used, emit only if there is no error in the code

benoitf · 2018-02-19T16:35:45Z

workspace-loader/webpack.common.js

@@ -0,0 +1,36 @@
+/*******************************************************************************
+ * Copyright (c) 2015-2018 Red Hat, Inc.


it should be only 2018, in 2015 this loader didn't exist

How it will be for pom.xml? 2018?

@vitaliy-guliy I would say yes
just specify correct inception year like https://github.com/eclipse/che/blob/master/dashboard/pom.xml#L27

Then we will have following

Copyright (c) 2018-2018 Red Hat, Inc.

you may want to add

<properties> <license_years>2018</license_years> </properties>

to have only

Copyright (c) 2018 Red Hat, Inc.

Let's decide. We have only 2018 or period 2018-2018.

benoitf · 2018-02-19T16:36:10Z

workspace-loader/webpack.common.js

+        ]
+    },
+    resolve: {
+        extensions: ['.tsx', '.ts', '.js']


why .tsx on this project ?

benoitf · 2018-02-19T16:37:46Z

workspace-loader/webpack.dev.js

+const HtmlWebpackPlugin = require('html-webpack-plugin');
+
+module.exports = merge(common, {
+    devtool: 'inline-source-map',


sourcemap is working if not enabled in tsconfig ?

I have just tested when disabled in tsconfig. It works but when debugging source files have a lot of extra information.

benoitf · 2018-02-19T16:38:18Z

workspace-loader/webpack.dev.js

+    },
+    devServer: {
+        contentBase: './dist',
+        port: 3050,


any reason to use 3050 instead of classical 3000 ?

Just a historical reason. Changed to 3000.

benoitf · 2018-02-19T16:38:36Z

workspace-loader/webpack.prod.js

@@ -0,0 +1,44 @@
+/*******************************************************************************
+ * Copyright (c) 2015-2018 Red Hat, Inc.


benoitf · 2018-02-20T09:26:23Z

is it also possible to have a docker build like for dashboard app, so if ppl don't have the expected nodejs version it's still fine

benoitf · 2018-02-20T09:28:45Z

assembly/assembly-workspace-loader-war/pom.xml

+    <parent>
+        <artifactId>che-assembly-parent</artifactId>
+        <groupId>org.eclipse.che</groupId>
+        <version>6.1.0-SNAPSHOT</version>


version here is incorrect

benoitf · 2018-02-20T10:18:23Z

workspace-loader/pom.xml

+    <name>Che Workspace Loader :: Web App</name>
+    <inceptionYear>2018</inceptionYear>
+    <dependencies>
+        <dependency>


could you please explain why we're having this dependency ?
AFAIK this module is not using it.

Removed as unused. Thanks.

benoitf · 2018-02-20T10:19:00Z

workspace-loader/pom.xml

+                        <exclude>**/*.eot</exclude>
+                        <exclude>**/*.css</exclude>
+                        <exclude>**/*.woff</exclude>
+                    </excludes>


is that the list of excludes is accurate ? it seems some of the files are not there so maybe the whole configuration can be removed

benoitf · 2018-02-20T10:19:42Z

workspace-loader/pom.xml

+                <artifactId>maven-source-plugin</artifactId>
+                <configuration>
+                    <includePom>true</includePom>
+                    <includes>


I suppose this is a copy from dashboard module but then I'm not sure the include are expected

benoitf · 2018-02-20T10:20:46Z

could you please also list CQsof new dependencies in the description of the PR ?

vitaliy-guliy · 2018-02-20T15:16:27Z

List of CQs
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=15581

vitaliy-guliy · 2018-02-21T14:48:53Z

Docker build is added

ashumilova · 2018-02-26T08:23:03Z

@vitaliy-guliy CQ is approved https://dev.eclipse.org/ipzilla/show_bug.cgi?id=15581

benoitf · 2018-02-27T09:28:28Z

workspace-loader/karma.conf.js

@@ -0,0 +1,77 @@
+// Karma configuration


it will require copyright headers

benoitf · 2018-02-27T09:28:49Z

workspace-loader/karma.conf.js

@@ -0,0 +1,77 @@
+// Karma configuration
+// Generated on Thu Feb 22 2018 15:39:12 GMT+0200 (EET)


maybe we could delete this generated line

not actual anymore

benoitf · 2018-02-27T09:28:58Z

workspace-loader/karma.conf.js

+  config.set({
+    webpack: webpackConfig,
+    mime: {
+      'text/x-typescript': ['ts','tsx']


do we have tsx ?

not actual anymore

benoitf · 2018-02-27T10:41:53Z

about the tests, I think that karma/mocha can be replaced in favor of jest
so, no need to have Chrome, etc, it runs very fast

$ ./node_modules/.bin/jest 
 PASS  test/test.spec.ts
  Workspace Loader
    ✓ > must have "workspace-loader" in DOM > (11ms)
    ✓ > test when workspace key is not specified (8ms)
    ✓ > test getWorkspace with test value (31ms)
    > must handle workspace
      ✓ > must be called (2ms)
    > must open IDE for RUNNING workspace
      ✓ > must be called (2ms)
    > must start STOPPED workspace
      ✓ > must be called (2ms)
    > must restart STOPPING workspace
      ✓ > must be called (2ms)

  console.error src/index.ts:32
    Workspace is not defined

Test Suites: 1 passed, 1 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        1.219s
Ran all test suites.

benoitf · 2018-02-27T13:00:53Z

workspace-loader/tsconfig.json

+    "src"
+  ],
+  "compilerOptions": {
+    /* Basic Options */


JSON is not allowing comments, so all comments should be removed
you may check on any online JSON validator

vitaliy-guliy · 2018-02-28T17:16:47Z

Babel, karma and jasmine have been removed.
Now we use jest.
Passing tests become more quick.
Thanks.

vitaliy-guliy · 2018-02-28T17:18:05Z

@benoitf Do we need to pass tests while compiling the app?

benoitf · 2018-02-28T17:28:17Z

I would say it's better so PR build will fail if someone broke the tests

benoitf

maybe new CQs are required for the tests ?

vitaliy-guliy · 2018-03-01T13:57:04Z

CQ for Jest
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=15795

benoitf · 2018-03-01T14:09:49Z

@vitaliy-guliy could you add the version of jest in CQ and append it with "and later version" so we can easily update version without new CQs
also you've to click on the button Issues addressed, return CQ to IPTeam

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

Adding workspace loader application.

vitaliy-guliy requested review from benoitf, evidolob, ashumilova, vparfonov and slemeur February 19, 2018 16:01

vitaliy-guliy requested review from riuvshin and skabashnyuk as code owners February 19, 2018 16:01

benoitf reviewed Feb 19, 2018

View reviewed changes

benoitf reviewed Feb 20, 2018

View reviewed changes

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Feb 23, 2018

benoitf reviewed Feb 27, 2018

View reviewed changes

benoitf approved these changes Feb 28, 2018

View reviewed changes

Vitaliy Guliy and others added 21 commits March 5, 2018 14:02

Fix name of workspace-loader directory

315e528

Fix name of workspace-loader directory

70c640c

Cleanup

448bbe4

Workspace loader application usage in dashboard

a259456

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

Fix license years

3fe4e8e

Remove .tsx

0d4565a

Remove unused dependency

b639037

Cleanup

a5faf98

Cleanup

2cf0f54

Fixup

0b3d8f4

Remove package-lock.json

f177c10

Build workspace loader in docker

48dec0f

Better cleaning

478c8f5

Add check for unused locals, parameters, fallthrough cases

cd73ec6

Add unit tests

38b0262

Fixup

3c9456b

Fixup

d0b418d

Use jest instead karma+jasmine

66694bb

Remove comments from tsconfig

40c76d8

Run tests when building

5f92e85

Original query params must be transfered to IDE

cddb76f

ashumilova force-pushed the workspace-loader branch from bce7100 to cddb76f Compare March 5, 2018 12:02

ashumilova merged commit 3da13d5 into master Mar 5, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 5, 2018

benoitf added this to the 6.2.0 milestone Mar 5, 2018

riuvshin mentioned this pull request Mar 6, 2018

[REGRESSION] CHE Multiuser, cannot start workspace from the Dashboard #9024

Closed

hkolvenbach pushed a commit to hkolvenbach/che that referenced this pull request Mar 12, 2018

Workspace loader (eclipse-che#8838)

5e2be42

Adding workspace loader application.

vitaliy-guliy deleted the workspace-loader branch March 13, 2018 14:09

skabashnyuk pushed a commit that referenced this pull request Jan 3, 2020

Workspace loader (#8838)

63a68d9

Adding workspace loader application.

skabashnyuk pushed a commit to skabashnyuk/che that referenced this pull request Mar 11, 2020

Workspace loader (eclipse-che#8838)

7698eaa

Adding workspace loader application.

		@@ -0,0 +1,36 @@
		/*******************************************************************************
		* Copyright (c) 2015-2018 Red Hat, Inc.

		@@ -0,0 +1,44 @@
		/*******************************************************************************
		* Copyright (c) 2015-2018 Red Hat, Inc.

		@@ -0,0 +1,77 @@
		// Karma configuration
		// Generated on Thu Feb 22 2018 15:39:12 GMT+0200 (EET)

Workspace loader #8838

Workspace loader #8838

Conversation

vitaliy-guliy commented Feb 19, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

benoitf 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

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

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

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

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

benoitf commented Feb 20, 2018

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

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

benoitf commented Feb 20, 2018

vitaliy-guliy commented Feb 20, 2018

vitaliy-guliy commented Feb 21, 2018

ashumilova commented Feb 26, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaliy-guliy commented Feb 28, 2018

vitaliy-guliy commented Feb 28, 2018

benoitf commented Feb 28, 2018

benoitf left a comment

Choose a reason for hiding this comment

vitaliy-guliy commented Mar 1, 2018

benoitf commented Mar 1, 2018