Skip to content

fix: parser S3Model not compatible with ObjectRemoved events - #1639

Closed
ran-isenberg wants to merge 1 commit into
aws-powertools:developfrom
ran-isenberg:s3
Closed

fix: parser S3Model not compatible with ObjectRemoved events#1639
ran-isenberg wants to merge 1 commit into
aws-powertools:developfrom
ran-isenberg:s3

Conversation

@ran-isenberg

Copy link
Copy Markdown
Contributor

issue #1637

@ran-isenberg
ran-isenberg requested a review from a team as a code owner October 23, 2022 19:31
@ran-isenberg
ran-isenberg requested review from rubenfonseca and removed request for a team October 23, 2022 19:31
@boring-cyborg boring-cyborg Bot added the tests label Oct 23, 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 23, 2022
@ran-isenberg

Copy link
Copy Markdown
Contributor Author

@heitorlessa @barreeeiroo FYI

@barreeeiroo

Copy link
Copy Markdown
Contributor

Thanks @ran-isenberg! I'm back with my laptop, so I was about to test it now. Would it be fine if I cherrypick your commit into a single PR?

@ran-isenberg

Copy link
Copy Markdown
Contributor Author

Yeah sure np! I had to make some fixes to the validator.

@barreeeiroo

barreeeiroo commented Oct 23, 2022

Copy link
Copy Markdown
Contributor

I see you added an if s3_object. Is it really needed? I added an additional test case (to make sure we throw the exception), and it seems to be passing without it.
The root_validator should run after parsing the object, so technically s3_object is never None (otherwise a ValidationException is thrown earlier.

@ran-isenberg

ran-isenberg commented Oct 23, 2022

Copy link
Copy Markdown
Contributor Author

I see you added an if s3_object. Is it really needed? I added an additional test case (to make sure we throw the exception), and it seems to be passing without it. The root_validator should run after parsing the object, so technically s3_object is never None (otherwise a ValidationException is thrown earlier.

no, it's not needed, i think you can remove the extra test case too.
I wrote an earlier version with pre=True but then removed it, forgot to remove this if.

@barreeeiroo

Copy link
Copy Markdown
Contributor

I think it should be done now then. Can you double-check #1638 to see if I missed anything from your changes?

@ran-isenberg
ran-isenberg deleted the s3 branch October 24, 2022 04:58
@heitorlessa

Copy link
Copy Markdown
Contributor

@leandrodamascena let's merge @ran-isenberg into @barreeeiroo PR, and make sure @ran-isenberg is a co-author in that PR

@heitorlessa
heitorlessa requested review from leandrodamascena and removed request for rubenfonseca October 24, 2022 06:04
@heitorlessa

Copy link
Copy Markdown
Contributor

nvm, it is already in (need coffee). I'll merge and add Ran as a co-author

@ran-isenberg

Copy link
Copy Markdown
Contributor Author

@heitorlessa it's already merged there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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