-
Notifications
You must be signed in to change notification settings - Fork 223
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
GetObject API with Args & Response objects. #473
Conversation
Versioning related code included. Examples added for versioning & conditional matches.
7a9782c
to
664eca5
Compare
8fa269f
to
efadd87
Compare
5246bdb
to
b9c190b
Compare
.WithMatchETag(matchEtag) | ||
.WithNotMatchETag(notMatchEtag) | ||
.WithModifiedSince(modifiedSince) | ||
.WithUnModifiedSince(unModifiedSince); |
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.
please remove conflicting options from the example
} | ||
} | ||
} | ||
} |
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.
this example pertains to PR #472 . Can it be removed?
private const string getObjectSignature3 = "Task GetObjectAsync(string bucketName, string objectName, string fileName, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature1 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature2 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature3 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; |
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.
they now have same signature - please remove the duplicates
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've not pushed the resolved conflicts files yet. I will do it soon.
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.
not fixed
public string GetCustomerKeyMD5() | ||
{ | ||
return utils.getMD5SumStr(this.key); | ||
} |
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.
Didn't we decide in previous reviews that this method is unnecessary
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've not pushed the resolved conflicts files yet. I will do it soon.
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.
not fixed
can you rebase this PR @BigUstad |
8a05618
to
f20205f
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.
github actions reported warnings/errors need to be fixed.
public string GetCustomerKeyMD5() | ||
{ | ||
return utils.getMD5SumStr(this.key); | ||
} |
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.
not fixed
public string GetCustomerKeyMD5() | ||
{ | ||
return null; | ||
} |
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.
this method is not required.
private const string getObjectSignature3 = "Task GetObjectAsync(string bucketName, string objectName, string fileName, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature1 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature2 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; | ||
private const string getObjectSignature3 = "Task GetObjectAsync(GetObjectArgs args, CancellationToken cancellationToken = default(CancellationToken))"; |
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.
not fixed
f20205f
to
e36fc2e
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
Minio/Helper/OperationsHelper.cs
Outdated
|
||
utils.ValidateFile(tempFileName); | ||
|
||
FileInfo tempFileInfo = new FileInfo(tempFileName); |
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.
can this be moved inside the if condition code
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.
Yes. Changed.
e36fc2e
to
3fee192
Compare
cdeade3
to
7191d57
Compare
@@ -363,7 +365,7 @@ public PresignedPutObjectArgs WithExpiry(int ex) | |||
} | |||
} | |||
|
|||
public class RemoveUploadArgs : EncryptionArgs<RemoveUploadArgs> | |||
public class RemoveUploadArgs : EncryptionArgs<RemoveUploadArgs> |
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.
indentation
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.
Noted, changed.
1d9afa2
to
d6054ac
Compare
|
||
if (this.ObjectLength < 0) | ||
{ | ||
throw new ArgumentException("Length should be greater than zero", nameof(this.ObjectLength)); |
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.
message should be greater than or equal to
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.
Changed the message & the condition.
@@ -335,7 +337,7 @@ public PresignedPostPolicyArgs WithPolicy(PostPolicy policy) | |||
{ | |||
this.Policy = policy; | |||
return this; | |||
} | |||
} |
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.
why is there an extra space
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.
Happened during vscode 'Accept Both changes'. Removing.
Minio/Helper/OperationsHelper.cs
Outdated
{ | ||
FileInfo tempFileInfo = new FileInfo(tempFileName); | ||
tempFileSize = tempFileInfo.Length; | ||
if (tempFileSize > length) |
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.
what if tempFileSize is < length?
2e7c0b4
to
5225f13
Compare
Minio/Helper/OperationsHelper.cs
Outdated
} | ||
if (File.Exists(args.FileName)) | ||
{ | ||
File.Delete(tempFileName); |
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.
File.Delete(tempFileName); | |
File.Delete(args.FileName); |
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.
Changing that.
5225f13
to
f41d5e7
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
Versioning related code included. Examples added for versioning & conditional matches.