Adding support for function #156

sudhirverma · 2021-07-13T11:20:15Z

joshuawilson · 2021-07-20T17:57:09Z

src/cli/cli-config.json

@@ -59,4 +89,4 @@
      }
    }
  }
-}
+}


Needs a new line here.

src/functions/functionsTreeItem.ts

joshuawilson · 2021-07-20T18:07:17Z

Please do not merge without adding tests.

src/functions/build-function.ts

joshuawilson · 2021-07-21T16:38:42Z

src/functions/function-type.ts

@@ -3,10 +3,23 @@
 *  Licensed under the MIT License. See LICENSE file in the project root for license information.
 *-----------------------------------------------------------------------------------------------*/

+import * as vscode from 'vscode';
+import { WorkspaceFolder } from 'vscode';


Why import all and a specific thing?

evidolob

If I provide wrong docker image name(with not valid docker registry path), build pass successfully but when I run deploy I got Deploying Function... notification which load never replace with error notification.

Another feedback, I think we need to stream logs on function build command, as that process can take a while, and I as user want to track build progress.

evidolob · 2021-07-21T11:55:18Z

package.json

@@ -382,4 +420,4 @@
      }
    }
  }
-}
+}


Please add new line.

evidolob · 2021-08-11T11:50:04Z

src/functions/function-tree-view.ts

+    if (functionCheck.test(result.stdout)) {
+      return [new FunctionNodeImpl(null, 'No Function Found', FunctionContextType.NONE, TreeItemCollapsibleState.None, null)];
+    }
+    window.showErrorMessage(`Fail to parse Json Error: ${getStderrString(error)}`);


I got this error when my cluster doesn't contains any functions. The func CLI ignore -o json and provide output:

No functions found null

for func list -o json

I have updated the func cli version they have done changes in the output.

apple@Apples-MacBook-Pro vscode-knative % func list -o json No functions found in knative-serving namespace apple@Apples-MacBook-Pro vscode-knative % ~/.vs-func/func list -o json No functions found null

sudhirverma · 2021-08-11T12:26:05Z

Another feedback, I think we need to stream logs on function build command, as that process can take a while, and I as user want to track build progress.

I think we can run the build and deploy command on the terminal so that we can see the stream logs

mohitsuman · 2021-09-15T20:23:59Z

@sudhirverma can you please have a look at the failed GH builds, if we can fix them.

odockal · 2021-09-17T12:34:55Z

@mohitsuman @sudhirverma Fixes for failing builds could be solved in #148.

odockal · 2021-09-17T18:22:57Z

Rebase this PR please.

codecov-commenter · 2021-12-17T08:05:48Z

Codecov Report

Merging #156 (02b2567) into main (6b4949e) will decrease coverage by 2.81%.
The diff coverage is 82.55%.

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
- Coverage   96.55%   93.73%   -2.82%     
==========================================
  Files          51       70      +19     
  Lines        2233     2841     +608     
  Branches      336      452     +116     
==========================================
+ Hits         2156     2663     +507     
- Misses         77      178     +101

Impacted Files	Coverage Δ
src/util/constants.ts	`0.00% <0.00%> (ø)`
src/cli/cmdCli.ts	`53.62% <20.00%> (+2.10%)`	⬆️
src/util/existing-workspace-folder-pick.ts	`33.33% <33.33%> (ø)`
.../functions/function-tree-view/functionsTreeItem.ts	`36.95% <36.95%> (ø)`
src/cli/cli-config.ts	`76.34% <50.00%> (-4.38%)`	⬇️
src/extension.ts	`83.67% <52.94%> (-16.33%)`	⬇️
src/cli/func-api.ts	`65.90% <65.90%> (ø)`
src/functions/active-namespace.ts	`76.92% <76.92%> (ø)`
src/functions/function-command/create-function.ts	`79.31% <79.31%> (ø)`
src/util/stderrstring.ts	`80.00% <80.00%> (ø)`
... and 17 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 6b4949e...02b2567. Read the comment docs.

