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

docs(examples): add example for AWS SAM #674

Merged
merged 26 commits into from
May 7, 2022

Conversation

bpauwels
Copy link
Contributor

Description of your changes

Added an example for AWS SAM

How to verify this change

see new SAM example, use SAM CLI to deploy it

Related issues, RFCs

#606

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

no breaking changes


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi changed the title Added example for AWS SAM docs(examples): add example for AWS SAM Mar 18, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Mar 18, 2022
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 18, 2022
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Hello @bpauwels thanks a lot for opening this PR. I've made a first round of review in which I've left some comments. To avoid repeating I've left the comments only in one function but they apply to others as well.

examples/sam/template.yaml Show resolved Hide resolved
examples/sam/src/handlers/tsconfig.json Outdated Show resolved Hide resolved
examples/sam/src/handlers/put-item.ts Outdated Show resolved Hide resolved
examples/sam/src/handlers/put-item.ts Outdated Show resolved Hide resolved
examples/sam/src/handlers/package.json Outdated Show resolved Hide resolved
examples/sam/src/handlers/package.json Outdated Show resolved Hide resolved
@ijemmy
Copy link
Contributor

ijemmy commented Mar 30, 2022

I'm concerned about the long-term maintenance of Lambda functions for different tools examples/<tools>.

Right now, we may have only SAM and CDK. But we'll eventually end up with probably 5+ tools. If we add/modify a feature, we'll need to do it 5+ times. The person who updates it will need to be familiar with 5+ tools. This will be very challenging to maintain and the example will be outdated very soon.

I'm wondering if it's possible to use Lambda functions from another folder? If so, could we reuse the same set of Lambda functions existing in the cdk examples?

# Current
examples/
  |- cdk/
  |----<all lambda files here>
  |- sam/
  |----<all lambda files here>
  
# Alternative
examples/
 |- cdk/
 |--- <only cdk related code with reference to ../lambda-functions folder>
 |- sam/
 |--- <only SAM related code with reference to ../lambda-functions folder>

The drawback is that this is quite uncommon pattern for both CDK and SAM. So we need to decide on a trade-off here.

@dreamorosi
Copy link
Contributor

As an alternative to @ijemmy's proposal, that I 100% share, I'd like to bring up the option of introducing a prerequisite step for users.

With a structure like this:

examples/
 |- lambda-functions/
 |- cdk/
 |--- <cdk related code & local reference>
 |- sam/
 |--- <SAM related code & local reference>

Before running cdk deploy or sam deploy customers would have to run some version of cp -R ../lambda-functions . (or run a sh script / npm script if copying process is more involved) to bring the code in the given example that they want to try out.

Since to test this they have to clone & checkout the repo locally anyway, any change they make would be on their local version.

This would help keep the examples consistent with their idiomatic way of structuring a project and would still ease the maintenance overhead. The trade-off in this scenario would be that the examples would be less easy to just browse on GitHub.

@bpauwels
Copy link
Contributor Author

bpauwels commented Apr 5, 2022

Thanks for reviewing, I have worked on the comments from @dreamorosi and added a few more commit.

I like your idea of using lambda functions from another folder - but as of now the CDK sample and the SAM sample use completely different functions. (actually the SAM sample is based on the SAM "Quick Start: App Backend using TypeScript"). Would it make sense then?

@ijemmy

@dreamorosi
Copy link
Contributor

dreamorosi commented Apr 5, 2022

Thanks for reviewing, I have worked on the comments from @dreamorosi and added a few more commit.

I like your idea of using lambda functions from another folder - but as of now the CDK sample and the SAM sample use completely different functions. (actually the SAM sample is based on the SAM "Quick Start: App Backend using TypeScript"). Would it make sense then?

@ijemmy

Thank you @bpauwels for addressing all the comments, really appreciate it.

We are open to modify the content of the Lambda functions in the CDK example & converge to a common example surface that showcases Lambda Powertools, so that shouldn't be an issue.

I think at this stage, and for the scope of this PR, it would be great to have the functions of this example extracted from the folder while still being able to deploy it easily and in a relatively idiomatic way.

Later on we will adapt the CDK example to use all / some / most of these same functions and definitely remove all the leftover functions under examples/cdk.

For the CDK example, the functions that we consider most important are:

We consider them as such because these are the ones that show most of the features of the three utilities and with different patterns (manual instrumentation, decorator, Middy middleware).

Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and the samples added. I also agree with @ijemmy and @dreamorosi's observation that this risks to become hard to maintain for us in the long term.

examples/sam/src/handlers/.eslintignore Outdated Show resolved Hide resolved
examples/sam/README.md Outdated Show resolved Hide resolved
examples/sam/README.md Outdated Show resolved Hide resolved
@bpauwels
Copy link
Contributor Author

bpauwels commented Apr 8, 2022

@dreamorosi @saragerion @ijemmy - I could not find any way to automatically create a symlink of an higher level lamdba-functions/ folder to src/handlers/ inside the SAM example. Last thing I tried was a git post-checkout hook but hooks are not commited to the repo. So the only way then would be to add an instruction to the README.md to create the symlink manually.
Do you all agree?

@dreamorosi
Copy link
Contributor

@dreamorosi @saragerion @ijemmy - I could not find any way to automatically create a symlink of an higher level lamdba-functions/ folder to src/handlers/ inside the SAM example. Last thing I tried was a git post-checkout hook but hooks are not commited to the repo. So the only way then would be to add an instruction to the README.md to create the symlink manually.
Do you all agree?

Yep, that's acceptable

examples/sam/template.yaml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
examples/sam/README.md Outdated Show resolved Hide resolved
The SAM CLI reads the application template to determine the API's routes and the functions that they invoke. The `Events` property on each function's definition includes the route and method for each path.

```yaml
Events:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's correct if you look at it relative to the larger template, but since this is a decontextualised code snippet it's a bit odd that the first line starts with 6 spaces.

Either way it's really a minor / nitpicky comment, you can leave it if you think it makes sense.

examples/sam/README.md Outdated Show resolved Hide resolved
examples/lambda-functions/get-all-items.ts Outdated Show resolved Hide resolved
examples/lambda-functions/get-all-items.ts Outdated Show resolved Hide resolved
examples/lambda-functions/get-all-items.ts Outdated Show resolved Hide resolved
examples/lambda-functions/get-by-id.ts Outdated Show resolved Hide resolved
examples/lambda-functions/put-item.ts Outdated Show resolved Hide resolved
@dreamorosi dreamorosi self-requested a review April 27, 2022 12:26
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

dreamorosi
dreamorosi previously approved these changes Apr 27, 2022
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thanks a ton for your hard work @bpauwels, looking forward to see this merged and to hear customer's feedback on it!

examples/sam/README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants