Added a new method SetUserMetadata to the PostPolicy #839

hnb2 · 2017-10-09T07:55:48Z

Hi there,

here is a PR for a new feature I need for a project I am working on.

Feature

SetUserSpecifiedMetadata(key string, value string) allows you to add user specified metadata following this guide which mention:

key	Explanation
x-amz-meta-*	User-specified metadata.This condition supports exact matching and starts-with condition match type.

Contributing

I went through the contributing doc and ran SERVER_ENDPOINT=xxx ACCESS_KEY=xxx SECRET_KEY=xxx go test -race ./... successfuly and ran go fmt.

But I do not have any unit tests though as the file post-policy.go does not have any I am not sure how you guys would like it to be tested, if you could please provide me some pointers.

I tested the feature itself, it is working and being used in staging (the header is set properly and retrieved through the Minio event notification when the upload is completed, and through HEAD as well).

houndci-bot · 2017-10-09T07:55:57Z

post-policy.go

@@ -167,6 +167,26 @@ func (p *PostPolicy) SetSuccessStatusAction(status string) error {
 	return nil
 }

+func (p *PostPolicy) SetUserSpecifiedMetadata(key string, value string) error {


exported method PostPolicy.SetUserSpecifiedMetadata should have comment or be unexported

harshavardhana · 2017-10-09T07:59:02Z

But I do not have any unit tests though as the file post-policy.go does not have any I am not sure how you guys would like it to be tested, if you could please provide me some pointers.

I tested the feature itself, it is working and being used in staging (the header is set properly and retrieved through the Minio event notification when the upload is completed, and through HEAD as well).

You should add tests in functional_tests.go - these tests will be run on our public server, also you would need to update API.md documenting this feature.

hnb2 · 2017-10-09T08:27:47Z

Hi @harshavardhana, I am trying to run those functional_tests.go but I can't get paste those errors:

➜  minio-go git:(user_defined_metadata) ✗ SERVER_ENDPOINT=10.200.0.71:9000 ACCESS_KEY=JSP83BZY4II2YQPUL308 SECRET_KEY=ccX75Z8HzN2FOdNoNxgbkK/gOgullxdjapdIJVB6 ENABLE_HTTPS=0 go run functional_tests.go
# command-line-arguments
./functional_tests.go:347:94: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument to c.PutObject
./functional_tests.go:356:94: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument to c.PutObject
./functional_tests.go:491:99: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument to c.PutObject
./functional_tests.go:501:23: too many arguments in call to c.GetObject
./functional_tests.go:501:48: undefined: minio.GetObjectOptions
./functional_tests.go:593:99: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument to c.PutObject
./functional_tests.go:604:23: too many arguments in call to c.GetObject
./functional_tests.go:604:48: undefined: minio.GetObjectOptions
./functional_tests.go:682:107: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument
to c.PutObject
./functional_tests.go:760:100: cannot use minio.PutObjectOptions literal (type minio.PutObjectOptions) as type *minio.PutObjectOptions in argument
to c.PutObject
./functional_tests.go:760:100: too many errors

I am on antergos (ArchLinux) running:

➜  minio-go git:(user_defined_metadata) ✗ go version                                                                                              
go version go1.9 linux/amd64

Am I missing something ? (I ran all the go get from the .travis.yml file)

vadmeste · 2017-10-09T12:06:18Z

Hey @hnb2,

I downloaded your PR locally, function_tests.go compiles fine in my machine. Can you check if your minio-go dir is under $GOPATH/src/github.com/minio/ directory ?

hnb2 · 2017-10-09T12:32:56Z

Hey @vadmeste , thanks for answering. I made it work by modifying (locally) the minio imports to use "hnb2/" instead of "minio/" because I am using my fork. I guess that your solution is cleaner.

vadmeste · 2017-10-11T16:56:12Z

post-policy.go

@@ -167,6 +167,28 @@ func (p *PostPolicy) SetSuccessStatusAction(status string) error {
 	return nil
 }

+// SetUserSpecifiedMetadata - Set user specified metadata as a key/value
+// couple. Can be retrieved through a HEAD request or an event.
+func (p *PostPolicy) SetUserSpecifiedMetadata(key string, value string) error {


Isn't better to call this SetUserMetadata(), it will be already consistent with other simliar APIs (like SetContentType, etc..)

Sounds good, will push the changes tomorrow

You might need to squash your changes and update the PR title here. Thanks @hnb2

@harshavardhana Done

krishnasrinivas

@hnb2 You can call it SetMetadata instead of SetUserMetadata

hnb2 · 2017-10-17T08:15:50Z

@krishnasrinivas Ok I will push this change + fix the conflicts

hnb2 · 2017-10-17T08:17:17Z

@krishnasrinivas regarding the method name, I see that this PR has been merged with the name SetUserMetadata. Do you still want me to rename it ?

krishnasrinivas · 2017-10-17T08:18:34Z

@krishnasrinivas regarding the method name, I see that this PR has been merged with the name SetUserMetadata. Do you still want me to rename it ?

No, SetUserMetadata is better

…data

vadmeste

LGTM

houndci-bot reviewed Oct 9, 2017

View reviewed changes

harshavardhana requested review from vadmeste and krishnasrinivas October 9, 2017 07:59

harshavardhana mentioned this pull request Oct 10, 2017

Added a new method SetUserMetadata to the PostPolicy minio/minio-dotnet#178

Merged

vadmeste reviewed Oct 11, 2017

View reviewed changes

hnb2 changed the title ~~Added a new method SetUserSpecifiedMetadata to the PostPolicy~~ Added a new method SetUserMetadata to the PostPolicy Oct 12, 2017

Added a new method SetUserMetadata to the PostPolicy

7b4a3ce

hnb2 force-pushed the user_defined_metadata branch from 552a5c5 to 7b4a3ce Compare October 12, 2017 06:03

Fix conflicts

95e8d23

krishnasrinivas reviewed Oct 17, 2017

View reviewed changes

krishnasrinivas approved these changes Oct 17, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into user_defined_meta…

de1d84a

…data

vadmeste approved these changes Oct 17, 2017

View reviewed changes

nitisht merged commit fe263bf into minio:master Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a new method SetUserMetadata to the PostPolicy #839

Added a new method SetUserMetadata to the PostPolicy #839

hnb2 commented Oct 9, 2017

houndci-bot Oct 9, 2017

harshavardhana commented Oct 9, 2017

hnb2 commented Oct 9, 2017

vadmeste commented Oct 9, 2017

hnb2 commented Oct 9, 2017 •

edited

vadmeste Oct 11, 2017

harshavardhana Oct 11, 2017

hnb2 Oct 11, 2017

harshavardhana Oct 12, 2017

hnb2 Oct 12, 2017

krishnasrinivas left a comment

hnb2 commented Oct 17, 2017

hnb2 commented Oct 17, 2017

krishnasrinivas commented Oct 17, 2017

vadmeste left a comment

Added a new method SetUserMetadata to the PostPolicy #839

Added a new method SetUserMetadata to the PostPolicy #839

Conversation

hnb2 commented Oct 9, 2017

Feature

Contributing

houndci-bot Oct 9, 2017

Choose a reason for hiding this comment

harshavardhana commented Oct 9, 2017

hnb2 commented Oct 9, 2017

vadmeste commented Oct 9, 2017

hnb2 commented Oct 9, 2017 • edited

vadmeste Oct 11, 2017

Choose a reason for hiding this comment

harshavardhana Oct 11, 2017

Choose a reason for hiding this comment

hnb2 Oct 11, 2017

Choose a reason for hiding this comment

harshavardhana Oct 12, 2017

Choose a reason for hiding this comment

hnb2 Oct 12, 2017

Choose a reason for hiding this comment

krishnasrinivas left a comment

Choose a reason for hiding this comment

hnb2 commented Oct 17, 2017

hnb2 commented Oct 17, 2017

krishnasrinivas commented Oct 17, 2017

vadmeste left a comment

Choose a reason for hiding this comment

hnb2 commented Oct 9, 2017 •

edited