Skip to content

Conversation

@williexu
Copy link
Contributor

@williexu williexu commented Mar 7, 2018

-The public_access property of a ContainerProperties is not being deserialized from x=ms-blob-public-access in the response
-added property to map and bumped patch version

'content-range': (None, 'content_range', _to_str),
'x-ms-blob-sequence-number': (None, 'page_blob_sequence_number', _to_int),
'x-ms-blob-committed-block-count': (None, 'append_blob_committed_block_count', _to_int),
'x-ms-blob-public-access': (None, 'public_access', _to_str),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, are you making this change because you wanted the public access info when calling get_container_properties?

Btw there's a separate API specifically for getting this property. And the deserialization was done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it doesn't make sense for the container property model to have "public_access" but not be able to deserialize it from the response.
https://github.com/Azure/azure-storage-python/blob/master/azure-storage-blob/azure/storage/blob/models.py#L48
This results in the property always having the value None

Copy link
Contributor

Choose a reason for hiding this comment

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

@williexu That's a good point. Could you please add a test+recording for this change? Thanks!

setup(
name='azure-storage-common',
version='1.1.0',
version='1.1.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not increment version numbers in PRs.

@codecov-io
Copy link

codecov-io commented Mar 7, 2018

Codecov Report

Merging #436 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   86.06%   86.04%   -0.03%     
==========================================
  Files          56       56              
  Lines        4429     4429              
  Branches      481      481              
==========================================
- Hits         3812     3811       -1     
  Misses        465      465              
- Partials      152      153       +1
Impacted Files Coverage Δ
...ge-common/azure/storage/common/_deserialization.py 81.75% <ø> (ø) ⬆️
...torage-blob/azure/storage/blob/_upload_chunking.py 71.42% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2709a0b...599d6e4. Read the comment docs.

@williexu
Copy link
Contributor Author

williexu commented Mar 8, 2018

@zezha-msft added testing, let me know if there are any other issues with this

@zezha-msft
Copy link
Contributor

@williexu looking good! Do you mind creating the PR against Dev instead of master?

@williexu williexu changed the base branch from master to dev March 9, 2018 19:59
@williexu
Copy link
Contributor Author

williexu commented Mar 9, 2018

Done, :)

@zezha-msft
Copy link
Contributor

@williexu Thanks!

@zezha-msft zezha-msft merged commit 733eff7 into Azure:dev Mar 9, 2018
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.

3 participants