feat(lambda): allow running a build file

[bergjaak]

Issue # (if applicable)

Closes #18470

Reason for this change

This allows customers to execute an arbitrary build script as part of cdk synth, which will enable customer to use esbuild plugins. The rationale for this decision is given the issue that is linked above.

Description of changes

1. Expose the code field on the aws-lambda-nodejs construct, so that customers can specify code in ways other than bundling, which was the default and abstracted away from customers before this change.
2. Add a new static method on Code, namely Code.fromCustomCommand. This method takes in the commands to run an arbitrary script during cdk synthesis that the customer provides. The customer also provides the location of the output from the buildscript. Then this output is supplied to a lambda function.

Description of how you validated changes

manual testing (involving inspecting output in the AWS Lambda console and invoking the function), integration tests, and full unit test coverage of new changes.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[colifran]

Can you help me understand why we need this new code block here? Why can't we just determine the handler in the same way we are doing it right now? Couldn't we just keep the existing code and do something where if code is supplied we use it, otherwise we do Bundling.bundle? So something like:

  // Entry and defaults
  const entry = path.resolve(findEntry(id, props.entry));
  const architecture = props.architecture ?? Architecture.X86_64;
  const depsLockFilePath = findLockFile(props.depsLockFilePath);
  const projectRoot = props.projectRoot ?? path.dirname(depsLockFilePath);
  const handler = props.handler ?? 'handler';

  super(scope, id, {
    ...props,
    runtime,
    code: props.code ?? Bundling.bundle(scope, {
      ...props.bundling ?? {},
      entry,
      runtime,
      architecture,
      depsLockFilePath,
      projectRoot,
    }),
    handler: handler.indexOf('.') !== -1 ? `${handler}` : `index.${handler}`,
  });
}

[bergjaak]

That's what I originally had, but I think it's a worse customer experience. Because there's nothing forcing the customer to name the file containing the exported handler index. It's entirely up to the customer to decide how they want to name that file, if they're using the Code.fromCustomCommand. Therefore, I think this is a better customer experience, even if it's a few more lines of code

[colifran]

I see where you're coming from but I'm not sure I agree. I think this construct has already established that the expected behavior is we will default the handler if it isn't supplied. Consider a customer that is using the nodejs function and they currently don't supply a handler and just use the default. Now, once this feature is released, they decide to use the new code property to run a custom build script. However, suddenly they're encountering errors since they don't set a handler. I recognize that it is trivial to just specify a handler, but it is a change from the expectation for the construct as a whole. Additionally, we generally try to avoid properties where we have to do a bunch of validation around if a property is set then another one must also be set. We consider it a bit of an anti-pattern. Our VPC construct is a more extreme example of this, but it shows where this can get to be hard to deal with when we have to start doing a ton of property validation - https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts

[bergjaak]

Now, once this feature is released, they decide to use the new code property to run a custom build script. However, suddenly they're encountering errors since they don't set a handler.

My understanding is, if a customer does switch to using Code.fromCustomCommand and wants an error-free transition with default handler values, then their build script will need to create a file (either in a directory or a zip file) named index.js with an exported function called handler. Defaulting to handler makes sense in this case since it's unlikely that the customer would have changed the name of the function. However, if the customer never knew that the file that contained their lambda code, when deployed to AWS Lambda, was called index.js (since that's abstracted away by this construct), then wouldn't the customer be confused when their lambda fails to work because their build file didn't output the handler in a file called index.js? That seems more confusing to me. By throwing the Error here, we push the error that the customer may run into much earlier, rather than the customer's lambda failing at runtime after deploying successfully -- which could cause an outage if a customer is deploying directly to production.

[bergjaak]

basically, to avoid having an outage in production, if a customer is switching to using this Code.fromCustomCommand, the customer would need to know that their build file must output a file called index.js that exports a function called handler. That's not something that I think should be hidden from the customer. By requiring handler, we don't abstract away a detail that could potentially lead to an outage

[bergjaak]

I agree that this is more complicated, but the alternative is to push this complexity onto the customer after they've deployed their lambda

[colifran]

Happy to change my mind on this, and with the way you've explained it I agree. Doesn't make sense to let the customer fail during deploy time.

[colifran]

leftover?

[bergjaak]

yeah, I'll remove this in my final revision

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[colifran]

I generally prefer to use interfaces where we can so that if there is ever a chance that a new argument is introduced we can just add it to the interface. What do you think about creating a new interface that just extends AssetOptions? We could call it CustomCommandOptions. Something like this:

export interface CustomCommandOptions extends AssetOptions {
	readonly commands?: { [options: string]: any },
}

Then the function definition could just be:

public static fromCustomCommand(output: string, command: string[], options: CustomCommandOptions = {})

[bergjaak]

I agree with you, that's an improvement

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 1e59d4a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository