Skip to content

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 May 22, 2024 19:32
@boring-cyborg
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
@codecov
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!!)

@sonarqubecloud
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
@boring-cyborg
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