containerless function when running locally #3838

gbockus-sf · 2022-02-14T19:11:13Z

What does this PR do?

Add support for running a function directly on the developers system.
Alters the Start Function command to run locally instead of in a container.
Refactor the start function code to support container & containerless
I'm not sure how to see unit test coverage (yet), but I attempted to get full code coverage for the classes I touched in this PR.

What issues does this PR fix or reference?

@W-10535876@
Also deals with W-10535850

Functionality Before

Previously we defaulted to running functions in the containers running locally on the developer system via the 'SFDX: Start Function' command.

Functionality After

There are now two commands available:
'SFDX: Start Function' Starts the function locally on the developers system outside of the container.
'SFDX: Start Container Function' Starts the function in the container locally on the developers system.

gbockus-sf · 2022-02-14T19:13:16Z

packages/salesforcedx-vscode-core/package.json

@@ -61,6 +62,7 @@
    "mocha-multi-reporters": "^1.1.7",
    "mock-spawn": "0.2.6",
    "nyc": "^13",
+    "proxyquire": "2.1.3",


add proxyquire as a dev dep to the core project to enable unit testing outside the integration test flow. We are using this in a few packages already, but didn't have any references in core.

gbockus-sf · 2022-02-14T19:14:12Z

packages/salesforcedx-vscode-core/package.json

+        {
+          "command": "sfdx.force.function.container.start",
+          "when": "sfdx:project_opened && resource =~ /.*/functions/.*/[^/]+(/[^/]+\\.(ts|js|java|json|toml))?$/ && resourceFilename != package.json && resourceFilename != package-lock.json && resourceFilename != tsconfig.json"
+        },


Note the previous sfdx.force.function.start was left unaltered here, but is updated to run a new command function in index.ts.

gbockus-sf · 2022-02-14T19:15:20Z

packages/salesforcedx-vscode-core/package.nls.json

@@ -70,6 +70,7 @@
  "force_analytics_template_create_text": "SFDX: Create Sample Analytics Template",
  "force_function_create": "SFDX: Create Function",
  "force_function_start": "SFDX: Start Function",
+  "force_function_start_container": "SFDX: Start Container Function",


New command that will run the old container based start function code. @sbudhirajadoc

gbockus-sf · 2022-02-14T19:16:57Z

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionContainerStartCommand.ts

@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.


I need to do a full pass at updating/add copyrights.

gbockus-sf · 2022-02-14T19:21:54Z

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionContainerStartCommand.ts

+ * Executes sfdx run:function:start --verbose
+ * @param sourceUri
+ */
+export async function forceFunctionContainerStartCommand(sourceUri?: Uri) {


Did some refactoring here to try and separate some concerns. Previously we have one file that exported both the command and executor. I moved those to different files to enable easier unit testing of each piece of the puzzle.

gbockus-sf · 2022-02-14T19:23:25Z

...vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerStartExecutor.ts

+import { notificationService } from '../../../notifications';
+import { FunctionService } from '../functionService';
+import { FUNCTION_RUNTIME_DETECTION_PATTERN } from '../types/constants';
+


I'll put all these import together.

gbockus-sf · 2022-02-14T19:25:15Z

...es/salesforcedx-vscode-core/src/commands/functions/forceFunctionContainerlessStartCommand.ts

+ * Executes sfdx run:function:start:local --verbose
+ * @param sourceUri
+ */
+export const forceFunctionContainerlessStartCommand = async (


I'll make this consistent across both function start commands.

gbockus-sf · 2022-02-14T19:26:29Z

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionStart.ts

-  }
-};
-
-export class ForceFunctionStartExecutor extends LibraryCommandletExecutor<


Refactored this into a base abstract class that is extended by the container and containerless executors. There was a large chunk of shared code which drove my decision here.

gbockus-sf · 2022-02-14T19:27:41Z

...vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerStartExecutor.ts

