-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix response structure for S3 ListMultipartUploads #7983
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
Conversation
aec8162
to
72afb68
Compare
72afb68
to
338ca3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! another nice catch :-) just wondering about whether we need moto here
localstack/services/s3/provider.py
Outdated
try: | ||
response: ListMultipartUploadsResult = call_moto(context) | ||
except NotImplementedError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question: do we need this try/except? is this method implemented in moto, and are we only using it partially? or can we just replace call_moto
with our implementation all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's partially implemented in moto, and raises only if some parameters are passed:
https://github.com/localstack/moto/blob/58830863f04661fd32d72a79c0046cb667068a34/moto/s3/responses.py#L428-L432
We could replace it I suppose because I think if there's an exception, we're basically doing what it would do if there wasn't an exception. Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it was in the case they would add more things, because right now our implementation is incomplete, and if moto adds feature we wouldn't get them.
This is a follow up from #7545, a user reported an issue in our Community Slack. The test is AWS validated, and once again the specs were not compliant with what S3 really returns.
We also reapply the logic that was in a patch previously set, moto would have raised an error if using some request parameters that were not implemented. It would raise a
NotImplemented
exception. The previous provider was swallowing the exception and letting it pass anyway without applying the parameters. We can keep the same behaviour for now and come back later and actually implement the right behaviour.related: #7981, localstack/docs#539
\cc @steffyP