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

feat(idempotency): support methods with the same name (ABCs) by including fully qualified name in v2 #1535

Merged

Conversation

leandrodamascena
Copy link
Contributor

@leandrodamascena leandrodamascena commented Sep 21, 2022

Issue number: #1330

Summary

Changes

This change modifies the name of the key generated by the idempotency methods. In case you have 2 functions with the same name, the hash is calculated only for the first one and the other one cannot use the idempotency function.

User experience

Please share what the user experience looks like before and after this change

Before to this change, the idempotency key was just using the function name:
image

After this change, the idempotency key includes the module and Python qualified name (PEP 3155)
image

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.


View rendered docs/utilities/idempotency.md

@leandrodamascena leandrodamascena requested a review from a team as a code owner September 21, 2022 09:51
@leandrodamascena leandrodamascena requested review from am29d and removed request for a team September 21, 2022 09:51
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2022
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation tests labels Sep 21, 2022
@github-actions github-actions bot added the feature New feature or functionality label Sep 21, 2022
@leandrodamascena leandrodamascena requested review from rubenfonseca and removed request for am29d September 21, 2022 09:52
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Base: 99.42% // Head: 99.42% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (07d76c8) compared to base (0777fc9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2    #1535      +/-   ##
==========================================
- Coverage   99.42%   99.42%   -0.01%     
==========================================
  Files         128      125       -3     
  Lines        5745     5726      -19     
  Branches      670      671       +1     
==========================================
- Hits         5712     5693      -19     
  Misses         18       18              
  Partials       15       15              
Impacted Files Coverage Δ
aws_lambda_powertools/tracing/tracer.py 100.00% <100.00%> (ø)
...ws_lambda_powertools/utilities/idempotency/base.py 100.00% <100.00%> (ø)
aws_lambda_powertools/event_handler/__init__.py
aws_lambda_powertools/metrics/__init__.py
aws_lambda_powertools/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

It seems the Lambda function is lacking permissions to RW to DDB.

Great job! I left some comments, but I would like to share some ideas about improving the e2e tests

tests/e2e/idempotency/test_idempotency_dynamodb.py Outdated Show resolved Hide resolved
tests/e2e/utils/data_fetcher/idempotency.py Outdated Show resolved Hide resolved
tests/e2e/utils/data_fetcher/idempotency.py Outdated Show resolved Hide resolved
@leandrodamascena
Copy link
Contributor Author

Actually I was missing the RW Permissions for DDB, but I'll investigate why I'm running fine here even without this permission.

Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

This is SOOOOOOOOOO awesome Leandro, fantastic job! Just left a slight note for us to think about, but maybe it's not a problem.

Should we also update the docs/upgrade.md explaining the breaking change?

@leandrodamascena
Copy link
Contributor Author

This is SOOOOOOOOOO awesome Leandro, fantastic job! Just left a slight note for us to think about, but maybe it's not a problem.

Should we also update the docs/upgrade.md explaining the breaking change?

Heyyyy @rubenfonseca, thank you so much for this feedback, I appreciate a lot! I like the way you encourage the work of others! ✌️ 🚀

I agree 100% that we must update the documentation and I did. Please review this and add what you think is important.

Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

image

@rubenfonseca rubenfonseca merged commit 5046a35 into aws-powertools:v2 Sep 27, 2022
rubenfonseca added a commit that referenced this pull request Oct 14, 2022
…ding fully qualified name in v2 (#1535)

Co-authored-by: Rúben Fonseca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or functionality 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.

3 participants