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

[S3]Add Eventbridge notification for object storage class change #8046

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MikiPWata
Copy link
Contributor

@MikiPWata MikiPWata commented Aug 28, 2024

send eventbridge notification when object's storage class changes
#7363

@MikiPWata
Copy link
Contributor Author

Hey @bblommers!
I'm trying to implement eventbridge notification for object storage class change based on previous PR
any tips on how I should implement it? I'm thinking adding getter and setter is not enough, as we change object's storage class by creating a copy...

@bblommers
Copy link
Collaborator

Hi @MikiPWata!

If I'm reading the documentation right, there are three scenario's where the StorageClass is set in AWS:

  1. When creating an object
  2. When copying an object (and the StorageClass-argument is set)
  3. When a LifeCycle rule kicks in

We can ignore 1, because that's not a change. Number 3 is irrelevant, because we don't act on the lifecycle rules.

So we only have to worry about number 2, the copy operation - and because that's not yet implemented, we have full control on how we do that. Am I correct so far, or have a missed a scenario?

If it's just the copy that we have to worry about, than it makes sense to use a getter/setter pattern IMO. We can use the setter during object creation as well, to check that the StorageClass is valid - as that doesn't happen atm.

@MikiPWata
Copy link
Contributor Author

Hey @bblommers!
Thanks for your comment!
I think you are right, we just need to worry about the copy operation.
I tried implementing getter and setter, but the following error comes up from all test cases for S3's Eventbridge notification.

FAILED tests/test_s3/test_s3_eventbridge_integration.py::test_delete_object_notification - botocore.errorfactory.BucketAlreadyOwnedByYou: An error occurred (BucketAlreadyOwnedByYou) when calling the CreateBucket operation: Your previous request to create the named bucket succeeded and you already own it.

As I'm not that proficient in getters and setters, would love to get some advice on that.

thanks in advance

@bblommers
Copy link
Collaborator

Hey @bblommers! Thanks for your comment! I think you are right, we just need to worry about the copy operation. I tried implementing getter and setter, but the following error comes up from all test cases for S3's Eventbridge notification.

FAILED tests/test_s3/test_s3_eventbridge_integration.py::test_delete_object_notification - botocore.errorfactory.BucketAlreadyOwnedByYou: An error occurred (BucketAlreadyOwnedByYou) when calling the CreateBucket operation: Your previous request to create the named bucket succeeded and you already own it.

As I'm not that proficient in getters and setters, would love to get some advice on that.

thanks in advance

That's odd @MikiPWata. The BucketAlreadyOwnedByYou should only occur when trying to recreate a bucket that already exists and that bucket is created in a non-standard location (outside of us-east-1). So nothing to do with getter/setters or storage class.

Can you push what you have so far? That would make debugging a little easier.

@MikiPWata
Copy link
Contributor Author

@bblommers
This PR currently contains the changes I made. Can you take a look at it?
There's no issue when I rollback the storage_class getter and setter, so I'm pretty sure that my implementation is wrong.

@bblommers
Copy link
Collaborator

Ah, I understand @MikiPWata. The indentation of both the getter and setter is wrong, I guess that broke things in a weird way. If you indent both methods to make it part of the class, the other tests do pass.

Fixed storage_class change eventbridge notification test case accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants