-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Support Azure storage container operations #82
Conversation
'lease_state' => 'available', | ||
'lease_duration' => nil}, | ||
metadata: { | ||
'key1' => 'value1', |
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.
Trailing whitespace detected.
end | ||
|
||
def test_get_container_acl_with_internal_client_success | ||
@blob_client.stub :get_container_acl, @storage_container_object do |
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.
Use 2 (not 4) spaces for indentation.
@shaffan-chaudhry-confiz Thanks for reviewing the long list of the files. I've addressed all the comments. The unclosed hound comments seems not valid because it doesn't show the latest change and the missing lines are added. I also added the unit test cases for the requests. Please be noted that I didn't directly call any REST API for storage data plane. They are wrapped in the azure-storage gem and I use the gem instead. So I don't make any request in the pattern of promise and check the response. There is fair coverage for the requests in the azure-storage gem. BTW, to avoid so many hound violation before I submit the PR, do you have any suggestion on scanning the code locally instead of the hound-ci commenting on the PR? Thanks. |
def self.new_account_credential?(options = {}) | ||
@account_name != options[:azure_storage_account_name] || | ||
@account_key != options[:azure_storage_access_key] || | ||
@connection_string != options[:azure_storage_connection_string] |
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 can be azure_storage_account_name and azure_storage_access_key only. azure_storage_connection_string is not necessary.
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.
azure_storage_connection_string can be useful for specifying the default protocol or user defined endpoint.
https://azure.microsoft.com/en-us/documentation/articles/storage-configure-connection-string/
Hound is using Rubocop for static code analysis on the code. You can use rubocop gem to resolve all violations on your local machine. |
service.delete_container name, options | ||
end | ||
|
||
def self.flatten_properties(container) |
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.
Please change this function name to 'parse' instead of 'flatten_properties'. You may have seen that we have followed that naming convention in all the models.
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.
Changed. Thanks.
Please add documentation of the features you added. I will appreciate if you follow documenting convention. |
Revised. Please review the changes in README.md. I added the docs for container operation and made some changes for the style convention on the existing doc:
|
end | ||
|
||
def self.parse(container) | ||
return container unless container.key?('properties') |
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.
Your code is breaking at this line num 38: undefined method 'key?'
Because you are getting Azure::Storage::Blob::Container::Container class instance in response, not a hash.
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 catch! Thanks.
Add support for API: