-
Notifications
You must be signed in to change notification settings - Fork 90
[feat] Import examples from aws-samples/aws-lambda-powertools-examples #1051
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
Conversation
@@ -0,0 +1,53 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
Introducing an intermediate parent pom to aggregate the samples together
@@ -0,0 +1,138 @@ | |||
# CoreUtilities |
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.
These and the rest of the concrete samples have simply been lifted from main
from the linked samples repository.
https://github.com/aws-samples/aws-lambda-powertools-examples
@@ -37,7 +37,8 @@ | |||
<module>powertools-validation</module> | |||
<module>powertools-test-suite</module> | |||
<module>powertools-cloudformation</module> | |||
<module>powertools-idempotency</module> | |||
<module>powertools-idempotency</module> | |||
<module>examples</module> |
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.
Discussion point: do we want the samples to be build as part of the main project reactor here?
It makes the build easier, and we can simply choose not to release the examples project.
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.
What's the build time? Does it have any side effects?
If there are no issues with mentioned above questions, I believe that it's good to have it built as a part of build process to have quick feedback when some changes are done in the project (version of PowerTools used in examples might be an issue here...)
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.
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.
Yes, that would be desirable, we'd have a mechanism to keep the samples up to date.
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.
+1
@@ -152,4 +152,13 @@ | |||
<Class name="software.amazon.lambda.powertools.logging.internal.LambdaLoggingAspect"/> | |||
<Method name="setLogLevelBasedOnSamplingRate"/> | |||
</Match> | |||
<Match> |
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.
Spotbugs wasn't run on the examples previously. This lint comes from the original projects and I have not yet corrected it.
examples/powertools-examples-sqs/src/main/java/org/demo/sqs/SqsMessageSender.java
Outdated
Show resolved
Hide resolved
examples/powertools-examples-sqs/src/main/java/org/demo/sqs/SqsPoller.java
Outdated
Show resolved
Hide resolved
@@ -450,7 +452,7 @@ | |||
<profile> | |||
<id>build-with-spotbugs</id> | |||
<activation> | |||
<activeByDefault>true</activeByDefault> | |||
<activeByDefault>false</activeByDefault> |
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 turned this off by default, as it 1/ helps with the SAM tooling for the examples and 2/ speeds up local builds that haven't opted into it.
The github workflows explicitly activate and de-activate these using the profiles, so it has no impact on CI.
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.
Should the example poms have the examples/pom.xml as parent or the lambda-powertools root pom?
(See my other comments)
yes - silly mistake - some were, some weren't. fixed. |
Looks good to me. One minor suggestion: I think that we might change names of 2 examples packages in theirs pom.xml to have it more consistent. Right now when we run all tests it produces logs (please ignore info about failing test):
|
I've changed the example project names to match the general format, and i've put a publish exclusion on them so they don't end up in maven central. |
Created change in aws-samples/aws-lambda-powertools-examples/pull/296 to forward viewers onto the samples here. |
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.
Good for me
Issue #, if available:
#1050
Description of changes:
Moving the samples from https://github.com/aws-samples/aws-lambda-powertools-examples into the repository. This will help make the samples more discoverable, allow us to keep them up-to-date with the powertools themselves, and follow the pattern established by the python powertools.
Checklist
Detailed Description
The code is lifted verbatim from the old repository. I've tied it all together into one big maven build tree, and introduced an intermediate pom in the examples project. This has included some effort to restructure the examples themselves so they match the general maven project structure.
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.