add bucket versioning for s3 #7512
Conversation
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
nadavMiz
force-pushed
the
bucket-versioning
branch
from
19c7c52
to
2d08c34
Compare
| async set_bucket_versioning(params) { | ||
| dbg.log0('NamespaceS3.set_bucket_versioning:', this.bucket, inspect(params)); | ||
| const { versioning } = params; | ||
| await this.s3.putBucketVersioning({Bucket: this.bucket, VersioningConfiguration: {Status: versioning}}).promise(); |
There was a problem hiding this comment.
In this file would you please separate the key-value in new line? (in putBucketVersioning and getBucketVersioning)
| await this.s3.putBucketVersioning({Bucket: this.bucket, VersioningConfiguration: {Status: versioning}}).promise(); | |
| await this.s3.putBucketVersioning({ | |
| Bucket: this.bucket, | |
| VersioningConfiguration: { Status: versioning }, | |
| }).promise(); |
| SUSPENDED: 'Suspended', | ||
| }); | ||
|
|
||
| // TODO: There is also MFA Delete configuration that we do not support |
There was a problem hiding this comment.
Would you please move this comment near the code line VersioningConfiguration (where we are supposed to return the MFADelete property)?
| Status: GET_VERSIONING_STATUS_MAP[bucket_info.versioning] | ||
| } | ||
| }; | ||
| return await req.object_sdk.get_bucket_versioning({name: req.params.bucket}); |
There was a problem hiding this comment.
Would you please save with formatter? I think it should be with extra space
| return await req.object_sdk.get_bucket_versioning({name: req.params.bucket}); | |
| return await req.object_sdk.get_bucket_versioning({ name: req.params.bucket }); |
| @@ -15,7 +15,7 @@ async function put_bucket_versioning(req) { | |||
| } | |||
| await req.object_sdk.set_bucket_versioning({ | |||
| name: req.params.bucket, | |||
| versioning: versioning.toUpperCase() | |||
| versioning: versioning | |||
There was a problem hiding this comment.
Would you please explain what you changed with UpperCase?
I see that you removed it from here and added it somewhere else, but didn't understand why.
| @@ -106,15 +107,40 @@ class BucketSpaceNB { | |||
|
|
|||
| async set_bucket_versioning(params, object_sdk) { | |||
| const ns = await object_sdk._get_bucket_namespace(params.name); | |||
| if (ns instanceof NamespaceS3) { | |||
There was a problem hiding this comment.
Would you please add a short comment that would explain that it was a decision to implement the set_bucket_versioning / get_bucket_versioning under namespace s3 and not in bucket space s3 (what you discussed yesterday)?
| Status: GET_VERSIONING_STATUS_MAP[bucket_info.versioning] | ||
| } | ||
| }; | ||
| return await req.object_sdk.get_bucket_versioning({name: req.params.bucket}); | ||
| } |
There was a problem hiding this comment.
Looking at other actions, we usually get the res and then take from it what we want and explicitly return it. Why did you choose to change it?
| @@ -824,6 +824,7 @@ interface BucketSpace { | |||
| delete_bucket_lifecycle(params: object): Promise<any>; | |||
|
|
|||
| set_bucket_versioning(params: object, object_sdk: ObjectSDK): Promise<any>; | |||
| get_bucket_versioning(params: object, object_sdk: ObjectSDK): Promise<any>; | |||
There was a problem hiding this comment.
Would you please explain why we need params? and why object_sdk?
I see that in the file object_sdk you send them, but I don't see that use them in namespace s3 - the function signature of get_bucket_versioning is without any parameter).
| }); | ||
|
|
||
| //should work, object exist | ||
| await s3.getObject({ Bucket: bucket_name, Key: text_file1, VersionId: put_res.VersionId}); |
There was a problem hiding this comment.
Maybe instead of getObject you can use headObject?
You can add a check of the response you get (to be sure that you've got what you planned).
| @@ -759,15 +806,23 @@ mocha.describe('s3_ops', function() { | |||
| // Objects: [{ Key: text_file1 }, { Key: text_file2 }] | |||
| // } | |||
| // }); | |||
| await s3.deleteObject({ Bucket: BKT5, Key: text_file5 }); | |||
| await s3.deleteObject({ Bucket: BKT5, Key: text_file5}); | |||
There was a problem hiding this comment.
I guess it was a typo (to remove the space).
| // clean up versioning products | ||
| await s3.deleteObject({ Bucket: bucket_name, Key: text_file1, VersionId: put_res.VersionId}); | ||
|
|
||
| await s3.putBucketVersioning({ | ||
| Bucket: bucket_name, | ||
| VersioningConfiguration: {Status: "Suspended"} | ||
| }); | ||
| }); |
There was a problem hiding this comment.
After this step, you should compleatly delete the bucket and create it again.
Suspended versioned bucket is not the same as not enabled one.
This is changing the intention of the entire suite.
|
This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed. |
|
This PR is stale and had no activity for too long - it will now be closed. |
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions: