-
Notifications
You must be signed in to change notification settings - Fork 32
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
add worker lambda template that's deployed by AWS CDK #395
Conversation
🦋 Changeset detectedLatest commit: c93dca8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dcb1be1
to
3e99bab
Compare
a94244f
to
1b3150d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! This looks like a generally useful addition 🙇
There's a bit of detail to work through as the template is fairly different from the others; my read is that this is unnecessary divergence but I may be mistaken. I'd recommend looking through the other templates and skuba init
ing the greeter
and lambda-sqs-worker
templates to get a feel for how those are wired up. If you'd like to propose changes to the status quo, I'd like to land those across all templates rather than just this one.
"@types/jest": "^24.0.22", | ||
"@types/node": "10.17.5", | ||
"aws-cdk": "^1.18.0", | ||
"esbuild": "^0.11.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't accept this template while it rolls its own build system. This is at odds with skuba's goals and any built-in template should stick to its commands (i.e. skuba build
). I also don't understand why we're using a bundler for backend code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use it to simplify transpile + packaging depedencies. Not so much for any code bundle optimisation that we can get - though that's a nice side effect. We could do without for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that there's some babel experimentation going on in build cli. Does that mean we can expect to be able to swap out the compile/transpile tool underneath skuba build
in the future or is it going to be just tsc
indefinitely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also curious, would the use of this construct, which does automatic transpilation and bundling with esbuild under the hood be deviating from the vision? It seems it only does one extra step over Serverless framework's packaging (i.e. bundling).
Just curious as the bottom line is I'm happy to swap the bundler out for vanilla transpile and package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean we can expect to be able to swap out the compile/transpile tool underneath
skuba build
in the future or is it going to be justtsc
indefinitely?
I'm envisioning that we leave the door open to defaulting to a different tool if it better meets our needs. Babel gives us more features like macros and custom import paths at compile time, but it isn't the default yet as we haven't really dug into its drawbacks (worse REPL, incomplete TypeScript syntax support, another moving part which will trail TypeScript to some degree).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the issue here. In other templates we simply compile to an output directory (i.e. lib
) then include that and node_modules
in the final image/zip. Are we using a bundler here to work around aws/aws-cdk#984?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice my team tends to have a few types of workers (that implement functional and "non-functional" requirements) within the same domain, and the worker types of that domain live in the same repository. So we have multiple points of entry to package for.
Using a bundler isn't a workaround, as we probably aren't blocked by the issue without one (TBD - I haven't explored this fully). However, until AWS CDK supplies a packaging feature, using a bundler eliminates the need to roll our own packaging script.
Could esbuild be added as a non-default build tool instead? I'm aware it's stretching (maybe even breaking) the current definition by virtue of it being a bundler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template doesn't demonstrate multiple workers / monorepoing though, and it's not clear to me how esbuild
enables that use case. Lambda function entry points are configured separate from their bundles and there are plenty of SEEK repos that implement the configuration you describe with the sensible defaults of tsc
and Serverless.
I don't think it makes sense for skuba build
to proxy esbuild
unless it enables a broadly useful scenario that can't be accomplished with tsc
or babel
today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed esbuild
and replaced it with basic packaging.
@@ -169,6 +169,14 @@ This initialises a new directory and Git repository. | |||
a message queue is employed between the source topic and the Lambda function, | |||
and unprocessed events are sent to a dead-letter queue for manual triage. | |||
|
|||
- `lambda-sqs-worker-cdk` | |||
|
|||
An asynchronous [worker] built on [AWS Lambda] and deployed with [AWS CDK]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem in line with the Technology Strategy: https://tech-strategy.ssod.skinfra.xyz/docs/v1/technology.html#lambda
I don't mind merging this, but can we kick off internal discussions over the merit of the CDK as a Lambda deployment tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a misunderstanding on my part:
https://tech-strategy.ssod.skinfra.xyz/docs/v1/components.html#infrastructure
I read it to include Lambda workers. We're already using CDK to deploy workers in practice (among other things).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @72636c is referring to:
Lambda functions SHOULD be deployed using Serverless
This also repeated under the Worker section:
SHOULD deploy using Serverless
And the Serverless section:
Serverless is our current default choice for packaging and deploying AWS Lambda functions.
Tech Strategy allowing CDK for infrastructure wouldn’t override the above guidance.
If your team has decided to deviate from SEEK’s Tech Strategy you could BYO GitHub template instead of including it in Skuba’s default templates.
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
To avoid naming collisions with built-in JavaScript and Typescript constructs
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
Was set by `cdk init` and overlooked
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
Sorry, couldn't suggest this change in the GitHub UI.
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay here, it's looking good. I just have a few outstanding questions.
@@ -0,0 +1,15 @@ | |||
#!/usr/bin/env node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the shebang do anything given this is TypeScript source code and there's a script to run it through ts-node
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's redundant, so I've removed it.
"format": "skuba format", | ||
"lint": "skuba lint", | ||
"test": "skuba test", | ||
"package": "yarn build && yarn --prod --modules-folder ./lib/node_modules", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a local operation or does Yarn have to fetch dependencies over the network? Ideally we'd try to avoid additional installation at this point so our docker-ecr-cache
actually pays off and to limit exposure of the npm token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used to build the docker image, and also doubles as a local operation.
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legend! Thanks for your perseverance here 🙇
Why?
Reduce the setup costs for AWS CDK deployed workers.