Implement Bucket ACL operations in S3 provider#6875
Conversation
| ex = InvalidArgument(message) | ||
| ex.ArgumentName = name | ||
| ex.ArgumentValue = value | ||
| return ex |
There was a problem hiding this comment.
would it help if we made it easier to set the exception attributes, so you can raise
raise InvalidArgument(message, ArgumentName=name, ArgumentValue=value)?
there are two ways that we can do that. either we change the scaffold to create the constructors of exceptions accordingly, or we extend the ServiceException constructor to accept kwargs, to something like:
def __init__(self, *args, **kwargs):
super(ServiceException, self).__init__(*args)
if len(args) >= 1:
self.message = args[0]
else:
self.message = ""
for k,v in kwargs.items():
setattr(self, k) = vthe second option is simpler and also easier to implement, however it makes it harder to subclass or customize the constructor. the first approach would maybe be cleaner but also a bit more effort.
There was a problem hiding this comment.
Right! I think right now, it's not exactly needed, as it concerns only S3. I'm thinking about maybe creating a new file exceptions.py for s3, with the helpers to create the necessary exceptions.
I have some kind of a weird case here with presigned URLs: some exceptions have a lot of members (up to 7 I think), but some fields are not returned nor created, depending on the kind of exceptions. So attaching conditionally the members is quite practical.
If I document it better, maybe it can just be know for s3 that you need to create the exceptions with the helper methods. And I like being able to add some logic while creating the exceptions, like the Bytes version of the field.
Presigned URL exception example:
localstack/localstack/services/s3/presigned_url.py
Lines 158 to 181 in e828540
But I still like your proposal, it's pretty clean. But not sure it's worth the trouble.
No integration tests were really checking ACLs, so this one is not reducing tests failure as much.