Storage Blobs - prototype API listing for comments #7373
Conversation
|
Can one of the admins verify this patch? |
| def __exit__(self, *args): ... | ||
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... |
There was a problem hiding this comment.
We should align the parameter names with the result of discussion on ETag handling.
Short term, let's make these if_* parameters keyword-only.
There was a problem hiding this comment.
This method signature has already been condensed, so maybe this was generated from out-of-date code?
https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/storage/azure-storage-blob/azure/storage/blob/aio/blob_client_async.py#L967
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def append_block(self, data, length: int=None, validate_content: bool=False, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, encoding, cpk, timeout, **kwargs) -> dict(str, Any): ... |
There was a problem hiding this comment.
should appendpos_condition be **kwonly?
There was a problem hiding this comment.
maxsize_condition can be **kwonly
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def append_block(self, data, length: int=None, validate_content: bool=False, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, encoding, cpk, timeout, **kwargs) -> dict(str, Any): ... |
There was a problem hiding this comment.
Let's make validate_content a **kwonly parameter.
| def get_service_stats(self, timeout: int=None, **kwargs) -> ~azure.storage.blob._generated.models.StorageServiceStats: ... | ||
| def get_user_delegation_key(self, key_start_time: datetime, key_expiry_time: datetime, timeout: int=None, **kwargs) -> ~azure.storage.blob._shared.models.UserDelegationKey: ... | ||
| def list_containers(self, name_starts_with: str=None, include_metadata: bool=False, results_per_page: int=None, timeout: int=None, **kwargs) -> ~azure.core.async_paging.AsyncItemPaged[~azure.storage.blob.models.ContainerProperties]: ... | ||
| def set_service_properties(self, logging: logging=None, hour_metrics: hour_metrics=None, minute_metrics: minute_metrics=None, cors: cors=None, target_version: str=None, delete_retention_policy: delete_retention_policy=None, static_website: static_website=None, timeout: int=None, **kwargs) -> None: ... |
There was a problem hiding this comment.
The parameter name logging is somewhat unfortunate (since it is not related to the standard logging module in any way)
| def stage_block_from_url(self, block_id: str, source_url, source_offset=None, source_length=None, source_content_md5: bytearray=None, *, lease, cpk, timeout, **kwargs) -> None: ... | ||
| def start_copy_from_url(self, source_url: str, metadata: metadata=None, incremental_copy: bool=False, *, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, destination_if_modified_since, destination_if_unmodified_since, destination_if_match, destination_if_none_match, destination_lease, source_lease, timeout, premium_page_blob_tier, requires_sync, **kwargs) -> Dict[str, Union[str, datetime]]: ... | ||
| def undelete_blob(self, *, timeout, **kwargs) -> None: ... | ||
| def upload_blob(self, data, blob_type: ~azure.storage.blob.models.BlobType=BlockBlob, overwrite: bool=False, length: int=None, metadata: metadata=None, content_settings: ~azure.storage.blob.models.ContentSettings=None, validate_content: bool=False, max_connections: int=1, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, maxsize_condition, cpk, encoding, timeout, **kwargs) -> dict[str, Any]: ... |
There was a problem hiding this comment.
Should upload overwrite an existing blob by default?
Currently this throws
RequestId:57d5ae44-401e-00e7-3929-73f5dc000000
Time:2019-09-24T22:42:22.4225937Z
ErrorCode:BlobAlreadyExists
Error:None
while in .Net it simply overwrites...not sure which way to go..but i believe its good to make it consistent,
Currently to overwrite a blob in python overwrite=True is passed
There was a problem hiding this comment.
We should have same behaviour across languages - also part of the etag conversation.
More discussion offline.
| def serialize(self, keep_readonly: bool=False) -> dict: ... | ||
| def validate(self) -> list: ... | ||
|
|
||
| class AccountPermissions(object): |
There was a problem hiding this comment.
In the other languages, we appear to include Sas (or similar) as part of the type-name. Should we do the same here and for the other "Permissions" types here?
| def __add__(self, other): ... | ||
| def __init__(self, read: bool=False, write: bool=False, delete: bool=False, list: bool=False, add: bool=False, create: bool=False, update: bool=False, process: bool=False, _str: str=None, *, AccountPermissions.ADD, AccountPermissions.CREATE, AccountPermissions.DELETE, AccountPermissions.LIST, AccountPermissions.PROCESS, AccountPermissions.READ, AccountPermissions.UPDATE, AccountPermissions.WRITE): ... | ||
| def __or__(self, other): ... | ||
| def __str__(self): ... |
There was a problem hiding this comment.
In the other languages, there is parse like method that allows you to go from a permission string (e.g "rld") to an object representing the permission. Should we have a similar one here (or is there some other way to go from these strings to permissions?)
There was a problem hiding this comment.
@rakshith91 , can you verify that all the consumers of this class takes a string. And please add a suggestion/issue to support a from_string class method.
There was a problem hiding this comment.
Needs to be non-lossy between versions - more consideration needed
| def create_append_blob(self, content_settings: ~azure.storage.blob.models.ContentSettings=None, metadata: metadata=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def create_page_blob(self, size: int, content_settings: ~azure.storage.blob.models.ContentSettings=None, sequence_number: int=None, metadata: metadata=None, premium_page_blob_tier: ~azure.storage.blob.models.PremiumPageBlobTier=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def create_snapshot(self, metadata: metadata=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, lease, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def delete_blob(self, delete_snapshots: str=False, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> None: ... |
There was a problem hiding this comment.
Why is delete_snapshots ' a string? I think it should be bool.
EDIT: seems like a typo here.
| def create_page_blob(self, size: int, content_settings: ~azure.storage.blob.models.ContentSettings=None, sequence_number: int=None, metadata: metadata=None, premium_page_blob_tier: ~azure.storage.blob.models.PremiumPageBlobTier=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def create_snapshot(self, metadata: metadata=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, lease, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def delete_blob(self, delete_snapshots: str=False, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> None: ... | ||
| def download_blob(self, offset: int=None, length: int=None, validate_content: bool=False, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> ~azure.storage.blob._blob_utils.StorageStreamDownloader: ... |
There was a problem hiding this comment.
The name length is not accurate (and misguiding). It should be 'range_end' ?
For example `download_blob(offset=5, length=5) returns 1 byte, not 5.
There was a problem hiding this comment.
We should move some of the options currently passed into the StreamDownloader, specifically max_connections up to this functions. Not sure if it should be keyword only or regular - either was consistent with upload.
There was a problem hiding this comment.
@rakshith91 FYI I opened #7061 about this last month (very confusing, as it is)
There was a problem hiding this comment.
Breaking change: Also, max_connections should be renamed to max_concurrency
| start:start | ||
|
|
||
| def __eq__(self, other): ... | ||
| def __init__(self, permission: permission=None, expiry: expiry=None, start: start=None): ... |
There was a problem hiding this comment.
@rakshith91, what is the semantics if you create an empty AccessPolicy (i.e. don't specify any parameters to the constructor)
There was a problem hiding this comment.
Default permission is None, same with start and expiry
There was a problem hiding this comment.
I believe permission shouldn't be optional
| @@ -0,0 +1,591 @@ | |||
| class AccessPolicy(Model, object): | |||
There was a problem hiding this comment.
@rakshith91, should this inherit Model or contain a model?
There was a problem hiding this comment.
AccessPolicy just exposes the generated AccessPolicy model which in turn inherits from Model.
I think its fine.
| _str:str | ||
|
|
||
| def __add__(self, other): ... | ||
| def __init__(self, read: bool=False, write: bool=False, delete: bool=False, list: bool=False, add: bool=False, create: bool=False, update: bool=False, process: bool=False, _str: str=None, *, AccountPermissions.ADD, AccountPermissions.CREATE, AccountPermissions.DELETE, AccountPermissions.LIST, AccountPermissions.PROCESS, AccountPermissions.READ, AccountPermissions.UPDATE, AccountPermissions.WRITE): ... |
There was a problem hiding this comment.
Breaking change: I would suggest removing the "enum-like" list parameters. @rakshith91, please open an issue.
There was a problem hiding this comment.
note: similar for blobpermissions
| _str:str | ||
|
|
||
| def __add__(self, other): ... | ||
| def __init__(self, read: bool=False, write: bool=False, delete: bool=False, list: bool=False, add: bool=False, create: bool=False, update: bool=False, process: bool=False, _str: str=None, *, AccountPermissions.ADD, AccountPermissions.CREATE, AccountPermissions.DELETE, AccountPermissions.LIST, AccountPermissions.PROCESS, AccountPermissions.READ, AccountPermissions.UPDATE, AccountPermissions.WRITE): ... |
There was a problem hiding this comment.
Breaking change: Please remove the _str constructor parameter. Internal positional parameters are pure evil.
| process:bool | ||
| _str:str | ||
|
|
||
| def __add__(self, other): ... |
There was a problem hiding this comment.
Breaking change: @rakshith91, what does adding two AccountPermissions do? And what is the difference between __add__ and __or__ (in the implementation of this class). Unless we are doing something sane here, please remove.
There was a problem hiding this comment.
adding two AccountPermissions will give you both permissons
ex:
a = AccountPermissions.READ (read =True)
b = AccountPermissions.WRITE (write=True)
=> a + b = AccountPermissions.READ + AccountPermissions.WRITE (read=True, write = True)
There was a problem hiding this comment.
We can remove these since we are removing the enum like params
| state:str | ||
| size:int | ||
|
|
||
| def __init__(self, block_id: str=None, state: str=Latest, *, size): ... |
There was a problem hiding this comment.
Breaking change:
@rakshith91, what happens if I don't provide a block_id? I very much assume it is required.
There was a problem hiding this comment.
It is required.
It will eventually be passed into encode_base64(str(block_id)) which encode the string 'None'
| def __aexit__(self, *args): ... | ||
| def __enter__(self): ... | ||
| def __exit__(self, *args): ... | ||
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... |
There was a problem hiding this comment.
The blob and container parameters should only take strings.
We should move the handling of the BlobProperties.name and ContainerProperties.name to the parent get_client functions
There was a problem hiding this comment.
Make the construct parameters non-optional:
def __init__(self, account_url, container, blob, snapshot=None, credential=None, **kwargs)
Then add a classmethod:
@classmethod
def from_url(blob_url, snapshot=None, credential=None, **kwargs)
There was a problem hiding this comment.
Move loop into **kwargs and verify that it's being passed through to the creation of the transport.
There was a problem hiding this comment.
If blob and container are strings, they should be renamed to blob_name and container_name
This also applies to the from_connection_string classmethod.
| def __enter__(self): ... | ||
| def __exit__(self, *args): ... | ||
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... |
There was a problem hiding this comment.
copy_id can be a dict, str, or BlobProperties
| def __exit__(self, *args): ... | ||
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... |
There was a problem hiding this comment.
should lease_id be proposed_lease_id? Compare with other languages
| def __init__(self, blob_url: str, container: container=None, blob: blob=None, snapshot: str=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def append_block(self, data, length: int=None, validate_content: bool=False, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, encoding, cpk, timeout, **kwargs) -> dict(str, Any): ... |
There was a problem hiding this comment.
data can be type: Union[Anystr, Iterable[AnyStr], IO[AnyStr]]
| def abort_copy(self, copy_id: copy_id, **kwargs) -> None: ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def append_block(self, data, length: int=None, validate_content: bool=False, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, encoding, cpk, timeout, **kwargs) -> dict(str, Any): ... | ||
| def append_block_from_url(self, copy_source_url: str, source_range_start: int=None, source_range_end: int=None, source_content_md5: bytearray=None, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, cpk, timeout, **kwargs): ... |
There was a problem hiding this comment.
source_content_md5, maxsize_condition, appendpos_condition can all be **kwonly
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def append_block(self, data, length: int=None, validate_content: bool=False, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, encoding, cpk, timeout, **kwargs) -> dict(str, Any): ... | ||
| def append_block_from_url(self, copy_source_url: str, source_range_start: int=None, source_range_end: int=None, source_content_md5: bytearray=None, maxsize_condition: int=None, appendpos_condition: int=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, cpk, timeout, **kwargs): ... | ||
| def clear_page(self, start_range: int, end_range: int, *, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, timeout, **kwargs) -> dict(str, Any): ... |
There was a problem hiding this comment.
rename start_range and end_range to offset and length?
also it seems like in some APIs, offset and length are behaving like start_range and end_range
#7061
cc: @annatisch @johanste
There was a problem hiding this comment.
start_offset and end_offset? Index? Inclusive values? offset and length for everything and be consistent!
"Page ranges" is page blob terminology.
More discussion needed here..... also check our behaviour matches terminology for length
| def clear_page(self, start_range: int, end_range: int, *, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, timeout, **kwargs) -> dict(str, Any): ... | ||
| def commit_block_list(self, block_list: list, content_settings: ~azure.storage.blob.models.ContentSettings=None, metadata: metadata=None, validate_content: bool=False, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict(str, Any): ... | ||
| def create_append_blob(self, content_settings: ~azure.storage.blob.models.ContentSettings=None, metadata: metadata=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def create_page_blob(self, size: int, content_settings: ~azure.storage.blob.models.ContentSettings=None, sequence_number: int=None, metadata: metadata=None, premium_page_blob_tier: ~azure.storage.blob.models.PremiumPageBlobTier=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> dict[str, Any]: ... |
There was a problem hiding this comment.
sequence_number can be moved to **kwonly
| def __len__(self): ... | ||
| def content_as_bytes(self, max_connections: int=1) -> bytes: ... | ||
| def content_as_text(self, max_connections: int=1, encoding=UTF-8) -> str: ... | ||
| def download_to_stream(self, stream, max_connections=1) -> Any: ... |
There was a problem hiding this comment.
We should rewrite this API to match the Python stream interface:
https://docs.python.org/3/library/io.html
content_as_bytes => readall (with optional encoding parameter to decode to str)
download_to_stream -> readinto
add read(size)
There was a problem hiding this comment.
@johanste - you mentioned supporting a open and close interface here - what do you envision that translating to in terms service calls?
There was a problem hiding this comment.
Also - do you believe we should remove the iterable functionality of this object entirely?
Currently a file-like object can be read iterably
| def download_blob(self, offset: int=None, length: int=None, validate_content: bool=False, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> ~azure.storage.blob._blob_utils.StorageStreamDownloader: ... | ||
| @classmethod | ||
| def from_connection_string(cls, conn_str: str, container: container, blob: blob, snapshot: str=None, credential=None, **kwargs): ... | ||
| def generate_shared_access_signature(self, permission: permission=None, expiry: expiry=None, start: start=None, policy_id: str=None, ip: str=None, protocol: str=None, account_name: str=None, cache_control: str=None, content_disposition: str=None, content_encoding: str=None, content_language: str=None, content_type: str=None, user_delegation_key: ~azure.storage.blob._shared.models.UserDelegationKey=None) -> str: ... |
There was a problem hiding this comment.
account_name, cache_control, protocol, all content_* can be moved to **kwonly
There was a problem hiding this comment.
Compare to simplest overloads in .Net/Java?
| def generate_shared_access_signature(self, permission: permission=None, expiry: expiry=None, start: start=None, policy_id: str=None, ip: str=None, protocol: str=None, account_name: str=None, cache_control: str=None, content_disposition: str=None, content_encoding: str=None, content_language: str=None, content_type: str=None, user_delegation_key: ~azure.storage.blob._shared.models.UserDelegationKey=None) -> str: ... | ||
| def get_account_information(self, **kwargs) -> dict(str, str): ... | ||
| def get_blob_properties(self, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> ~azure.storage.blob.models.BlobProperties: ... | ||
| def get_block_list(self, block_list_type: str=committed, *, lease, timeout, **kwargs) -> tuple(list(~azure.storage.blob.models.BlobBlock), list(~azure.storage.blob.models.BlobBlock)): ... |
There was a problem hiding this comment.
breaking change: return a single list instead of two separate lists for committed and un-committed blocks.
There was a problem hiding this comment.
Further discussion. Low traffic anyway.
| def get_account_information(self, **kwargs) -> dict(str, str): ... | ||
| def get_blob_properties(self, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs) -> ~azure.storage.blob.models.BlobProperties: ... | ||
| def get_block_list(self, block_list_type: str=committed, *, lease, timeout, **kwargs) -> tuple(list(~azure.storage.blob.models.BlobBlock), list(~azure.storage.blob.models.BlobBlock)): ... | ||
| def get_page_ranges(self, start_range: int=None, end_range: int=None, previous_snapshot_diff: str=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> tuple(list(dict(str, str), list(dict(str, str)): ... |
There was a problem hiding this comment.
confirm behavior of end_range and start_range vs the behavior of offset and length.
- Check with other languages
- Make it consistent
There was a problem hiding this comment.
previous_snapshot_diff can be **kwonly
There was a problem hiding this comment.
breaking change:
Make it similar to list_blob_blocks.
- Create a PageRange model similar to
BlobBlock, which specifiesclear == True|False - Return a single list instead of tuple
| def get_page_ranges(self, start_range: int=None, end_range: int=None, previous_snapshot_diff: str=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> tuple(list(dict(str, str), list(dict(str, str)): ... | ||
| def resize_blob(self, size: int, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, timeout, **kwargs) -> dict(str, Any): ... | ||
| def set_blob_metadata(self, metadata: metadata=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs): ... | ||
| def set_http_headers(self, content_settings: ~azure.storage.blob.models.ContentSettings=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> Dict[str, Any]: ... |
There was a problem hiding this comment.
content_settings should be **kwonly
| def set_sequence_number(self, sequence_number_action: str, sequence_number: str=None, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, timeout, **kwargs) -> dict(str, Any): ... | ||
| def set_standard_blob_tier(self, standard_blob_tier: standard_blob_tier, *, timeout, lease, **kwargs) -> None: ... | ||
| def stage_block(self, block_id: str, data, length: int=None, validate_content: bool=False, *, lease, encoding, cpk, timeout, **kwargs) -> None: ... | ||
| def stage_block_from_url(self, block_id: str, source_url, source_offset=None, source_length=None, source_content_md5: bytearray=None, *, lease, cpk, timeout, **kwargs) -> None: ... |
There was a problem hiding this comment.
double check that offset and length aren't acting like start_range and end_range
| def stage_block(self, block_id: str, data, length: int=None, validate_content: bool=False, *, lease, encoding, cpk, timeout, **kwargs) -> None: ... | ||
| def stage_block_from_url(self, block_id: str, source_url, source_offset=None, source_length=None, source_content_md5: bytearray=None, *, lease, cpk, timeout, **kwargs) -> None: ... | ||
| def start_copy_from_url(self, source_url: str, metadata: metadata=None, incremental_copy: bool=False, *, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, destination_if_modified_since, destination_if_unmodified_since, destination_if_match, destination_if_none_match, destination_lease, source_lease, timeout, premium_page_blob_tier, requires_sync, **kwargs) -> Dict[str, Union[str, datetime]]: ... | ||
| def undelete_blob(self, *, timeout, **kwargs) -> None: ... |
There was a problem hiding this comment.
nitpick: undelete sounds odd..,may rename to restore_blob?
There was a problem hiding this comment.
Keyvault uses "recover" and "soft delete"
There was a problem hiding this comment.
Consider recover as in keyvault.
| def start_copy_from_url(self, source_url: str, metadata: metadata=None, incremental_copy: bool=False, *, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, destination_if_modified_since, destination_if_unmodified_since, destination_if_match, destination_if_none_match, destination_lease, source_lease, timeout, premium_page_blob_tier, requires_sync, **kwargs) -> Dict[str, Union[str, datetime]]: ... | ||
| def undelete_blob(self, *, timeout, **kwargs) -> None: ... | ||
| def upload_blob(self, data, blob_type: ~azure.storage.blob.models.BlobType=BlockBlob, overwrite: bool=False, length: int=None, metadata: metadata=None, content_settings: ~azure.storage.blob.models.ContentSettings=None, validate_content: bool=False, max_connections: int=1, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, maxsize_condition, cpk, encoding, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def upload_page(self, page: bytes, start_range: int, end_range: int, length: int=None, validate_content: bool=False, *, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, encoding, timeout, **kwargs) -> dict(str, Any): ... |
There was a problem hiding this comment.
startrange and end_range should be offset and length
There was a problem hiding this comment.
not sure why length is needed?
| def undelete_blob(self, *, timeout, **kwargs) -> None: ... | ||
| def upload_blob(self, data, blob_type: ~azure.storage.blob.models.BlobType=BlockBlob, overwrite: bool=False, length: int=None, metadata: metadata=None, content_settings: ~azure.storage.blob.models.ContentSettings=None, validate_content: bool=False, max_connections: int=1, *, lease, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, maxsize_condition, cpk, encoding, timeout, **kwargs) -> dict[str, Any]: ... | ||
| def upload_page(self, page: bytes, start_range: int, end_range: int, length: int=None, validate_content: bool=False, *, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, encoding, timeout, **kwargs) -> dict(str, Any): ... | ||
| def upload_pages_from_url(self, source_url: str, range_start: int, range_end: int, source_range_start: int, source_content_md5: bytes=None, *, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs): ... |
There was a problem hiding this comment.
source_content_md5 can be **kwonly
| def upload_page(self, page: bytes, start_range: int, end_range: int, length: int=None, validate_content: bool=False, *, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, encoding, timeout, **kwargs) -> dict(str, Any): ... | ||
| def upload_pages_from_url(self, source_url: str, range_start: int, range_end: int, source_range_start: int, source_content_md5: bytes=None, *, source_if_modified_since, source_if_unmodified_since, source_if_match, source_if_none_match, lease, if_sequence_number_lte, if_sequence_number_lt, if_sequence_number_eq, if_modified_since, if_unmodified_since, if_match, if_none_match, cpk, timeout, **kwargs): ... | ||
|
|
||
| class BlobPermissions(object): |
There was a problem hiding this comment.
breaking change: I would suggest removing the "enum-like" list parameters.
|
|
||
| def __add__(self, other): ... | ||
| def __init__(self, read: bool=False, add: bool=False, create: bool=False, write: bool=False, delete: bool=False, _str: str=None, *, BlobPermissions.ADD, BlobPermissions.CREATE, BlobPermissions.DELETE, BlobPermissions.READ, BlobPermissions.WRITE): ... | ||
| def __or__(self, other): ... |
There was a problem hiding this comment.
Add and or are doing the same thing...check the behaviour and remove/modify them
| def __aexit__(self, *args): ... | ||
| def __enter__(self): ... | ||
| def __exit__(self, *args): ... | ||
| def __init__(self, account_url: str, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... |
There was a problem hiding this comment.
I would like a method to initialize using account_name and credential instead of account_url.
There was a problem hiding this comment.
apparent security issues
| def __enter__(self): ... | ||
| def __exit__(self, *args): ... | ||
| def __init__(self, account_url: str, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def create_container(self, name: str, metadata: metadata=None, public_access: public_access=None, timeout: int=None, **kwargs) -> ~azure.storage.blob.aio.container_client_async.ContainerClient: ... |
There was a problem hiding this comment.
should timeout be **kwonly
| def from_connection_string(cls, conn_str: str, credential=None, **kwargs): ... | ||
| def generate_shared_access_signature(self, resource_types: resource_types, permission: permission, expiry: expiry, start: start=None, ip: str=None, protocol: str=None) -> str: ... | ||
| def get_account_information(self, **kwargs) -> dict(str, str): ... | ||
| def get_blob_client(self, container: container, blob: blob, snapshot: snapshot=None) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... |
There was a problem hiding this comment.
this should be changed to accept only string for container (which should be renamed to container_name).
| def __exit__(self, *args): ... | ||
| def __init__(self, account_url: str, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def create_container(self, name: str, metadata: metadata=None, public_access: public_access=None, timeout: int=None, **kwargs) -> ~azure.storage.blob.aio.container_client_async.ContainerClient: ... | ||
| def delete_container(self, container: container, lease: ~azure.storage.blob.lease.LeaseClient=None, timeout: int=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, **kwargs) -> None: ... |
There was a problem hiding this comment.
container should only be string and not ContainerProperties.
Also should be renamed to container_name
| def get_blob_client(self, container: container, blob: blob, snapshot: snapshot=None) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... | ||
| def get_container_client(self, container: container) -> ~azure.core.blob.aio.container_client_async.ContainerClient: ... | ||
| def get_service_properties(self, timeout: int=None, **kwargs) -> ~azure.storage.blob._generated.models.StorageServiceProperties: ... | ||
| def get_service_stats(self, timeout: int=None, **kwargs) -> ~azure.storage.blob._generated.models.StorageServiceStats: ... |
There was a problem hiding this comment.
timeout in all bsc related APIs should be **kwonly.
| def __aexit__(self, *args): ... | ||
| def __enter__(self): ... | ||
| def __exit__(self, *args): ... | ||
| def __init__(self, container_url: str, container: container=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... |
There was a problem hiding this comment.
container should only be string and renamed to container_name.
| def __init__(self, container_url: str, container: container=None, credential=None, loop=None, *, url, primary_endpoint, primary_hostname, secondary_endpoint, secondary_hostname, location_mode, **kwargs): ... | ||
| def acquire_lease(self, lease_duration: int=-1, lease_id: str=None, timeout: int=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, **kwargs) -> ~azure.storage.blob.aio.lease_async.LeaseClient: ... | ||
| def create_container(self, metadata: metadata=None, public_access: ~azure.storage.blob.models.PublicAccess=None, timeout: int=None, **kwargs) -> None: ... | ||
| def delete_blob(self, blob: blob, delete_snapshots: str=None, lease: lease=None, timeout: int=None, *, if_modified_since, if_unmodified_since, if_match, if_none_match, **kwargs) -> None: ... |
There was a problem hiding this comment.
blob should be str only
| def from_connection_string(cls, conn_str: str, container: container, credential=None, **kwargs): ... | ||
| def generate_shared_access_signature(self, permission: permission=None, expiry: expiry=None, start: start=None, policy_id: str=None, ip: str=None, protocol: str=None, account_name: str=None, cache_control: str=None, content_disposition: str=None, content_encoding: str=None, content_language: str=None, content_type: str=None, user_delegation_key: ~azure.storage.blob._shared.models.UserDelegationKey=None) -> str: ... | ||
| def get_account_information(self, **kwargs) -> dict(str, str): ... | ||
| def get_blob_client(self, blob: blob, snapshot: str=None) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... |
There was a problem hiding this comment.
blob should be str only, changed to blob_name
| def get_account_information(self, **kwargs) -> dict(str, str): ... | ||
| def get_blob_client(self, blob: blob, snapshot: str=None) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... | ||
| def get_container_access_policy(self, lease: str=None, timeout: int=None, **kwargs) -> dict[str, str]: ... | ||
| def get_container_properties(self, lease: ~azure.storage.blob.aio.lease_async.LeaseClient=None, timeout: int=None, **kwargs) -> ~azure.storage.blob.models.ContainerProperties: ... |
There was a problem hiding this comment.
timeout should be **kwonly
| def get_blob_client(self, blob: blob, snapshot: str=None) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... | ||
| def get_container_access_policy(self, lease: str=None, timeout: int=None, **kwargs) -> dict[str, str]: ... | ||
| def get_container_properties(self, lease: ~azure.storage.blob.aio.lease_async.LeaseClient=None, timeout: int=None, **kwargs) -> ~azure.storage.blob.models.ContainerProperties: ... | ||
| def list_blobs(self, name_starts_with: str=None, include: list[str]=None, timeout: int=None, **kwargs) -> ~azure.core.async_paging.AsyncItemPaged[~azure.storage.blob.models.BlobProperties]: ... |
There was a problem hiding this comment.
include can be both string and a list.
I think timeout in all APIs should be **kwonly
| def get_container_properties(self, lease: ~azure.storage.blob.aio.lease_async.LeaseClient=None, timeout: int=None, **kwargs) -> ~azure.storage.blob.models.ContainerProperties: ... | ||
| def list_blobs(self, name_starts_with: str=None, include: list[str]=None, timeout: int=None, **kwargs) -> ~azure.core.async_paging.AsyncItemPaged[~azure.storage.blob.models.BlobProperties]: ... | ||
| def set_container_access_policy(self, signed_identifiers: signed_identifiers=None, public_access: ~azure.storage.blob.models.PublicAccess=None, lease: lease=None, timeout: int=None, *, if_modified_since, if_unmodified_since, **kwargs): ... | ||
| def set_container_metadata(self, metadata: metadata=None, lease: str=None, timeout: int=None, *, if_modified_since, **kwargs): ... |
There was a problem hiding this comment.
should lease be **kwonly in all the APIs
| def list_blobs(self, name_starts_with: str=None, include: list[str]=None, timeout: int=None, **kwargs) -> ~azure.core.async_paging.AsyncItemPaged[~azure.storage.blob.models.BlobProperties]: ... | ||
| def set_container_access_policy(self, signed_identifiers: signed_identifiers=None, public_access: ~azure.storage.blob.models.PublicAccess=None, lease: lease=None, timeout: int=None, *, if_modified_since, if_unmodified_since, **kwargs): ... | ||
| def set_container_metadata(self, metadata: metadata=None, lease: str=None, timeout: int=None, *, if_modified_since, **kwargs): ... | ||
| def upload_blob(self, name: name, data, blob_type: ~azure.storage.blob.models.BlobType=BlockBlob, overwrite: bool=False, length: int=None, metadata: metadata=None, content_settings: ~azure.storage.blob.models.ContentSettings=None, validate_content: bool=False, lease: lease=None, timeout: int=None, max_connections: int=1, encoding: str=UTF-8, *, if_modified_since, if_unmodified_since, if_match, if_none_match, premium_page_blob_tier, maxsize_condition, **kwargs) -> ~azure.storage.blob.aio.blob_client_async.BlobClient: ... |
There was a problem hiding this comment.
rename name to blob_name
|
|
||
| def __add__(self, other): ... | ||
| def __init__(self, read: bool=False, write: bool=False, delete: bool=False, list: bool=False, _str: str=None, *, ContainerPermissions.DELETE, ContainerPermissions.LIST, ContainerPermissions.READ, ContainerPermissions.WRITE): ... | ||
| def __or__(self, other): ... |
There was a problem hiding this comment.
_add_ and __or__ are doing the same thing,
There was a problem hiding this comment.
Breaking change: remove the "enum-like" list parameters.
| def values(self): ... | ||
|
|
||
| class CorsRule(Model, object): | ||
| allowed_origins:list(str) |
There was a problem hiding this comment.
can allowed_* be Union[str, list[str]] instead of list[str]. It'll be more friendly to pass in one item.
| def send(self, request: request) -> ~azure.core.pipeline.PipelineResponse: ... | ||
| def sleep(self, settings, transport): ... | ||
|
|
||
| class PageRange(DictMixin, object): |
There was a problem hiding this comment.
add a clear = True|False to page range
| object:bool | ||
| _str:str | ||
|
|
||
| def __add__(self, other): ... |
There was a problem hiding this comment.
_add_ and _or_ are doing the same thing,
breaking change: remove enum like parameters
| _str:str | ||
|
|
||
| def __add__(self, other): ... | ||
| def __init__(self, service: bool=False, container: bool=False, object: bool=False, _str: str=None, *, ResourceTypes.CONTAINER, ResourceTypes.OBJECT, ResourceTypes.SERVICE): ... |
There was a problem hiding this comment.
object should be more explicit
|
@johanste - I'm guessing this can be closed now? |
This is the output for a prototype script for API listing for review. Let's see if it makes sense from a review perspective.
Known issues:
+ more.The alternative is to review the API using the docs. Which we will also have to do.