feat: Func plugin to add/remove functions within function app #151

tbarlow12 · 2019-05-28T16:59:23Z

Adds CLI commands sls func add -n {functionName} and sls func remove -n {functionName}
Adds 1 plugin with 3 hooks: (func, func:add, func:remove)
Generates code directory and updates serverless.yml appropriately
For now, only supports http functions. Working on supporting additional triggers
Still working on tests, documentation and edge cases

tbarlow12 · 2019-05-29T17:42:50Z

src/index.ts

-import { AzureLoginPlugin } from './plugins/login/loginPlugin';
-import { AzureApimServicePlugin } from './plugins/apim/apimServicePlugin';
-import { AzureApimFunctionPlugin } from './plugins/apim/apimFunctionPlugin';
+import Serverless from "serverless";


Updated to use double quotes

wbreza

Added some feedback. Let's discuss.

wbreza · 2019-05-29T21:47:13Z

src/plugins/func/funcUtils.ts

+  }
+
+  public static getFunctionHandler(name: string) {
+    return `"use strict";


I would move this inline script into a separate template file. We can then use JS string interpolation (similar to vott resources) to include any dynamic string content.

So I've spent some time trying to make this work. My problem has been what kind of file to put the template in. Here are the options I've tried:

Template file options:

TypeScript file

Gets transpiled to JavaScript in build - doesn't keep template structure

JavaScript file

Can't be included in build

Tried setting 'allowJs' to be true, but that conflicts with 'declarations' being true

Text file

Could make it work, but it would mean an extra step post-build of copying the text file over. Not sure that's warranted just to avoid the inline script

Other options?

Yah, the ideal solution would be to be able to include a js file of the handler since that's technically what we're packaging, but if that isn't really an option, then inline seems reasonable.

Let's add a work item to re-review this later.

src/plugins/func/remove/azureFuncRemove.ts

src/plugins/func/azureFunc.ts

src/plugins/func/funcUtils.ts

mydiemho

not sure where it should go and if it's out of scope of this pr, but we need some tests to enforce the right file structure for function zip folder.

README.md

mydiemho · 2019-05-30T16:54:24Z

src/plugins/func/azureFunc.test.ts

+      expect(sls.cli.log).toBeCalledWith("Need to provide a name of function to remove")
+    });
+
+    it("returns with pre-existing function", async () => {


copy/pasta: should be returns with non-existing function

src/plugins/func/azureFunc.ts

mydiemho · 2019-05-30T17:04:02Z

src/plugins/func/azureFunc.ts

+
+  private createFunctionDir(name: string) {
+    this.serverless.cli.log("Creating function dir");
+    fs.mkdirSync(name);


should we worry about error here? in cases of permission issues?

I'll put a try catch here

mydiemho · 2019-05-30T17:09:54Z

src/plugins/func/funcUtils.ts

+
+  private static defaultFunctionSlsObject(name: string) {
+    return {
+      handler: `${name}/index.handler`,


we don't want ${name}/index.handler, this will generate incorrect binding and function won't work.

just index.handler

This is the structure that serverless.yml needs to find the handler since it will live inside the directory {functionName and inside the file index.js, with the function name handler

yes but the plugin will generate incorrect scriptFilePath since function.json now resides at the same level as index.json

mydiemho · 2019-05-30T17:10:47Z

src/test/mockFactory.ts

+
+  public static createTestFunctionMetadata(name: string) {
+    return {
+      "handler": `${name}/index.handler`,


same as above no ${name}

Same as above

same as above :p

src/plugins/func/azureFunc.ts

mydiemho · 2019-05-30T22:11:20Z

src/plugins/func/funcUtils.ts

+
+  private static defaultFunctionSlsObject(name: string) {
+    return {
+      handler: `${name}/index.handler`,


yes but the plugin will generate incorrect scriptFilePath since function.json now resides at the same level as index.json

mydiemho · 2019-05-30T22:11:35Z

src/test/mockFactory.ts

+
+  public static createTestFunctionMetadata(name: string) {
+    return {
+      "handler": `${name}/index.handler`,


same as above :p

mydiemho

lgtm! only have one question about a use of string interpolation.

README.md

mydiemho · 2019-05-31T16:20:40Z

src/plugins/func/funcUtils.ts

+
+  private static defaultFunctionSlsObject(name: string) {
+    return {
+      handler: `index.handler`,


Nit: can change back to double quote since we're no longer interpolating a variable

mydiemho · 2019-05-31T16:22:20Z

src/shared/utils.ts

+    const template = sls.utils.readFileSync(path);
+    const names = params.keys();
+    const vals = params.values();
+    return new Function(...names, `return \`${template}\`;`)(...vals);


Q: can

`return \`${template}\`;`

be replace with

`return ${template};`

This is actually a string interpolation that won't happen here. We actually have the same function (kind of) in VoTT. This is expecting a file with templated variables. I'm actually not using this right now... I was using it when I had a javascript template file. But I might keep it in just in case we go back to that

PIC123

I just had a small question but looks good!

