Remove hardcoded NODEJS runtime

pocesar commented 4 years ago

Minor nitpick, it could fallback to NODEJS_10_X in case process.env['NODE_RUNTIME'] isn't set, because functionProps doesn't allow override for that https://github.com/punchcard/punchcard/blob/d0b6fe43d5e9ef5994c96929b28405bf7ca6cc45/packages/punchcard/lib/lambda/function.ts#L86-L90

Another way would to put runtime property before the functionProps spread (which is much easier and concise)

sam-goodwin commented 4 years ago

This is a good idea. I agree that we should support customizability here. is NODE_RUNTIME an environment variable you came up with? Perhaps we could default to the version that you run cdk synth with? I think a bare minimum of supporting it as a parameter is necessary.

pocesar commented 4 years ago

yeah, there's no defacto var name for setting the runtime version (unlike NODE_ENV) afaik the env var is easier to use than having to change code, but just changing the order of the spread operator is enough for most use cases I guess.

 const lambdaFunction = new lambda.Function(scope, id, { 
   ...functionProps, 
   code: Code.tryGetCode(scope) || Code.mock, 
   runtime: lambda.Runtime.NODEJS_10_X, 
   handler: 'index.handler',

to

 const lambdaFunction = new lambda.Function(scope, id, { 
   runtime: lambda.Runtime.NODEJS_10_X, 
   ...functionProps, 
   code: Code.tryGetCode(scope) || Code.mock, 
   handler: 'index.handler',

sam-goodwin / punchcard

Remove hardcoded NODEJS runtime #56