Skip to content

feat: Powertools will now override the Log4J log level configured if … #39

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

Closed
wants to merge 0 commits into from

Conversation

msailes
Copy link
Contributor

@msailes msailes commented Aug 18, 2020

…the 'LOG_LEVEL' environment variable is set to a Log4J Level (INFO, ERROR etc...)

Issue #, if available:

Description of changes:

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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

Copy link
Contributor

@pankajagrawal16 pankajagrawal16 left a comment

Choose a reason for hiding this comment

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

One suggestion, else looks neat 🎉

@@ -33,6 +36,15 @@
@Aspect
public final class LambdaLoggingAspect {
private static final ObjectMapper mapper = new ObjectMapper();
private static final String LOG_LEVEL = System.getenv("LOG_LEVEL");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use a example in one of the sample lambda function in example folder ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@msailes I believe after my PR #41 goes inn, we can actually validate this part as well ? Even though it might feel like testing log4j itself, but will be good to validate if those 3 lines of magic does works 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, lets merge #41 first, then re-review this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sure. You can take it as another PR as well, fine by me.

@pankajagrawal16
Copy link
Contributor

hmm what just happend.. i just tried rebasing 🤔

@msailes msailes deleted the log-level-env-var branch August 31, 2020 09:46
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.

2 participants