+ */
+type ForceFunctionStartErrorType = 'force_function_start_docker_plugin_not_installed_or_started';
+
+const forceFunctionStartErrorInfo: {


The code here is mainly taken from the original executor. I just refactored it into the new abstract methods.

Reading through the code, I see what forceFunctionStartErrorInfo is doing, and looks like this code & its approach originally came from forceFunctionStart.ts (and isn't your fault). But reading through the code, my first thought was... "what?"

This (the original code that was carried forward) seems (IMHO) to be a little over engineered. This might be a good opportunity to clean it up and make it more simple. We could get rid of ForceFunctionStartErrorType and forceFunctionStartErrorInfo, and just change it to:

const forceFunctionStartErrorInfo = { force_function_start_docker_plugin_not_installed_or_started: { cliMessage: 'Cannot connect to the Docker daemon', errorNotificationMessage: nls.localize( 'force_function_start_warning_docker_not_installed_or_not_started' ) } }

...and then further down, not have to do (Object.keys(forceFunctionStartErrorInfo) as ForceFunctionStartErrorType[]).forEach(...

Thoughts?

I felt that "what?" I've had to review this a couple of times to understand it and even then it def seem over engineered. I'll simplify. 👍

gbockus-sf · 2022-02-14T19:28:17Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+  public async cancelFunction(
+    registeredStartedFunctionDisposable: Disposable
+  ): Promise<void> {
+    // TODO: how to stop the localRun


Will need to follow up with a new ticket for how to handle stopping the containerless functions as there is no mechanism for achieving that today.

gbockus-sf · 2022-02-14T19:28:45Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+        console.log('Gordon resolved: ' + msg);
+      })
+      .catch(err => {
+        console.log('Gordon Error: ' + err.message);


Will address consoles. Wasn't sure what exactly should happen in these cases.

gbockus-sf · 2022-02-14T19:29:51Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+      debugPort: FUNCTION_DEFAULT_DEBUG_PORT
+    });
+
+    localRun


LocalRun has a return type of Promise<void>. So there is not handle to the started function process.

gbockus-sf · 2022-02-14T19:30:05Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+    functionName: string,
+    functionDirPath: string
+  ): Promise<void> {
+    console.log('No build for containerless function');


I'll address these consoles in the code.

gbockus-sf · 2022-02-14T19:30:47Z

...sforcedx-vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionStartExecutor.ts

+  FUNCTION_DEFAULT_PORT
+} from '../types/constants';
+
+export abstract class ForceFunctionStartExecutor extends LibraryCommandletExecutor<


base class extended by the two executors.

gbockus-sf · 2022-02-14T19:31:25Z

...sforcedx-vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionStartExecutor.ts

+    return true;
+  }
+
+  public abstract setupFunctionListeners(


new abstract methods that are implemented by the different flavors of executors.

gbockus-sf · 2022-02-14T19:31:36Z

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionStart/index.ts

@@ -0,0 +1,4 @@
+


need a copyright.

gbockus-sf · 2022-02-14T19:32:08Z

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionStart/index.ts

+
+export {ForceFunctionContainerlessStartExecutor} from './ForceFunctionContainerlessStartExecutor';
+export {ForceFunctionContainerStartExecutor} from './ForceFunctionContainerStartExecutor';
+export { validateStartFunctionsUri } from './validateStartFunctionsUri';


hum...I'm not sure why prettier didn't catch the space inconsistency in these exports.

Don't think we have a rule for this. But for consistency, I see we use space in both exports and imports.

hum...yeah I double checked and prettier should take care of this. I'm not sure how it slipped by the first time.

gbockus-sf · 2022-02-14T19:33:00Z

packages/salesforcedx-vscode-core/src/commands/functions/functionService.ts

@@ -43,6 +52,8 @@ export interface FunctionExecution extends Terminable {
  debugSession?: vscode.DebugSession;
 }

+export const FUNCTION_TYPE_ERROR = 'Unable to determine type of exectucting function.';


@sbudhirajadoc New error message on invalid function type.

typo with "exectucting"

gbockus-sf · 2022-02-14T19:34:15Z

packages/salesforcedx-vscode-core/src/index.ts

@@ -352,7 +353,12 @@ function registerCommands(

  const forceFunctionStartCmd = vscode.commands.registerCommand(
    'sfdx.force.function.start',
-    forceFunctionStart
+    forceFunctionContainerlessStartCommand
+  );


this is where the containerless start function is replacing the previous behavior.

gbockus-sf · 2022-02-14T19:35:38Z

packages/salesforcedx-vscode-core/src/messages/i18n.ts

@@ -143,7 +143,8 @@ export const messages = {
  force_lightning_event_create_text: 'SFDX: Create Aura Event',
  force_lightning_interface_create_text: 'SFDX: Create Aura Interface',
  force_function_create_text: 'SFDX: Create Function',
-  force_function_start_text: 'SFDX: Start Function',
+  force_function_container_start_text: 'SFDX: Start Container Function',
+  force_function_containerless_start_text: 'SFDX: Start Local Function',


DO I need to do anything with the package.nls.ja.json if new values are added here?

Not required. We rely on an external contributor (and until then, it defaults to English).

gbockus-sf · 2022-02-14T19:41:52Z

...e-core/test/vscode-integration/commands/functions/forceFunctionContainerStartCommand.test.ts

+    let sfdxWorkspaceCheckerConstructorStub: SinonStub;
+    let forceFunctionContainerStartExecutorConstructorStub: SinonStub;
+
+    beforeEach(() => {


So I probably spent entirely too much time trying to figure out how to deal with the vscode module when developing unit tests locally. The issue is it doesn't exist locally so you can't mock it (or even run a test that imports code where it gets referenced) when running the tests outside of vscode (the ide).

I found a work around that I don't love, but found it to be the lesser of the two evils of not writing unit tests.
Using proxyquire I can ensure we are stubbing all dependancies. The downside is I'm losing the type checking when calling the function under test (note this impact the typescript compile for the test only).

gbockus-sf · 2022-02-14T19:42:24Z

...e-core/test/vscode-integration/commands/functions/forceFunctionContainerStartCommand.test.ts

+    const fakeUri: any = 'what';
+    let validateStartFunctionsUriStub: SinonStub;
+    let commandletRunStub: SinonStub;
+    let forceFunctionContainerStartCommand: any;


this is our type checking sadness I mentioned below.

gbockus-sf · 2022-02-14T19:44:04Z

...e-core/test/vscode-integration/commands/functions/forceFunctionContainerStartCommand.test.ts

+            validateStartFunctionsUri: validateStartFunctionsUriStub
+          }
+        }
+      ));


So here is where I pull in the exported members from the forceFunctionContainerStartCommand module. This is done here in order to provide the stubbed dependancies to the class under test.

gbockus-sf · 2022-02-14T19:44:33Z

...e-core/test/vscode-integration/commands/functions/forceFunctionContainerStartCommand.test.ts

+      validateStartFunctionsUriStub.returns(undefined);
+      await forceFunctionContainerStartCommand(fakeUri);
+      assert.notCalled(commandletRunStub);
+    });


