webview to start pipeline #323

sudhirverma · 2020-06-07T15:39:53Z

Starting Pipeline with git resource and image resource

Starting Pipeline with workspace

codecov-commenter · 2020-06-07T15:43:17Z

Codecov Report

Merging #323 into master will decrease coverage by 1.55%.
The diff coverage is 25.21%.

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage   74.16%   72.61%   -1.56%     
==========================================
  Files          45       47       +2     
  Lines        3399     3484      +85     
  Branches      632      644      +12     
==========================================
+ Hits         2521     2530       +9     
- Misses        878      954      +76

Impacted Files	Coverage Δ
src/tkn.ts	`63.14% <11.90%> (-2.28%)`	⬇️
src/tekton/webviewstartpipeline.ts	`18.75% <18.75%> (ø)`
src/view/pipeline/pipelineViewLoader.ts	`22.85% <22.85%> (ø)`
src/tekton/tektonitem.ts	`77.21% <30.00%> (-7.72%)`	⬇️
src/tekton/pipeline.ts	`65.75% <44.44%> (-2.43%)`	⬇️
src/tekton/pipelinecontent.ts	`93.45% <100.00%> (ø)`
src/util/MultiStepInput.ts	`10.16% <0.00%> (-2.95%)`	⬇️
src/pipeline/preview.ts	`9.82% <0.00%> (-0.80%)`	⬇️
src/tools.ts	`94.68% <0.00%> (-0.06%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a77a31...cb31c55. Read the comment docs.

sudhirverma · 2020-06-14T18:29:36Z

@evidolob and @siamaksade

webview for starting pipeline.

sudhirverma · 2020-06-17T09:20:01Z

Workspace UI

sudhirverma · 2020-06-23T06:55:13Z

starting pipeline with pipeline resource

sudhirverma · 2020-06-23T08:15:06Z

Starting pipeline with workspace

evidolob

There are some vulnerabilities in packages that you add:

                                                                                
                       === npm audit security report ===                        
                                                                                
┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Cross-Site Scripting                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.5.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @patternfly/react-catalog-view-extension                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @patternfly/react-catalog-view-extension > patternfly >      │
│               │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1518                            │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Cross-Site Scripting                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ bootstrap-select                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=1.13.6                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @patternfly/react-catalog-view-extension                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @patternfly/react-catalog-view-extension > patternfly >      │
│               │ bootstrap-select                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1522                            │
└───────────────┴──────────────────────────────────────────────────────────────┘

Can you fix them?

Also as I say in comment you should add comment for all copied files with link where you take them.

Also some combobox dropdown list not rendered correctly:

evidolob · 2020-06-23T11:31:47Z

package.json

+          "Title": "Start Pipeline",
+          "type": "boolean",
+          "default": false,
+          "description": "Enable/disable to Start Pipeline in webView"


Enable/disable to Start Pipeline in webView Is not clear as for me,
maybe Enable WebView Wizard to Start Pipeline is better.
As for me Enable/disable is very confusing.

evidolob · 2020-06-23T11:32:50Z

src/tekton/pipeline.ts

  static async start(pipeline: TektonNode): Promise<string> {
+    if (Pipeline.checkWebViewStartPipeline()) {


Maybe having separate command for webview wizard will be better? @siamaksade ?

I think enabling webview from setting would be better.

evidolob · 2020-06-23T11:35:19Z

src/tekton/pipeline.ts

@@ -33,7 +46,7 @@ export class Pipeline extends TektonItem {
    const pipelineTrigger = data.map<Trigger>(value => ({
      name: value.metadata.name,
      resources: value.spec.resources,
-      params: value.spec.params ? value.spec.params : undefined,
+      parameters: value.spec.params ? value.spec.params : undefined,


there you can use value.spec.params ?? undefined see nullish-coalescing But in this case is no sense to do that check.

evidolob · 2020-06-23T11:38:39Z

src/tekton/webviewstartpipeline.ts

+}
+
+
+export const pipelineData = async (name: string) => {


I think you can define function instead of exporting constant, which you assign lambda, or you have particular reason to do that?

No particular reason for using lambda. I will use function here.

evidolob · 2020-06-23T11:40:42Z

src/view/pipeline/app/common/ModalStructure.tsx

+ *  Copyright (c) Red Hat, Inc. All rights reserved.
+ *  Licensed under the MIT License. See LICENSE file in the project root for license information.
+ *-----------------------------------------------------------------------------------------------*/
+


It seems that copied file, you need to add comment from which repository you copied all this files.

evidolob · 2020-06-23T11:45:00Z

src/view/pipeline/pipelineViewLoader.ts

+      localResourceRoots: [localResourceRoot],
+      retainContextWhenHidden: true
+    });
+    panel.iconPath = vscode.Uri.file(path.join(PipelineViewLoader.extensionPath, 'images/tekton.svg'));


This will not work for windows, this line should be panel.iconPath = vscode.Uri.file(path.join(PipelineViewLoader.extensionPath, 'images','tekton.svg'));

evidolob · 2020-06-23T11:47:23Z

src/view/pipeline/pipelineViewLoader.ts

+    panel.webview.html = PipelineViewLoader.getWebviewContent(PipelineViewLoader.extensionPath, context);
+    panel.webview.onDidReceiveMessage(async (event) => {
+      if (event.action === 'cancel') {
+        await commands.executeCommand('workbench.action.closeActiveEditor');


You should use panel.webview.dispose() instead of calling 'commands.executeCommand('workbench.action.closeActiveEditor');' command

dispose function does not exist on type Webview

But it exists on WebviewPanel

evidolob · 2020-06-23T11:47:33Z

src/view/pipeline/pipelineViewLoader.ts

+        await commands.executeCommand('workbench.action.closeActiveEditor');
+      }
+      if (event.action === 'start') {
+        await commands.executeCommand('workbench.action.closeActiveEditor');


evidolob · 2020-06-23T11:48:46Z

src/view/pipeline/pipelineViewLoader.ts

+      path.join(reactAppRootOnDisk, 'pipelineView.js'),
+    );
+    const reactAppUri = reactAppPathOnDisk.with({ scheme: 'vscode-resource' });
+    const htmlString: Buffer = fs.readFileSync(path.join(reactAppRootOnDisk, 'index.html'));


You should mostly never use synchronous node API, please use readFile instead of readFileSync

siamaksade · 2020-06-23T16:06:58Z

We should pivot to a more wizard-like experience instead of the modal-based approach that we have in dev console:

Also the the look and feel of it should match much closer to the vs code UI elements. This wizard would we quite similar to the Settings UI:

evidolob · 2020-06-24T06:27:17Z

@siamaksade For start @sudhirverma just copy code from dev-console, so basically we embed part of the dev-console UI.
We start discussion with dev-console team, about code sharing. They have a plan to extract some parts of dev-console from OS dashboard repository. But it seems it take time, and will be not so fast.

You propose to implement own version of wizard, base on UI mockup for dev-console?

To be honest I'm not very happy with that copied code, especially with it dependencies, compiled single JS file for that wizard is 24Mb, where all archive for vscode-tekton-0.2.0.vsix is 9Mb.

So I'm definitely +1 to implement own, but we need to figure out how we can share that new implementation with dev-console team. And we need to be carefully about dependencies what we use in it.

evidolob · 2020-06-24T06:30:38Z

package.json

    "webpack-cli": "^3.3.11"
  },
  "dependencies": {
    "@kubernetes/client-node": "^0.10.2",
+    "@patternfly/patternfly": "2.71.5",


All dependencies that we use in any webview should be moved to devDependencies as we compiled them it webpack bundle, and we do not need to distribute them inside vsix package

siamaksade · 2020-06-24T13:15:36Z

@evidolob I don't think we should sacrifice the UX for the sake of sharing code with dev console. If sharing code with dev console saves time, sure. If adds overhead, not sure what the value is.

sudhirverma · 2020-07-03T13:15:43Z

Closing this PR as we are implementing a new wizard according to vscode.

sudhirverma marked this pull request as draft June 7, 2020 15:40

sudhirverma force-pushed the 310 branch 4 times, most recently from a6b5a6c to 7897813 Compare June 14, 2020 18:15

sudhirverma force-pushed the 310 branch 2 times, most recently from 9a8363f to 61492a8 Compare June 22, 2020 16:35

sudhirverma marked this pull request as ready for review June 23, 2020 01:28

sudhirverma force-pushed the 310 branch from bff3ff0 to ca1182d Compare June 23, 2020 03:04

sudhirverma requested a review from evidolob June 23, 2020 03:12

sudhirverma marked this pull request as draft June 23, 2020 06:00

sudhirverma added 15 commits June 23, 2020 13:00

webview to start pipeline

3502818

add hadir and footer

ac67868

add scss compilation in webpack

256ba53

Added dependency

4b4332e

add Parameters support

88327ac

refactor and creating UI for resource

e7ad9ec

Added dropdown for pipelineresource.

a10af3f

implementing UI for workspaces

2432092

Adding css for workspaces

8151122

Fix scss

4106eb9

set background-color

21aa367

Fixing workspace

0f7fdf3

implementing UI for workspaces

7feecef

Fix workspace UI

41f8bba

using kubectl to create Ui for git resouce, image resource and workspase

e9097f3

sudhirverma added 8 commits June 23, 2020 13:00

working on starting pipeline from webview.

827ee79

fix resource formate to start pipeline

e1ccf5d

Fix start pipeline

6ba490f

close window on click cancel button

db53716

Fix test

65f1648

webpack to start pipeline

b4f322e

Fixing PVC.

761743a

Added support for PVC

092703f

sudhirverma force-pushed the 310 branch from 2c9513d to 092703f Compare June 23, 2020 07:45

sudhirverma changed the title ~~webview to start pipeline [WIP]~~ webview to start pipeline Jun 23, 2020

Fix starting pipeline from web view

cb31c55

sudhirverma marked this pull request as ready for review June 23, 2020 08:20

evidolob requested changes Jun 23, 2020

View reviewed changes

evidolob reviewed Jun 24, 2020

View reviewed changes

sudhirverma closed this Jul 3, 2020

sudhirverma deleted the 310 branch July 8, 2020 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webview to start pipeline #323

webview to start pipeline #323

sudhirverma commented Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jun 7, 2020 •

edited

Loading

sudhirverma commented Jun 14, 2020

sudhirverma commented Jun 17, 2020

sudhirverma commented Jun 23, 2020 •

edited

Loading

sudhirverma commented Jun 23, 2020

evidolob left a comment

evidolob Jun 23, 2020

evidolob Jun 23, 2020

sudhirverma Jun 23, 2020

evidolob Jun 23, 2020

evidolob Jun 23, 2020

sudhirverma Jun 23, 2020

evidolob Jun 23, 2020

evidolob Jun 23, 2020

evidolob Jun 23, 2020

sudhirverma Jun 23, 2020

evidolob Jun 24, 2020

evidolob Jun 23, 2020

evidolob Jun 23, 2020

siamaksade commented Jun 23, 2020

evidolob commented Jun 24, 2020

evidolob Jun 24, 2020

siamaksade commented Jun 24, 2020

sudhirverma commented Jul 3, 2020

		static async start(pipeline: TektonNode): Promise<string> {
		if (Pipeline.checkWebViewStartPipeline()) {

webview to start pipeline #323

webview to start pipeline #323

Conversation

sudhirverma commented Jun 7, 2020 • edited Loading

codecov-commenter commented Jun 7, 2020 • edited Loading

Codecov Report

sudhirverma commented Jun 14, 2020

sudhirverma commented Jun 17, 2020

sudhirverma commented Jun 23, 2020 • edited Loading

sudhirverma commented Jun 23, 2020

evidolob 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

siamaksade commented Jun 23, 2020

evidolob commented Jun 24, 2020

Choose a reason for hiding this comment

siamaksade commented Jun 24, 2020

sudhirverma commented Jul 3, 2020

sudhirverma commented Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jun 7, 2020 •

edited

Loading

sudhirverma commented Jun 23, 2020 •

edited

Loading