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

chore(deps): remove email-validator; use Str over EmailStr in SES model #1608

Merged

Conversation

rubenfonseca
Copy link
Contributor

@rubenfonseca rubenfonseca commented Oct 14, 2022

Issue number: #1607

Summary

Changes

Removes email-validator from the extra group dependencies.

Please provide a summary of what's being changed

User experience

This will also reduce the total Lambda Layer and SAR app size.

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

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.

@boring-cyborg boring-cyborg bot added the dependencies Pull requests that update a dependency file label Oct 14, 2022
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2022
@rubenfonseca rubenfonseca marked this pull request as ready for review October 14, 2022 20:11
@rubenfonseca rubenfonseca requested a review from a team as a code owner October 14, 2022 20:11
@rubenfonseca rubenfonseca requested review from leandrodamascena and heitorlessa and removed request for a team October 14, 2022 20:11
@rubenfonseca rubenfonseca linked an issue Oct 14, 2022 that may be closed by this pull request
2 tasks
@github-actions github-actions bot added the internal Maintenance changes label Oct 14, 2022
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Base: 99.38% // Head: 99.40% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (6507bee) compared to base (4b76eca).
Patch coverage: 91.46% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2    #1608      +/-   ##
==========================================
+ Coverage   99.38%   99.40%   +0.01%     
==========================================
  Files         125      127       +2     
  Lines        5731     5836     +105     
  Branches      359      367       +8     
==========================================
+ Hits         5696     5801     +105     
+ Misses         18       17       -1     
- Partials       17       18       +1     
Impacted Files Coverage Δ
aws_lambda_powertools/tracing/extensions.py 100.00% <ø> (ø)
aws_lambda_powertools/utilities/batch/__init__.py 100.00% <ø> (ø)
...ws_lambda_powertools/utilities/batch/exceptions.py 100.00% <ø> (ø)
...a_powertools/utilities/parser/envelopes/kinesis.py 100.00% <ø> (ø)
aws_lambda_powertools/shared/cookies.py 58.69% <58.69%> (ø)
...bda_powertools/utilities/data_classes/alb_event.py 90.47% <66.66%> (ø)
...ambda_powertools/utilities/parameters/appconfig.py 94.28% <86.66%> (ø)
aws_lambda_powertools/utilities/batch/base.py 97.79% <87.50%> (-1.42%) ⬇️
aws_lambda_powertools/event_handler/api_gateway.py 100.00% <100.00%> (ø)
aws_lambda_powertools/event_handler/appsync.py 100.00% <100.00%> (ø)
... and 41 more

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.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2022
@@ -21,7 +20,7 @@ class SesReceiptAction(BaseModel):
class SesReceipt(BaseModel):
timestamp: datetime
processingTimeMillis: PositiveInt
recipients: List[EmailStr]
recipients: List[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rubenfonseca @heitorlessa , that's a breaking change. users are expecting here proper email address validation.
I think a better solution would be:

  1. During import try to import EmailStr, if there's an except redefine the type as str
  2. add docs and specify that for Ses model, you should install the extra dependency

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ran-isenberg , we hear you and appreciate your feedback. We’re including this change in V2 to not impact customers in any shape or form.

To add to my comment earlier, as helpful as e-mail validator may be for an untrusted payload, this is not the case for SES. It would, however, for something like “body” in API Gateway.

I’ll update the issue linked with this and related Pydantic overhead that are making us more cautious on double validation.

@heitorlessa
Copy link
Contributor

Reviewing it now.

Before merging, we need to make sure @huonw @mgochoa are credited. Let's try using the recently discovered manual co-authors feature in GitHub. We should do it in the Release notes regardless

@heitorlessa heitorlessa self-assigned this Oct 18, 2022
@rubenfonseca rubenfonseca changed the title chore: remove email-validator dependency from the all group chore: remove email-validator dependency Oct 18, 2022
@heitorlessa heitorlessa changed the title chore: remove email-validator dependency chore(deps): remove email-validator dependency; use Str over EmailStr in SES model Oct 18, 2022
@heitorlessa heitorlessa changed the title chore(deps): remove email-validator dependency; use Str over EmailStr in SES model chore(deps): remove email-validator; use Str over EmailStr in SES model Oct 18, 2022
@heitorlessa heitorlessa removed the request for review from leandrodamascena October 18, 2022 08:51
@rubenfonseca rubenfonseca merged commit 648501b into aws-powertools:v2 Oct 18, 2022
@rubenfonseca rubenfonseca deleted the fix/remove-email-validator branch October 18, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file internal Maintenance changes size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: Remove email-validator dependency for v2
4 participants