Make serverless/function into a subclass of lambda/function. #202

CyrusNajmabadi · 2018-04-30T21:02:03Z

No description provided.

CyrusNajmabadi · 2018-04-30T21:02:24Z

Note: this is not ready to go in. This is to start the discussion with @lukehoban

CyrusNajmabadi · 2018-04-30T21:03:02Z

overlays/nodejs/serverless/function.ts

-            let attachment = new RolePolicyAttachment(`${name}-${sha1hash(policy)}`, {
-                role: this.role,
-                policyArn: policy,
-            }, { parent: this });


note: parent:this was lost in my translation. the lambda-function needs the role. so it's unclear to me how to accomplish this.

… its own location.

CyrusNajmabadi · 2018-05-01T19:57:38Z

@lukehoban can you take a look?

lukehoban · 2018-05-02T00:41:24Z

overlays/nodejs/lambda/createFunction.ts

+    }
+    if (!args.role) {
+        throw new Error("Missing 'role' in 'args'");
+    }


This argument validation is taken care of in aws.lambda.Function constructor, soprobably not needed here since we don't use it in this context.

That said - I wonder if we do want to validate that code, handler and any other properties that conflict with these are not passed, since we'll be overwriting those.

yes, that's a good idea. Note: i front loaded the role check because i like early errors. it makes it clear it's the problem of the person who calls us, not a mistake on our end.

lukehoban · 2018-05-02T00:42:07Z

overlays/nodejs/lambda/createFunction.ts

+    serialize = serialize || (_ => true);
+    const finalSerialize = (o: any) => {
+        return serialize(o) && o !== this;
+    }


Separate from this change - I'd still love to get rid of this serialize thing alltogether. I believe we think it is not actually needed anymore?

It's been mitigated... but i think it may still be necessary. effectively, it depends on the asynchrony in user code. If we 'pump' while serializing, it's possible for us to observe loops form, which can then cause deadlocks.

I'll see about removing in a post PR.

lukehoban · 2018-05-02T00:44:39Z

overlays/nodejs/lambda/createFunction.ts

+    const argsCopy = {
+        ...args,
+        runtime: args.runtime || runtimes.NodeJS8d10Runtime,
+        timeout: args.timeout === undefined ? 180 : args.timeout,


We can probably remove this line here - and just provide this 180 default via the existing aws.serverless.Function (and possibly in the future some equivalent layer in @pulumi/cloud). Feels like we should only "change" things that we absolutely must to enable closure serialization to be used.

sounds good.

lukehoban · 2018-05-02T00:46:26Z

overlays/nodejs/serverless/function.ts

            role: this.role.arn,
            timeout: options.timeout === undefined ? 180 : options.timeout,
            memorySize: options.memorySize,
            deadLetterConfig: options.deadLetterConfig,
            vpcConfig: options.vpcConfig,
-        }, { parent: this });
+            // createFunction will actually supply the handler.  This is just to satisfy typescript.
+            handler: undefined!,


Doesn't this mean that any user of createFunction will have to do something awkward like this at it's callsite? That seems unfortunate?

I don't suppose there is any way to create a type that is another type but without a particular set of fields? :-)

Alternatively, we could force handler to be not be marked as required in resources.go. That somewhat weakens type checking for cases where you are calling aws.lambda.Function() directly, but may be worth it?

I don't suppose there is any way to create a type that is another type but without a particular set of fields? :-)

I believe so :) I think typescript added new type-operating functions for precisely thsi purpose. let me look into it.

lukehoban · 2018-05-02T00:48:31Z

overlays/nodejs/serverless/function.ts

-        }, { parent: this });
+            // createFunction will actually supply the handler.  This is just to satisfy typescript.
+            handler: undefined!,
+            runtime: options.runtime || lambda.NodeJS6d10Runtime,


Why are we overriding this to 6.10 here?

this is hte logic that was already here. I'm not sure our reasoning. @swgillespie ?

CyrusNajmabadi · 2018-05-02T02:14:31Z

overlays/nodejs/utils.ts

+    [K in O]: (Record<D, never> & Record<string, K>)[K]
+}[O]
+
+export type Omit<O, D extends string> = Pick<O, Sub<keyof O, D>>


typescript be crazy!

i had to look at this for a few minutes to wrap my head around what was going on.

CyrusNajmabadi · 2018-05-02T02:15:11Z

@lukehoban can you takea nother look?

CyrusNajmabadi · 2018-05-02T02:29:07Z

overlays/nodejs/utils.ts

+
+export type Sub<O extends string, D extends string> = {
+    [K in O]: (Record<D, never> & Record<string, K>)[K]
+}[O]


this is the weirdest part for me (specifically the [O] part). I've never used the indexed access operator in creating a type before.

lukehoban

