Skip to content
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

add bucket tagging support #3182

Merged
merged 2 commits into from
May 15, 2020

Conversation

balamurugana
Copy link
Member

@balamurugana balamurugana commented Apr 19, 2020

@BigUstad

This comment has been minimized.

@balamurugana
Copy link
Member Author

I get the following errors after pulling the 3 related PRs (minio, minio-go & mc). The errors are in mc code base.

No wonder of failure unless respective base PRs are fixed and merged. I would recommend to test base PRs than higher level PRs

@balamurugana balamurugana force-pushed the add-bucket-tagging-support branch 5 times, most recently from e5ebde0 to 2f5a0f1 Compare April 25, 2020 10:39
@kannappanr
Copy link
Collaborator

@balamurugana Can you PTAL at the build failure? Does this PR depend on minio/minio-go#1273 as minio/minio-go#1279 has been merged and included in go.mod?

@balamurugana
Copy link
Member Author

@balamurugana Can you PTAL at the build failure? Does this PR depend on minio/minio-go#1273 as minio/minio-go#1279 has been merged and included in go.mod?

The PR depends on minio/minio-go#1273

@balamurugana balamurugana force-pushed the add-bucket-tagging-support branch 5 times, most recently from 1a128db to b541906 Compare April 30, 2020 05:54
@BigUstad
Copy link
Contributor

BigUstad commented May 1, 2020

  1. Please refer fixed: copy-object tagging-directive REPLACE with no tags minio#9477. This same bug is present.

  2. Indentation is absent:
    $ mc tag list prihome9001/bkt1/smear-oil-casserole-dish_4.jpg
    datetime: 04/30/20_22:23:23
    confidentiality: open-to-authenticated-only
    editable: only-by-owner-and-authenticated
    designation1: value1

  3. No message for tag not set.
    $ mc tag list prihome9001/bkt1/smear-oil-casserole-dish_3.jpg

  4. Conflicting output message
    $ mc tag remove prihome9001/bkt1/
    mc: Unable to remove tags for prihome9001/bkt1/ We encountered an internal error, please try again.
    Tags removed for prihome9001/bkt1/.
    $ mc tag remove s3/pribkt6
    mc: Unable to remove tags for s3/pribkt6 The specified bucket does not exist
    Tags removed for s3/pribkt6.

@balamurugana
Copy link
Member Author

1. Please refer [minio/minio#9477](https://github.com/minio/minio/issues/9477). This same bug is present.

I removed getCpObjTagsFromHeader() and am using the logic directly.

@balamurugana balamurugana force-pushed the add-bucket-tagging-support branch 2 times, most recently from 6ef1f81 to 7f15b33 Compare May 1, 2020 13:38
@balamurugana
Copy link
Member Author

balamurugana commented May 1, 2020

2\. Indentation is absent:

Fixed. Also note that the difference in output.

3\. No message for tag not set.

Two different behaviour in AWS S3. For buckets, it returns below error, but objects it returns success with no tags.

<?xml version="1.0" encoding="UTF-8"?>                                                                                                             
<Error><Code>NoSuchTagSet</Code><Message>The TagSet does not exist</Message><BucketName>bala-test-bucket1</BucketName><RequestId>35834BE42705186B</RequestId><HostId>VzUofcmRRhYyjQkmWc7X5tRsgZWgui57S0HocPyKayydaLR4f9Ya0cRmVVfKNwO+XktqBvMi4go=</HostId></Error>

I have introduced to print an error message returned like S3, but --debug shows success.

4\. Conflicting output message

mc tag remove should behave like below

[bala@localhost mc]$ ./mc -C ~/.mc tag remove s3/bala-test-bucket1
Tags removed for s3/bala-test-bucket1.
[bala@localhost mc]$ ./mc -C ~/.mc tag remove s3/bala-test-bucket1
Tags removed for s3/bala-test-bucket1.
[bala@localhost mc]$ ./mc -C ~/.mc tag remove s3/bala-test-bucket1/zero
Tags removed for s3/bala-test-bucket1/zero.
[bala@localhost mc]$ ./mc -C ~/.mc tag remove s3/bala-test-bucket1/zero
Tags removed for s3/bala-test-bucket1/zero.
[bala@localhost mc]$ 

cmd/tag-remove.go Outdated Show resolved Hide resolved
cmd/tag-set.go Outdated Show resolved Hide resolved
kannappanr
kannappanr previously approved these changes May 1, 2020
Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kannappanr
Copy link
Collaborator

Output currently is

$ mc tag list myminio/vvertica/issue
Name                :    issue
datetime            :    2020Mar96TS120000
editable            :    only-by-owner-and-authenticated
confidentiality     :    open-to-authenticated-only

with this PR it is

$ mc tag list myminio/vvertica/issue
confidentiality : open-to-authenticated-only
datetime        : 2020Mar96TS120000
editable        : only-by-owner-and-authenticated

it is missing the name of the object being displayed. Can that be added back with the same color?

@balamurugana
Copy link
Member Author

balamurugana commented May 3, 2020

it is missing the name of the object being displayed. Can that be added back with the same color?

I removed it on purpose because the command fetches only one entity at once and Name : issue is a confusion which is mixed with tags in the output.

Another interesting case is an user can have tag key as Name and the output would ended up with a surprise.

BigUstad
BigUstad previously approved these changes May 5, 2020
Copy link
Contributor

@BigUstad BigUstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kannappanr
Copy link
Collaborator

@balamurugana We followed the mc stat style

 $ mc stat play/test/issue
Name      : issue
Date      : 2020-05-04 13:41:03 PDT 
Size      : 26 B   
ETag      : b954418e6a50d4d4cb8f02776d867550 
Type      : file 
Metadata  :
  X-Amz-Meta-Mc-Attrs                : atime:1588604789/ctime:1559178264/gid:0/gname:root/mode:33188/mtime:1549351932/uid:0/uname:root 
  X-Amz-Object-Lock-Mode             : GOVERNANCE 
  Content-Type                       : application/octet-stream 
  X-Amz-Object-Lock-Retain-Until-Date: 2020-05-05T20:41:03Z 
Encrypted :
  X-Amz-Server-Side-Encryption: AES256 

The one in the top is in different color and you will not see that in json format. Not sure what is the cause for confusion.

@harshavardhana
Copy link
Member

@balamurugana #3212 please take some changes from this PR including fixes for

  • support autocompletion
  • non-idiomatic usage of probe.NewError
  • error messages and indentation in help messages etc.

@balamurugana
Copy link
Member Author

The one in the top is in different color and you will not see that in json format. Not sure what is the cause for confusion.

Name is added back like stat output.

kannappanr
kannappanr previously approved these changes May 7, 2020
Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@harshavardhana harshavardhana merged commit 09c31c4 into minio:master May 15, 2020
@balamurugana balamurugana deleted the add-bucket-tagging-support branch May 20, 2020 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants