Skip to content

fix: parser S3Model not compatible with ObjectRemoved events #1639

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

Closed
wants to merge 1 commit into from

Conversation

ran-isenberg
Copy link
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
Contributor Author

@heitorlessa @barreeeiroo FYI

@barreeeiroo
Copy link
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
Contributor Author

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

@barreeeiroo
Copy link
Contributor

barreeeiroo commented Oct 23, 2022

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
Copy link
Contributor Author

ran-isenberg commented Oct 23, 2022

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
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
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
Contributor

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

@ran-isenberg
Copy link
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