-
Notifications
You must be signed in to change notification settings - Fork 282
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
VersionId Support in removeObjects api #909
VersionId Support in removeObjects api #909
Conversation
2fa7637
to
a96ba10
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
src/main/minio.js
Outdated
// * `objectsList` _array_: array of objects | ||
// * `objectsList` _array_: array of objects of one of the following: | ||
// * List of Object names as array of strings which are object keys: ['objectname1','objectname2'] | ||
// * List of Object name and versionId as an object: [{Key:"objectname",VersionId:"my-version-id"}] |
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.
Non-versioned listObjects
output is accepted by removeObjects
api as is, but when it comes to versioned objects, user is supposed to manipulate the output and set the objects list with specific key names:
{
Key: <object-name>
VersionId: <version-id>
},
{
Key: <object-name>
VersionId: <version-id>
},
:
:
IMHO, the deleteList
should stick with the same nomenclature listObjects
api returns.
Here is what listObjects
api output looks like for versioned objects:
{
name: <object-name>
versionId: <version-id>
},
{
name: <object-name>
versionId: <version-id>
},
:
:
This is confusing for the end user, that is, sometimes it is ok to feed the output of listObjects
api to removeObjects
api without any manipulation, and sometimes it should be manipulated before feeding it to removeObjects
api.
I think, removeObjects
api should always take the listObjects
output as is regardless of the type of listing, and we need to take care of required structural key name changes internally.
src/main/minio.js
Outdated
//Backward Compatibility | ||
let entry | ||
if(isObject(value)){ | ||
entry={"Object": [{"Key": value.Key, "VersionId":value.VersionId}]} |
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.
To achieve the suggested change in the above comment, we need a one line change here:
entry={"Object": [{"Key": value.Key, "VersionId":value.VersionId}]} | |
entry={"Object": [{"Key": value.name, "VersionId":value.versionId}]} |
examples/remove-objects.js
Outdated
|
||
var s3Client = new Minio.Client({ | ||
endPoint: 's3.amazonaws.com', | ||
port: 9000, | ||
useSSL: false, | ||
accessKey: 'YOUR-ACCESSKEYID', | ||
secretKey: 'YOUR-SECRETACCESSKEY' | ||
}); | ||
}) | ||
|
||
var objectsList = [] | ||
|
||
// List all object paths in bucket my-bucketname. | ||
var objectsStream = s3Client.listObjects('my-bucketname', '', true) |
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.
Assuming the other 2 suggested changes/comments are accepted, this example script can be simplified to work for both versioned and non-versioned scenarios:
var objectsList = []
var bucket = '<bucket-name>'
var prefix = '<prefix-name>'
var recursive = <true | false>
var includeVersion = <true | false>
// List all object paths in bucket
var objectsStream = s3Client.listObjects(bucket, prefix, recursive, {IncludeVersion: includeVersion})
objectsStream.on('data', function(obj) {
if (includeVersion) {
objectsList.push(obj)
} else {
objectsList.push(obj.name)
}
})
objectsStream.on('error', function(e) {
return console.log(e)
})
objectsStream.on('end', function() {
s3Client.removeObjects(bucket, objectsList, function(e) {
if (e) {
return console.log(e)
}
console.log("Success")
})
})
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.
Thank you for the suggestion. I will refactor.
e9277c4
to
940c9bd
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.
Couple of minor changes...
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
VersionId Support in removeObjects api