Looks good to me. Longterm, I can imagine perhaps wanting to fold createFunction into the existing new aws.lambda.Function, with a single unified bag of args, but this seems a very pragmatic way to expose creation of a raw callback-based Lambda for now.

lukehoban · 2018-05-02T03:52:24Z

overlays/nodejs/lambda/createFunction.ts

+
+// User sholud not supply 'code' or 'handler' to createFunction.  We will create those
+// out of the handler they pass in.
+export type CallbackFunctionArgs = utils.Omit<lambda.FunctionArgs, "code" | "handler">


Since this is public - lets put a doc comment and phrase this as user facing documentation.

lukehoban · 2018-05-02T04:40:55Z

overlays/nodejs/serverless/function.ts

            role: this.role.arn,
            timeout: options.timeout === undefined ? 180 : options.timeout,
            memorySize: options.memorySize,
            deadLetterConfig: options.deadLetterConfig,
            vpcConfig: options.vpcConfig,
-        }, { parent: this });
+            runtime: options.runtime || lambda.NodeJS6d10Runtime,


This code used to use 8.10, and it seems you are changing it back to use 6.10. Any reason why?

The change to 8.10 was made a couple weeks ago in #194.

lukehoban · 2018-05-02T04:42:34Z

overlays/nodejs/lambda/createFunction.ts

+    }
+    if ((<any>args).handler) {
+        throw new Error("'handler' property should not be provided in 'args'");
+    }


Given that these properties are no longer on the type, seems fine to not check for them not being passed.

I'd like to keep this in. People may call from JS and they may easily get confused and think they need these due to the docs for lambda.Function. This is a pretty simple check to have, and it could prevent confusion.

lukehoban · 2018-05-02T04:44:48Z

overlays/nodejs/utils.ts

+// Helpers to create 'subtraction' types.  i.e. a type equivalent to some other object type,
+// just with some members removed.
+
+export type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>


:-| I have no idea how that works, but nice that this is possible.

Are there any downsides to this? I assume error messages are still reasonable when these types are involved?

Yes. the error messages will still be of the form "you need this property 'x'"

lukehoban · 2018-05-02T04:47:10Z

overlays/nodejs/lambda/createFunction.ts

+export type CallbackFunctionArgs = utils.Omit<lambda.FunctionArgs, "code" | "handler">
+
+/**
+ * Helper to create an aws lambda function out of an actual javascript callback.


Nit: Capitalization of all proper nouns here, also maybe phrase a little more explicitly as a first class part of the API.

Make serverless/function into a subclass of lambda/function.

458354f

CyrusNajmabadi requested a review from lukehoban April 30, 2018 21:02

CyrusNajmabadi commented Apr 30, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into functionSubclass

73b0ee0

CyrusNajmabadi mentioned this pull request Apr 30, 2018

Make 'Function' a generic type #203

Merged

Cyrus Najmabadi added 5 commits May 1, 2018 12:23

More func work

18968d4

Extract code that actually creates an aws lambda out of callback into…

cbc368a

… its own location.

Update comments.

fb3cf79

restore code.

9f99d5e

restore code.

237d01e

Cyrus Najmabadi added 3 commits May 1, 2018 16:41

IMprove overlay.

48e7a4f

Rmeove unnecessary code.

2c30229

Use simpler helper.

8ef72c6

lukehoban reviewed May 2, 2018

View reviewed changes

CyrusNajmabadi commented May 2, 2018

View reviewed changes

lukehoban approved these changes May 2, 2018

View reviewed changes

Cyrus Najmabadi added 4 commits May 2, 2018 13:26

Update docs.

b3a41f1

Update docs.

7cab16c

Pick the right runtime.

94857bc

Don't require so high a TS version.

938abf9

CyrusNajmabadi merged commit 85ae2b7 into master May 2, 2018

CyrusNajmabadi deleted the functionSubclass branch May 2, 2018 21:41

lukehoban mentioned this pull request May 9, 2018

Updates for 0.12.1 pulumi/docs#254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make serverless/function into a subclass of lambda/function. #202

Make serverless/function into a subclass of lambda/function. #202

CyrusNajmabadi commented Apr 30, 2018

CyrusNajmabadi commented Apr 30, 2018

CyrusNajmabadi Apr 30, 2018

CyrusNajmabadi commented May 1, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

CyrusNajmabadi May 2, 2018

CyrusNajmabadi May 2, 2018

CyrusNajmabadi commented May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban left a comment

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

CyrusNajmabadi May 2, 2018

lukehoban May 2, 2018

Make serverless/function into a subclass of lambda/function. #202

Make serverless/function into a subclass of lambda/function. #202

Conversation

CyrusNajmabadi commented Apr 30, 2018

CyrusNajmabadi commented Apr 30, 2018

Choose a reason for hiding this comment

CyrusNajmabadi commented May 1, 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

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

CyrusNajmabadi commented May 2, 2018

Choose a reason for hiding this comment

lukehoban 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