Repository navigation
fix(logger): empty or unparsable POWERTOOLS_DEV no longer crashes Logger - #8783
Conversation
… env vars Signed-off-by: Krishna <[email protected]>
Signed-off-by: Krishna <[email protected]>
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
powertools-for-aws-oss-automation
Bot
added
the
size/M
blacksoxx
left a comment
blacksoxx
left a comment
There was a problem hiding this comment.
Reads correct to me; I did not run the tests here.
The scope matches what leandrodamascena settled on #8765: one resolver in shared/functions.py, used from powertools_dev_is_set, powertools_debug_is_set, is_metrics_disabled and the circuit breaker, with the two idempotency call sites left for #8728. A missing variable still returns the default without a warning, an empty or unparsable one warns with PowertoolsUserWarning and falls back, and valid values go through strtobool unchanged, so existing behaviour holds. The Logger and circuit breaker tests go through the public API, and closes #8765 sits on its own line.
Validation: read the diff at 6505f99 against base 9348364 and the four call sites on develop. Not run: pytest, mypy, ruff.
|
Thanks for the careful review, and for checking the call sites too. Let me know if anything else needs a look. |
leandrodamascena
left a comment
leandrodamascena
left a comment
There was a problem hiding this comment.
Thanks @krishbuilds01! This is what I had in mind on #8765. I ran logger, metrics and circuit breaker on top of develop and it's all green. Once this is in I'll make #8734 use your resolver too, so we don't end up with two of them. Goes out in 3.41.0, no need to rebase, you'll be in the release notes.
powertools-for-aws-oss-automation
Bot
added
size/XL
leandrodamascena
left a comment
leandrodamascena
left a comment
There was a problem hiding this comment.
Re-approving after pushing the regenerated agent skill references, nothing else changed.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8783 +/- ##
========================================
Coverage 97.18% 97.18%
========================================
Files 323 323
Lines 16546 16555 +9
Branches 1495 1496 +1
========================================
+ Hits 16080 16089 +9
Misses 326 326
Partials 140 140 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
|
Thanks Leandro, really appreciate the review and the guidance on this one. Happy it is going out in 3.41.0. |
Setting POWERTOOLS_DEV to an empty string crashed Logger while it was being created, because powertools_dev_is_set() passed the raw value to strtobool and the ValueError escaped. An empty value is easy to end up with from a template or a CI variable, so the crash hit every invocation. An invalid value like treu crashed the same way.
This adds one resolver, resolve_bool_env_var, in shared/functions.py. An empty or unparsable value now warns with PowertoolsUserWarning and falls back to the default, as ADR 0006 asks for, and it matches how #8757 handled POWERTOOLS_PARAMETERS_MAX_AGE. The resolver is used by powertools_dev_is_set, powertools_debug_is_set, is_metrics_disabled and the circuit breaker check, which is the scope settled in the issue thread. The two call sites in idempotency are left alone, they belong to #8728. Valid values behave exactly as before.
The new tests fail on develop and pass with this change. I ran the shared functions and metrics unit tests (52 passed), the circuit breaker functional tests (46 passed), the logger and metrics unit tests (38 passed) and the logger functional tests (177 passed), plus ruff and mypy on the changed files.
I used an AI assistant while preparing this change and reviewed the diff and the test results myself before opening it.
closes #8765
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.