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

feat(aws-lambda): AWS Lambda layer target #160

Merged
merged 27 commits into from
Jan 14, 2021
Merged

Conversation

iker-barriocanal
Copy link
Contributor

Add a target to publish a layer and sets its permissions in each region for the JS SDK.

Check and get environment variables `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY`,
before publishing to AWS Lambda.
@iker-barriocanal iker-barriocanal changed the title AWS Lambda target for JS SDK feat(aws-lambda): AWS Lambda target for JS SDK Jan 7, 2021
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
@iker-barriocanal iker-barriocanal marked this pull request as ready for review January 11, 2021 13:49
@tonyo tonyo self-requested a review January 11, 2021 16:26
Copy link
Contributor

@tonyo tonyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor comments.

Separate 👍 👍 for adding tests for the target!

Please also add the documentation for this new target to README.

.craft.yml Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/__tests__/awsLambda.test.ts Outdated Show resolved Hide resolved
src/targets/__tests__/awsLambda.test.ts Outdated Show resolved Hide resolved
src/targets/__tests__/awsLambda.test.ts Outdated Show resolved Hide resolved
src/targets/__tests__/awsLambda.test.ts Show resolved Hide resolved
- Delete `aws-lambda` target from `.craft.yml`.
- Include start of line anchor (`^`) in the AWS Lambda regex.
- Return promise instead of awaiting it inside the aws publish and add layer permissions methods.
- Delete `TestArtifactProvider` and use `NoneArtifactProvider` instead.
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff @iker-barriocanal

Dropped some comments to help move this to completion. Let's chat if I can help.

High level notes:

  • Need to split clearly what is part of project config and what is part of the Craft implementation.
  • Missing some documentation on how to use this new target in a craft-powered project (like sentry-javascript and sentry-python), essentially documenting what is the required config.

.craft.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/awsLambda.ts Outdated Show resolved Hide resolved
src/targets/__tests__/awsLambda.test.ts Outdated Show resolved Hide resolved
- AWS SDK upgrade from v2 to v3.
- AWS regions are now fetched using the EC2 client.
- A license is now required in the project config.
- Update tests to support SDKv3 and region fetch.
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/artifact_providers/base.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
// returned. Thus, both alternatives have been considered.
expect(publishedLayerVersion).toBe(undefined);
} catch (error) {
expect(error instanceof Error).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm: wouldn't this always be true?

If you want to confirm that a function throws an error, please consider using https://jestjs.io/docs/en/expect.html#tothrowerror

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code inside catch, yes. publish will only throw if it's not running in dry-run mode; if it is, it doesn't throw anything and returns undefined. So, the approach is to run publish and expect to get undefined; if it throws an error before that, catch it. The method toThrow, afaik, requires to make the function call inside the expect, passing the test when the dry-run mode is off but failing when it's on.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I think we can go ahead and update sentry-python and sentry-javascript to use this and make any adjustments later if need be. Good job @iker-barriocanal!

README.md Show resolved Hide resolved
Comment on lines +914 to +921
targets:
- name: aws-lambda-layer
includeNames: /^sentry-node-serverless-\d+(\.\d+)*\.zip$/
layerName: SentryNodeServerlessSDK
compatibleRuntimes:
- nodejs10.x
- nodejs12.x
license: MIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/targets/awsLambdaLayer.ts Outdated Show resolved Hide resolved
src/targets/awsLambdaLayer.ts Show resolved Hide resolved
src/targets/awsLambdaLayer.ts Outdated Show resolved Hide resolved
@rhcarvalho rhcarvalho requested a review from tonyo January 14, 2021 14:06
@rhcarvalho
Copy link
Contributor

Re-requesting Anton if he still has any concerns from the points raised earlier.

@tonyo
Copy link
Contributor

tonyo commented Jan 14, 2021

@rhcarvalho Let's give Iker a break 😅 I've already left one a few minutes ago.

@rhcarvalho
Copy link
Contributor

Oh sorry, I did not see it before I posted.

@iker-barriocanal iker-barriocanal changed the title feat(aws-lambda): AWS Lambda target for JS SDK feat(aws-lambda): AWS Lambda layer target Jan 14, 2021
@iker-barriocanal iker-barriocanal merged commit 2e3a53e into master Jan 14, 2021
@iker-barriocanal iker-barriocanal deleted the target-awslambda branch January 14, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants