Implement Bucket Notifications APIs #404
Conversation
| headers = { | ||
| 'Content-Length': str(len(content)), | ||
| 'Content-MD5': encode_to_base64(get_md5(content.encode('utf-8'))) |
There was a problem hiding this comment.
Can you test this with 'python 3.5' - the changes related to 'utf-8' were done with this purpose?
There was a problem hiding this comment.
I am using python 3.5 by default for development. It works fine. It also works in python 2.7
|
Can you add some unit-tests as well here located at |
|
Ok at this point APIs are implemented and do appear to work in my testing (both python 3.5 and python 2.7). Up for review. Working on unit testing. |
Will review .. |
|
Added a batch of unit tests. |
|
|
||
| { | ||
| 'TopicConfigurations': [ | ||
| { |
There was a problem hiding this comment.
Need to decide on an appropriate place to put the in-code documentation for the structure expected by the set_bucket_notification() API and returned by get_bucket_notification() API.
The expected structure is indicated here at this point.
|
Next up is documentation in API.md. |
|
Aargh. Tests fail because Windows UA does not match the ones in the unit tests. Need to figure/discuss this. |
* Implements set_bucket_notification(), get_bucket_notification() and remove_all_bucket_notifications() * Notifications structure used for set and get APIs is currently documented in the xml_marshal.py file (TODO: move this to set/get API)?
8ea7243 to
4e96068
Compare
|
|
||
| |Param |Type |Description | | ||
| |:---|:---|:---| | ||
| |``Policy`` | _dict_ | If there is no notification configuration, an empty dictionary is returned. Otherwise it has the same structure as the argument to set_bucket_notification | |
There was a problem hiding this comment.
This is not Policy.. should be NotificationConfig
| <a name="set_bucket_notification"></a> | ||
| ### set_bucket_notification(bucket_name, notification) | ||
|
|
||
| Fetch the notifications configuration on a bucket. |
There was a problem hiding this comment.
This is incorrect Fetch the notifications configuration on a bucket is not the purpose here. We need to change this to reflect what the set_bucket_notification should be doing.
There was a problem hiding this comment.
I was still iterating on it. Wanted to see how it looked. Forgot it would update the PR as well. Still working on it.
There was a problem hiding this comment.
I am going to be pushing and squashing more commits now. I will ping when its ready for review. Please wait :-)
c23d516 to
3297dd1
Compare
3297dd1 to
9a4237f
Compare
|
@harshavardhana Ready for review and perhaps merge! |
Towards resolving #394
At this point it is an early preview, and it is not yet complete.