Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lambda): erroneous inline code support for ruby #6365

Merged
merged 3 commits into from
Feb 20, 2020

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Feb 19, 2020

Commit Message

fix(lambda): erroneous inline code support for ruby (#6365)

See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#cfn-lambda-function-code-zipfile

fixes #6302


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

@nija-at nija-at added the contribution/core This is a PR that came from AWS. label Feb 19, 2020
@nija-at nija-at self-assigned this Feb 19, 2020
@nija-at nija-at changed the title fix(lambda): no inline code support for Ruby fix(lambda): no inline code support for ruby Feb 19, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2020

In the CHANGELOG this will read as:

FIXES

  • lambda: no inline code support for ruby

Which might be interpreted as "there used to be no inline support for Ruby but we fixed that". I'd prefer a clearer phrasing.

Maybe "Ruby erroneously advertises inline code support"

@iliapolo
Copy link
Contributor

@nija-at Can we add some kind of test?

https://github.com/aws/aws-cdk/pull/6365/checks?check_run_id=455627583

I would imagine the following code should fail on instantiation, right?

new lambda.Function(stack, 'foobar', {
  code: new lambda.InlineCode('def handler(event:, context:) ; "success" ; end'),
  handler: 'index.handler',
  timeout: cdk.Duration.seconds(15),
  runtime: lambda.Runtime.RUBY_2_5
});

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: a4cb7a4
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@nija-at nija-at changed the title fix(lambda): no inline code support for ruby fix(lambda): erroneous inline code support for ruby Feb 19, 2020
@nija-at
Copy link
Contributor Author

nija-at commented Feb 19, 2020

I would imagine the following code should fail on instantiation, right?

Yes, this would previously fail on cdk deploy but would now fail earlier. No one should have a working CDK code with ruby and InlineCode.

can we add some kind of test?

There's no 'useful' test I can think of. Suggestions?

@iliapolo
Copy link
Contributor

There's no 'useful' test I can think of. Suggestions?

I guess that depends where exactly the code will fail. But my suggestion is to add a validation in the lambda.Function constructor:

if (props.code instanceof InlineCode && !props.runtime.supportsInlineCode) {
  throw new ...
}

And then simply add a test to test.function.ts.

WDYT?

@iliapolo
Copy link
Contributor

@nija-at I guess an even simpler test would be:

const rubyRuntime = lambda.Runtime.RUBY_2_5;
assert !rubyRuntime.supportsInlineCode 

That is assuming we have a test further down the line that validates we fail when defining inline code with an unsupported runtime.

@nija-at
Copy link
Contributor Author

nija-at commented Feb 19, 2020

@iliapolo - sorry, but I don't find these to be useful tests. They are not testing any cyclomatic complexity, which is what unit tests should cover.
How would such a test have prevented this in the first place? The person who did this erroneously, would also erroneously add the test.

The only valuable test would be an integration test that attempts to deploy a stack and checks that the deployment succeeds. Unfortunately, we don't have a mechanism in place to do this during PR or pipeline builds.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 854d6ae
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 14a0ab4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 8e21e78 into master Feb 20, 2020
@mergify mergify bot deleted the nija-at/lambda-inlinecode-ruby branch February 20, 2020 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lambda Ruby Runtime doesn't Support ZipFile
4 participants