The tests are nice and easy, but the stubbing is kind of gnarly

gbockus-sf · 2022-02-14T19:45:19Z

...lesforcedx-vscode-core/test/vscode-integration/commands/functions/forceFunctionStart.test.ts

@@ -5,15 +5,15 @@
 * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause


I'll rename this to indicate it's an integration test for the container start flow.

jeffb-sfdc · 2022-02-16T23:31:59Z

...vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerStartExecutor.ts

+
+  public async startFunction(
+    functionName: string,
+    functionDirPath: string


functionDirPath isn't used. Can it be removed?

good call. in the abstract method def the param is optional I can just exclude it (needed for containerless implementation)

jeffb-sfdc · 2022-02-16T23:34:49Z

...vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerStartExecutor.ts

+    if (!this.functionsBinary) {
+      throw new Error('Unable to start function with no binary.');
+    }
+    this.functionsBinary.run(functionName, {}).catch(err => console.log(err));


I'm not a fan of putting multiple statements on a single line. (at the very least) it makes placing break points difficult, and (me personally) I prefer:

this.functionsBinary.run(functionName, {}) .catch(err => console.log(err));

I'd like others on the team to weigh in. Maybe it's just me, but I prefer the calls to be on seperate lines.

me neither! love it. I don't think prettier would let me put it on the next line as is, but I'm totally cool adding the () and {} to the catch handler for exactly the reason you said.

turns out we have a tslint rule for including parans for a single param function. but {} added.

jeffb-sfdc · 2022-02-16T23:36:40Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+export class ForceFunctionContainerlessStartExecutor extends ForceFunctionStartExecutor {
+  public async setupFunctionListeners(
+    functionDirPath: string,
+    functionDisposable: Disposable


functionDirPath and functionDisposable aren't used. Can they be removed?

jeffb-sfdc · 2022-02-16T23:37:32Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+
+  public async buildFunction(
+    functionName: string,
+    functionDirPath: string


functionName and functionDirPath aren't used. Can they be removed?

jeffb-sfdc · 2022-02-16T23:40:11Z

...sforcedx-vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionStartExecutor.ts

+
+  public async run(
+    response: ContinueResponse<string>,
+    progress?: vscode.Progress<{


Is progress used, or can it be removed?

I don't think it can b/c there is an additional optional parameter (increment) that is used. If we remove message then increment will be undefined when run is called.

jeffb-sfdc · 2022-02-16T23:48:26Z

...sforcedx-vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionStartExecutor.ts

+    this.buildFunction(functionName, functionDirPath);
+
+    channelService.appendLine(`Starting ${functionName}`);
+    await this.startFunction(functionName, functionDirPath);


VSC is reporting a warning here that await is't needed. Can it be removed?

👍 need to figure out why vsc wasn't complaining for me

jeffb-sfdc · 2022-02-16T23:52:39Z

packages/salesforcedx-vscode-core/src/commands/functions/functionService.ts

+/**
+ * An enum for the different types of functions.
+ */
+export enum FUNCTION_TYPE {


I think the convention for enums in TS is PascalCase, so:

export enum FunctionType { JavaScript, TypeScript, Java }

Here's what the TypeScript docs say: https://www.typescriptlang.org/docs/handbook/enums.html

packages/salesforcedx-vscode-core/src/commands/functions/functionService.ts

gbockus-sf · 2022-02-17T17:43:40Z

...sforcedx-vscode-core/src/commands/functions/forceFunctionStart/ForceFunctionStartExecutor.ts

+          }
+        }
+      );
+    } catch (error) {


I don't really understand this line. empty catches only lead to sadness 🤷

gbockus-sf · 2022-02-17T18:01:24Z

...e-core/test/vscode-integration/commands/functions/forceFunctionContainerStartCommand.test.ts

+
+const proxyquireStrict = proxyquire.noCallThru();
+
+describe.only('Force Function Start Container Command Unit Tests.', () => {


missed an only 🤦 . fixed.

jeffb-sfdc · 2022-02-23T16:54:36Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

+        telemetryService.sendException(this.UNEXPECTED_ERROR_KEY, err.message);
+        notificationService.showErrorMessage(errorNotificationMessage);
+        channelService.appendLine(errorNotificationMessage);
+        if (err?.message) {


I think the exception here isn't nullable, so the ? isn't needed. Either that, or line 55 should be changed to .catch((err: Error | undefined)

gbockus-sf · 2022-02-23T20:55:17Z

...de-core/src/commands/functions/forceFunctionStart/ForceFunctionContainerlessStartExecutor.ts

    const localRun = new LocalRun(functionLanguage, {
      path: functionDirPath,
      port: FUNCTION_DEFAULT_PORT,
      debugPort: FUNCTION_DEFAULT_DEBUG_PORT
    });

+    const debugType = functionLanguage === functionType.JAVA ? 'java' : 'node';
+    FunctionService.instance.updateFunction(functionDirPath, debugType);


So in the container function flow this happens in an odd place so I missed it for containerless function. This updates the debug type after the function is started (which is required to run the service method for getting the type.

* feat: package update and wiring for containerless funcs * feat: add flow for containerless func start * feat: changes from self review * feat: add copyright to all the new files * feat: add unit test for ForceFunctionContainerStartExecutor * feat: lint fixes * feat: add ForceFunctionContainerlessStartExecutor unit tests * fix: changes from review * fix: update FunctionService unit test * fix: unit test fixes * fix: fix start comment integration test * fix: add log for error that occurs on local function start * fix: changes from review * fix: allow debugging local java functions

gbockus-sf added 2 commits February 14, 2022 12:24

feat: package update and wiring for containerless funcs

0de770c

feat: add flow for containerless func start

08820f9

gbockus-sf commented Feb 14, 2022

View reviewed changes

packages/salesforcedx-vscode-core/src/commands/functions/forceFunctionStart/index.ts

@@ -0,0 +1,4 @@

Copy link

Contributor Author

gbockus-sf Feb 14, 2022

Choose a reason for hiding this comment

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

need a copyright.

gbockus-sf commented Feb 14, 2022

View reviewed changes

gbockus-sf added 3 commits February 14, 2022 15:03

feat: changes from self review

84a1eb8

feat: add copyright to all the new files

6cc1410

feat: merge with develop

777f1df

jeffb-sfdc reviewed Feb 16, 2022

View reviewed changes

packages/salesforcedx-vscode-core/src/commands/functions/functionService.ts Show resolved Hide resolved

gbockus-sf commented Feb 17, 2022

View reviewed changes

gbockus-sf added 6 commits February 17, 2022 12:16

fix: changes from review

6b499eb

Merge branch 'develop' into gbockus/containerless-functions

6ea1a91

fix: update FunctionService unit test

9643d93

fix: unit test fixes

650cb8c

fix: fix start comment integration test

c1cc28d

Merge branch 'develop' into gbockus/containerless-functions

c544e13

gbockus-sf requested a review from jeffb-sfdc February 18, 2022 01:47

gbockus-sf added 2 commits February 23, 2022 00:21

fix: add log for error that occurs on local function start

9f67d48

Merge branch 'develop' into gbockus/containerless-functions

08fab7e

jeffb-sfdc reviewed Feb 23, 2022

View reviewed changes

gbockus-sf added 2 commits February 23, 2022 11:01

fix: changes from review

f34c6f6

fix: allow debugging local java functions

bc5eaf6

gbockus-sf commented Feb 23, 2022

View reviewed changes

Merge branch 'develop' into gbockus/containerless-functions

5b4623e

jeffb-sfdc approved these changes Feb 23, 2022

View reviewed changes

jeffb-sfdc merged commit a4f61b9 into develop Feb 23, 2022

jeffb-sfdc deleted the gbockus/containerless-functions branch February 23, 2022 21:41

jag-j mentioned this pull request Feb 24, 2022

Cherry pick containerless functions #3857

Merged

		@@ -0,0 +1,41 @@
		/*
		* Copyright (c) 2020, salesforce.com, inc.

		@@ -5,15 +5,15 @@
		* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause


		const proxyquireStrict = proxyquire.noCallThru();

		describe.only('Force Function Start Container Command Unit Tests.', () => {

containerless function when running locally #3838

containerless function when running locally #3838

Conversation

gbockus-sf commented Feb 14, 2022 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

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

jeffb-sfdc Feb 16, 2022 • 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

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

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

gbockus-sf commented Feb 14, 2022 •

edited

Loading

jeffb-sfdc Feb 16, 2022 •

edited

Loading