lstocchi

i started playing with it. The major problem i faced is that it asks me to download clis (kubectl and func) multiple times.

E.g I have a function opened in workspace, it is displayed fine on the tree.
I create a new function using the wizard and open it in same workspace, the tree is not refreshed correctly and i notice it asks me to download the cli again... but i already downloaded it before otherwise it would not have worked.

lstocchi · 2021-12-17T14:47:48Z

src/cli/cli-config.ts

                  // Change the file permissions if on Linux or Mac
                  if (Platform.OS !== 'win32') {
                    fs.chmodSync(toolCacheLocation, 0o755);
+                  } else {
+                    fs.chmodSync(toolCacheLocation, '+x');


Not quite sure about it... is it executing chmod necessary in windows? isn't .exe already executable?

it only read and write -rw-r--r--@ 1 apple staff 66777600 Dec 20 18:18 func_windows_amd64.exe

lstocchi · 2021-12-17T14:53:13Z

src/cli/func-api.ts

+import { CliCommand, CmdCli, createCliCommand } from './cmdCli';
+
+function funcCliCommand(cmdArguments: string[]): CliCommand {
+  return createCliCommand('func', ...cmdArguments);


Shouldn't this be func.exe in windows?

lstocchi · 2021-12-20T09:10:32Z

src/cli/cli-config.ts

+                  if (toolDlLocation.endsWith('.zip') || toolDlLocation.endsWith('.tar.gz')) {
+                    await Archive.unzip(
+                      toolDlLocation,
+                      path.resolve(Platform.getUserHomePath(), cliFile),


Couldn't you use toolCacheLocation as the other else if branch and merge both?

lstocchi · 2021-12-20T09:12:59Z

src/cli/cli-config.ts

@@ -67,41 +64,10 @@ async function getVersion(location: string): Promise<string> {
  if (cmd === 'kubectl') {
    version = KubectlAPI.getKubectlVersion(location);
  }
+  if (cmd === 'func') {


Putting this here as i can't comment above but it's about line ~48.
The interface Config is

export interface Config { cmd?: CliConfig; kn?: CliConfig; kubectl?: CliConfig; }

Why cmd?? What does that represent?
Shouldn't you add func?: CliConfig there as well?

lstocchi · 2021-12-20T09:27:11Z

src/extension.ts

+  // eslint-disable-next-line @typescript-eslint/no-floating-promises
+  startTelemetry(extensionContext);
+  // eslint-disable-next-line @typescript-eslint/no-floating-promises
+  setCommandContext(CommandContext.funcDisableRun, false);


Why did you add it? I cannot find any way to change it to true, missing something?

for localFunctionsEnablement context or node it will be disabled and always false.

lstocchi · 2021-12-20T11:21:19Z

src/functions/function-command/create-function.ts

+  id: string,
+  items: ValidatorResponseItem[],
+): ValidatorResponse {
+  if (fs.existsSync(pathValue) && pathValue) {


Should not this be the opposite order? So if pathValue is null you don't execute existsSync?

lstocchi · 2021-12-20T11:27:09Z

src/functions/function-command/delete-function.ts

+      }
+      if (context.functionStatus === FunctionStatus.CLUSTERLOCALBOTH || context.functionStatus === FunctionStatus.LOCALONLY) {
+        const funcYaml = path.join(context.contextPath.fsPath, 'func.yaml');
+        await fsExtra.remove(funcYaml);


Why are you deleting the function locally? Maybe users want to delete it on the current namespace only.
I would completely remove the delete action for local functions. What are we going to delete? Only the func.yaml? All the code? How can we decide? If users wants to delete something locally they can do it manually or we need to be extra clear about what we are going to do.

I will wait till we get confirmation on what should we do with the local function whether we should delete the function or not.

lstocchi · 2021-12-20T12:04:13Z

src/functions/function-command/run-function.ts

+  if (!context) {
+    return null;
+  }
+  await knExecutor.executeInTerminal(FuncAPI.runFunc(context.contextPath.fsPath));


Should we add a guard for contextPath or it contains a valid value in all cases?

yes, it will contain the valid value I will test it on windows.

lstocchi · 2021-12-20T12:08:42Z

src/functions/function-type.ts

+}
+
+export interface Namespace {
+  Kind?: string;


Why do you need Kind?

lstocchi · 2021-12-20T12:09:23Z

src/functions/functionsExplorer.ts

+  async reveal(item: FunctionNode): Promise<void> {
+    await this.refresh(item.getParent());
+    await this.treeView.reveal(item);
+    await this.treeView.reveal(item);


this row is a duplicate?

lstocchi

Tried again a full example like create new function -> build -> run -> deploy and delete. Everything worked fine. There is just an annoying dialog which pops up telling me i'm not connected to a cluster. In the video below you can see i have access to a cluster.

lstocchi · 2022-01-03T14:11:23Z

I think there is an issue with the regex that verifies the image name.
If i start writing sio/s it correctly fails telling me Provide full image name in the form registry/namespace/name:tag

but then if i add a dot -> s.io/s it is accepted as correct name and then it fails during execution.

In IJ, Jeff made me add an example in the validation message and a default image name as placeholder so user can just click next if they don't want to change anything.
So the vlidation message looks like -> Provide full image name in the form [registry]/[namespace]/[name]:[tag] (e.g quay.io/boson/image:latest)
and the default image name as placeholder is "quay.io/" + defaultUsername + "/" + name + ":latest" where defaultUsername is the username env variable and name is the function name in func.yaml

lstocchi

Works fine for me. There are some improvements to add (suggested by Jeff) #169 and #168 but it's better to stop adding stuff here. It is becoming huge and difficult to review.

sudhirverma marked this pull request as draft July 13, 2021 11:20

openshift-ci bot added the do-not-merge/work-in-progress label Jul 13, 2021

sudhirverma force-pushed the 155 branch 2 times, most recently from 51cc60c to d702a7d Compare July 19, 2021 12:11

joshuawilson reviewed Jul 20, 2021

View reviewed changes

src/cli/cli-config.json

@@ -59,4 +89,4 @@

}

}

}

}

}

Copy link

Member

joshuawilson Jul 20, 2021

Choose a reason for hiding this comment

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

Needs a new line here.

joshuawilson reviewed Jul 20, 2021

View reviewed changes

src/functions/functionsTreeItem.ts Outdated Show resolved Hide resolved

sudhirverma force-pushed the 155 branch from 03c14cf to 9c069ac Compare July 21, 2021 11:00

joshuawilson reviewed Jul 21, 2021

View reviewed changes

src/functions/build-function.ts Outdated Show resolved Hide resolved

joshuawilson reviewed Jul 21, 2021

View reviewed changes

sudhirverma force-pushed the 155 branch 2 times, most recently from a2e1b24 to 3e14a77 Compare July 22, 2021 10:11

evidolob reviewed Aug 11, 2021

View reviewed changes

sudhirverma requested a review from evidolob August 11, 2021 12:17

sudhirverma force-pushed the 155 branch from ff6f0ca to d44e792 Compare August 11, 2021 18:11

sudhirverma force-pushed the 155 branch from 13f53c7 to 3f75dec Compare September 17, 2021 05:44

sudhirverma added 10 commits September 20, 2021 12:51

Adding support for function

4b715ff

download func if not found in path

8d90447

add feature to open created function in workspace

4144f23

create function from command palette

8a46ad2

Fix build

71e2c33

Fix build

163d021

Added treeview

9b793dd

added delete command for function

254c886

Added build command

1a8eecb

Added deploy command

293a6f1

sudhirverma added 6 commits December 13, 2021 12:51

Fix build

2fba975

Added test for validate item

e9db323

Fix windows test

92a19a7

Fix test

4e498df

Added more test

99a65c6

using single tree view for function

42eb72c

redhat-developer deleted a comment from codecov-commenter Dec 16, 2021

Added test for functionsExplorer

f379423

sudhirverma added 2 commits December 17, 2021 16:09

Added test for func.ts

ed9ab7b

refresh tree view when folder is added or remove

64799fb

sudhirverma marked this pull request as ready for review December 17, 2021 11:00

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 17, 2021

sudhirverma requested a review from lstocchi December 17, 2021 11:05

lstocchi requested changes Dec 20, 2021

View reviewed changes

Fix review changes

a004523

sudhirverma requested a review from lstocchi December 24, 2021 11:51

sudhirverma force-pushed the 155 branch from 8d3f64b to 965c8db Compare December 24, 2021 12:15

Fix windows problem

220f685

sudhirverma force-pushed the 155 branch from 965c8db to 220f685 Compare December 24, 2021 12:26

lstocchi requested changes Dec 24, 2021

View reviewed changes

Fix dont show error message when cluster is active.

61c9e06

sudhirverma force-pushed the 155 branch from 61e3dca to 5a8ef93 Compare January 3, 2022 11:21

Fix test

02b2567

sudhirverma force-pushed the 155 branch from 5a8ef93 to 02b2567 Compare January 3, 2022 11:38

delete -> undeploy

c6d72f7

sudhirverma requested a review from lstocchi January 4, 2022 11:52

lstocchi approved these changes Jan 7, 2022

View reviewed changes

sudhirverma merged commit b5cb711 into redhat-developer:main Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for function #156

Adding support for function #156

sudhirverma commented Jul 13, 2021 •

edited

Loading

joshuawilson Jul 20, 2021

joshuawilson commented Jul 20, 2021

joshuawilson Jul 21, 2021

evidolob left a comment

evidolob Jul 21, 2021

evidolob Aug 11, 2021

sudhirverma Aug 11, 2021

sudhirverma commented Aug 11, 2021 •

edited

Loading

mohitsuman commented Sep 15, 2021

odockal commented Sep 17, 2021

odockal commented Sep 17, 2021

codecov-commenter commented Dec 17, 2021 •

edited

Loading

lstocchi left a comment •

edited

Loading

lstocchi Dec 17, 2021

sudhirverma Dec 21, 2021

lstocchi Dec 17, 2021

lstocchi Dec 20, 2021

lstocchi Dec 20, 2021

lstocchi Dec 20, 2021

sudhirverma Dec 21, 2021 •

edited

Loading

lstocchi Dec 20, 2021

lstocchi Dec 20, 2021

sudhirverma Dec 22, 2021

lstocchi Dec 20, 2021

sudhirverma Dec 21, 2021

lstocchi Dec 20, 2021

lstocchi Dec 20, 2021

lstocchi left a comment

lstocchi commented Jan 3, 2022

lstocchi left a comment

@@ @@ -59,4 +89,4 @@ @@
                     }
                   }
                 }
-              }
+              }

@@ @@ -382,4 +420,4 @@ @@
                     }
                   }
                 }
-              }
+              }

Adding support for function #156

Adding support for function #156

Conversation

sudhirverma commented Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

joshuawilson commented Jul 20, 2021

Choose a reason for hiding this comment

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

sudhirverma commented Aug 11, 2021 • edited Loading

mohitsuman commented Sep 15, 2021

odockal commented Sep 17, 2021

odockal commented Sep 17, 2021

codecov-commenter commented Dec 17, 2021 • edited Loading

Codecov Report

lstocchi left a comment • edited Loading

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

sudhirverma Dec 21, 2021 • edited Loading

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

lstocchi left a comment

Choose a reason for hiding this comment

lstocchi commented Jan 3, 2022

lstocchi left a comment

Choose a reason for hiding this comment

sudhirverma commented Jul 13, 2021 •

edited

Loading

sudhirverma commented Aug 11, 2021 •

edited

Loading

codecov-commenter commented Dec 17, 2021 •

edited

Loading

lstocchi left a comment •

edited

Loading

sudhirverma Dec 21, 2021 •

edited

Loading