PIC123 · 2019-05-31T22:18:03Z

src/plugins/func/funcUtils.ts

+  }
+
+  public static getFunctionHandler(name: string) {
+    return `"use strict";


Yah, the ideal solution would be to be able to include a js file of the handler since that's technically what we're packaging, but if that isn't really an option, then inline seems reasonable.

PIC123 · 2019-05-31T22:41:32Z

src/plugins/func/azureFunc.test.ts

+
+    it("returns with missing name", async () => {
+      const sls = MockFactory.createTestServerless();
+      const options = MockFactory.createTestServerlessOptions();


Q: Would it make sense to add a default name to the options and then remove/override as needed? Just thinking if we'll need it for other function tests if we add more functionality, like the option to include the extension bundles

For sure. Let's add that in another PR

wbreza

Thanks for addressing earlier feedback.

wbreza · 2019-05-31T22:44:31Z

src/plugins/func/funcUtils.ts

+  }
+
+  public static getFunctionHandler(name: string) {
+    return `"use strict";


Let's add a work item to re-review this later.

tbarlow12 added 10 commits May 28, 2019 07:13

Added stubs for add and remove func plugins

287c478

Removing from serverless yml and function directory

da5d06c

Generating scaffolding code with add function

544accf

Mocking file system

7e22553

Stub tests for func utils

6882d73

Added tests for add and remove plugins

4b01f4c

Remove sample data

ffebc30

Add docs

6a74d86

Remove junk files from test

552fce2

Update message in func plugin

d32aef2

tbarlow12 changed the title ~~WIP - feat: Func plugin to add/remove plugins within function app~~ feat: Func plugin to add/remove plugins within function app May 29, 2019

Removed jest functions and instead added spies

b518cd8

tbarlow12 requested review from wbreza, mydiemho and PIC123 May 29, 2019 17:36

Update to double quotes

acc4fa1

tbarlow12 commented May 29, 2019

View reviewed changes

tbarlow12 added 2 commits May 29, 2019 10:51

Remove unused import

c3a5c33

Update with changes from dev

2bd21b2

wbreza suggested changes May 29, 2019

View reviewed changes

tbarlow12 added 10 commits May 29, 2019 15:57

Update func utils and tests to not use regex

c60fc50

Fix bug in altering serverless yml

490e983

Clearing all mocks after each test to avoid conflicts

ef2ee1b

Added template file for generated javascript

4f59c32

Create cwd

9f1787c

Run tests in band, add mocked file to func plugin tests

4d497b8

Changed test to not spy on writeFileSync

8b339eb

Using inline handler script and templates directory

dc5afbd

Update func util to use sls util read file

4245bb0

Remove text from tsconfig include

e5bf4da

mydiemho suggested changes May 30, 2019

View reviewed changes

Respond to PR feedback

136c97a

mydiemho suggested changes May 30, 2019

View reviewed changes

mydiemho approved these changes May 31, 2019

View reviewed changes

Update string to not use template string

85d8cca

tbarlow12 changed the title ~~feat: Func plugin to add/remove plugins within function app~~ feat: Func plugin to add/remove functions within function app May 31, 2019

PIC123 approved these changes May 31, 2019

View reviewed changes

wbreza approved these changes May 31, 2019

View reviewed changes

Include updates from dev branch

49a0e2a

tbarlow12 merged commit 3639b0f into dev May 31, 2019

tbarlow12 deleted the tabarlow/func-plugin branch June 7, 2019 17:27

tbarlow12 added a commit that referenced this pull request Sep 13, 2019

feat: Func plugin to add/remove functions within function app (#151)

8e44411

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Func plugin to add/remove functions within function app #151

feat: Func plugin to add/remove functions within function app #151

tbarlow12 commented May 28, 2019 •

edited

Loading

tbarlow12 May 29, 2019

wbreza left a comment

wbreza May 29, 2019

tbarlow12 May 30, 2019

PIC123 May 31, 2019

wbreza May 31, 2019

tbarlow12 May 31, 2019

mydiemho left a comment

mydiemho May 30, 2019

mydiemho May 30, 2019

tbarlow12 May 30, 2019

mydiemho May 30, 2019

tbarlow12 May 30, 2019

mydiemho May 30, 2019

mydiemho May 30, 2019

tbarlow12 May 30, 2019

mydiemho May 30, 2019

mydiemho May 30, 2019

mydiemho May 30, 2019

mydiemho left a comment

mydiemho May 31, 2019

mydiemho May 31, 2019

tbarlow12 May 31, 2019

PIC123 left a comment

PIC123 May 31, 2019

PIC123 May 31, 2019

tbarlow12 May 31, 2019

wbreza left a comment

wbreza May 31, 2019

feat: Func plugin to add/remove functions within function app #151

feat: Func plugin to add/remove functions within function app #151

Conversation

tbarlow12 commented May 28, 2019 • edited Loading

Choose a reason for hiding this comment

wbreza 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

mydiemho 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

mydiemho 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

PIC123 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

wbreza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbarlow12 commented May 28, 2019 •

edited

Loading