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

fix(idempotency): POWERTOOLS_IDEMPOTENCY_DISABLED should respect truthy values #4391

Merged

Conversation

stevenhoelscher
Copy link

Issue number: 4390

Summary

Changes

  • When evaluating the POWERTOOLS_IDEMPOTENCY_DISABLED environment variable, first transform the string truthy value into a bool.

User experience

  • Before: A user sets POWERTOOLS_IDEMPOTENCY_DISABLED=false but the idempotency layer would actually be disabled
  • After: A user sets POWERTOOLS_IDEMPOTENCY_DISABLED=false and the idempotency layer would not be disabled

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

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

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@stevenhoelscher stevenhoelscher requested a review from a team as a code owner May 22, 2024 19:32
Copy link

boring-cyborg bot commented May 22, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 22, 2024
@heitorlessa
Copy link
Contributor

heitorlessa commented May 22, 2024 via email

@github-actions github-actions bot added the bug Something isn't working label May 22, 2024
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.28%. Comparing base (e14e768) to head (ffe8e00).
Report is 519 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4391      +/-   ##
===========================================
- Coverage    96.38%   96.28%   -0.10%     
===========================================
  Files          214      219       +5     
  Lines        10030    10489     +459     
  Branches      1846     1871      +25     
===========================================
+ Hits          9667    10099     +432     
- Misses         259      274      +15     
- Partials       104      116      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@boring-cyborg boring-cyborg bot added the tests label May 22, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 22, 2024
@leandrodamascena
Copy link
Contributor

Hi @stevenhoelscher! Thank you so much for submitting this PR. It's a great catch, and I'm glad you discovered this long-standing bug. We really appreciate you taking the time to fix it!

I made only two changes in this PR:
1 - I set the default value of os.getenv to false, making the behavior more explicit.
2 - I included a test case to ensure the correct behavior.

Thanks

@leandrodamascena leandrodamascena requested review from heitorlessa and removed request for heitorlessa May 22, 2024 23:15
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

let's add a warning to prevent customers forgetting it's disabled in dev but promoting it to prod.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 23, 2024
@leandrodamascena
Copy link
Contributor

@heitorlessa ready to review again.

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Last question to clarify a piece I missed (changes are great!!)

aws_lambda_powertools/utilities/idempotency/idempotency.py Outdated Show resolved Hide resolved
aws_lambda_powertools/utilities/idempotency/idempotency.py Outdated Show resolved Hide resolved
aws_lambda_powertools/warnings/__init__.py Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@heitorlessa heitorlessa merged commit 751bf99 into aws-powertools:develop Jun 4, 2024
15 of 16 checks passed
Copy link

boring-cyborg bot commented Jun 4, 2024

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@heitorlessa
Copy link
Contributor

hey @stevenhoelscher thank you so much for kicking this off <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working commons size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: POWERTOOLS_IDEMPOTENCY_DISABLED does not respect truthy values
3 participants