Refactors get_bucket_policy and set_bucket_policy apis #639
Conversation
ebozduman
force-pushed
the
policy_rewrite2
branch
from
a0ca60d to
48ec3cd
Compare
docs/API.md
Outdated
| ]} | ||
|
|
||
|
|
||
| minioClient.set_bucket_policy('mybucket', policy_READ_ONLY) |
There was a problem hiding this comment.
would be better to rename this variable as policy_read_only
examples/set_bucket_policy.py
Outdated
| # enables 'my-bucketname' readable by everyone. | ||
| client.set_bucket_policy('my-bucketname', '', Policy.READ_ONLY) | ||
| # Set bucket policy to read-only for bucket 'my-bucketname' | ||
| policy_READ_ONLY = { |
There was a problem hiding this comment.
lower casing this variable is better
examples/set_bucket_policy.py
Outdated
| client.set_bucket_policy('my-bucketname', 'public-folder/', | ||
| Policy.READ_WRITE) | ||
| # Set bucket policy to read-write for bucket 'my-bucketname' | ||
| policy_READ_WRITE = { |
There was a problem hiding this comment.
s/policy_READ_WRITE/policy_read_write
| # writeable by everyone. | ||
| client.set_bucket_policy('my-bucketname', 'incoming', | ||
| Policy.WRITE_ONLY) | ||
| # Set bucket policy to write-only for bucket 'my-bucketname' |
There was a problem hiding this comment.
s/policy_WRITE_ONLY/policy_write_only
| policy_name = client.get_bucket_policy(bucket_name) | ||
| if policy_name != Policy.READ_ONLY: | ||
| raise ValueError('Failed to set ReadOnly bucket policy: ' + policy_name) | ||
| if not policy_validated(client, bucket_name, policy): |
There was a problem hiding this comment.
wouldn't a simple string compare of policy json to be uploaded and policy retrieved by get_bucket_policy validate this? Don't see a need for this helper function.
There was a problem hiding this comment.
I've seen different, sometimes duplicate Action and/or Resource entries returned from the server. So, the format of the json response is not guaranteed to be the same each time. That's why I've introduced the helper function.
| policy = minioClient.get_bucket_policy('mybucket', | ||
| 'my-prefixname') | ||
| # Get current policy of all object paths in bucket "mybucket". | ||
| policy = minioClient.get_bucket_policy('mybucket') |
There was a problem hiding this comment.
When the bucket doesn't have a policy, this function raises minio.error.NoSuchBucketPolicy exception, before, we were returning None. It makes sense to discuss if we need to keep the old behavior or not, and document this in all cases.
There was a problem hiding this comment.
We should return NoSuchBucketPolicy since that would be expected now. it's a breaking change so don't expect old behavior.
There was a problem hiding this comment.
okay, then we need to document this
There was a problem hiding this comment.
You mean in the Release Notes, right? Yes, we have to explain this breaking change in the Release Notes.
There was a problem hiding this comment.
We should simply bump the major version, it is expected that they revisit their applications after such an upgrade.
ebozduman
force-pushed
the
policy_rewrite2
branch
from
50460bd to
15d92b9
Compare
|
@vadmeste, @harshavardhana, @poornas , |
|
@ebozduman, please check the build failures. |
docs/API.md
Outdated
| minioClient.set_bucket_policy('mybucket', | ||
| 'my-prefixname', | ||
| Policy.READ_ONLY) | ||
| # Set policy to read only to all object paths in bucket. |
There was a problem hiding this comment.
suggest rewording line 361 to Set bucket policy to read only
There was a problem hiding this comment.
Done.
Also resolved Travis problem.
ebozduman
force-pushed
the
policy_rewrite2
branch
3 times, most recently
from
55574fa to
3be945f
Compare
|
@poornas, @harshavardhana, @krishnasrinivas, |
|
@poornas, @harshavardhana, @krishnasrinivas, |
harshavardhana
left a comment
harshavardhana
left a comment
There was a problem hiding this comment.
One thing missing is version needs to be updated @ebozduman
ebozduman
force-pushed
the
policy_rewrite2
branch
from
fd83117 to
ac1a340
Compare
5 participants
Fixes #635