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
chore(storage): add proto converters for Object metadata #4583
Conversation
9888779
to
880454a
Compare
b83994e
to
123a6b6
Compare
123a6b6
to
830c7b9
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.
Good start! Two more thoughts
- I think we have unit tests for raw object encoding/decoding-- we should add the same for proto.
- This PR only covers object metadata and not bucket metadata. Let's change the title to indicate that.
Acl: toProtoObjectACL(o.ACL), | ||
Metadata: o.Metadata, | ||
CustomTime: ct, | ||
KmsKey: o.KMSKeyName, |
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 it necessary to add this field here and not in the apiary object?
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.
Idk if it's necessary or not, but it's in the proto so I included it here. Should I remove it?
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.
Hmm, looks like it might be an oversight in the current implementation which is concerning!
Deleted: convertProtoTime(o.GetDeleteTime()), | ||
Updated: convertProtoTime(o.GetUpdateTime()), | ||
CustomTime: convertProtoTime(o.GetCustomTime()), | ||
} |
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 noticed ETag is missing, is that no longer provided via gRPC?
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.
There isn't an etag field in the protos so I guess not.
@tritone I've added test cases PTAL! |
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.
thanks for adding tests!
Acl: toProtoObjectACL(o.ACL), | ||
Metadata: o.Metadata, | ||
CustomTime: ct, | ||
KmsKey: o.KMSKeyName, |
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.
Hmm, looks like it might be an oversight in the current implementation which is concerning!
Adds converters for the various metadata types, between the library version and the protobuf version.