-
Notifications
You must be signed in to change notification settings - Fork 111
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 S3EventNotifier
example
#477
Conversation
Thank you for your contribution. This is much appreciated. I will add some comments as a code review.
|
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.
Thank you for this great contribution ! I think you're on the right path. I did propose a couple of simplifications. The primary objective of the example code is readability and understandability. The code must be simple to read and understand and distraction-free.
I think there are a bit of improvement possible with this objective in mind.
I'm happy to discuss alternatives.
@sebsto I currently have no easy way to test the CLI commands you recommended, I did check them against what we have currently deployed and they look good. Should we keep them in? |
Thank you for the quick turnaround. I will provide additional comments on the README. The example does not compile on the CI chain. This is because it compiles on Linux and This triggers the question : did you package and deploy this code on Lambda to test it ? |
Yep I have the lambda (with some additions such as the POST request) running in production just fine. I guess either AHC or some other dependency which I'm using there and we trimmed here use Foundation and it's falling back to that. I'll update this to use Foundation |
Thank you for all these changes. It looks good to me now. I will check the CLI command before merging |
Examples/S3EventNotifier/README.md
Outdated
[!IMPORTANT] | ||
The Lambda function and the S3 bucket must be located in the same AWS Region. Adjust the code above according to your closest AWS Region. |
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 may be unnecessary if we have the one above already? But I'm not precious about it. If you do end up leaving it, this should use block quotes too
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 made it on purpose - it so important and I'm sure so many will not read it :-)
Examples/S3EventNotifier/README.md
Outdated
[!IMPORTANT] | ||
The Lambda function and the S3 bucket must be located in the same AWS Region. In the code below, we use `eu-west-1` (Ireland). |
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 GH markdown requires this to be in block quotes, so
> [!IMPORTANT]
> The Lambda function...
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.
You're correct - thank you
Add
S3EventNotifier
exampleMotivation:
There is currently no example regarding an AWS event triggered lambda, such as a lambda that gets invoked on an S3 object upload. I've had to look for a while to figure out that the
AWSLambdaEvents
exists and that it can be used for that (had to find out via a Java example! 😜) so I think this could be useful and a somewhat common use caseModifications:
Added an example of a lambda that gets triggered when an object gets uploaded to an S3 bucket.
I have not described how to set up the actual S3 event in AWS because I figured if you needed such a lambda you'd know, but I can describe that too in the README if needed