Update ABSStore for compatibility with newer azure.storage.blob.
#759
Conversation
|
Hello @TomAugspurger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-06-01 19:14:09 UTC |
|
cc @tjcrone. |
TomAugspurger
changed the title
azure.storage.blob.
|
Welcome Tom 😄 Thanks for working on this! cc'd a couple of people who may be more familiar with this part of the code (and hopefully able to review) |
|
Why isn't CI running for this PR? Can I click on "Approve and run"? |
|
GitHub requires maintainer approval before running CI on PRs from first-time contributors (https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks), to prevent abuses like mining cryptocurrencies. I thought that it was a one-time thing, and John's approval for 43cc982 would apply to this entire PR, but apparently not. So you can click it as long as you trust I haven't inserted some crypto-mining code into the CI workflow :) |
|
odd error on this build: https://github.com/zarr-developers/zarr-python/pull/759/checks?check_run_id=2674084907 |
|
I merged master, in case it was just a transient issue. |
Codecov Report
@@ Coverage Diff @@
## master #759 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 28 28
Lines 10537 10562 +25
=======================================
+ Hits 10531 10556 +25
Misses 6 6
|
|
Ugh, sorry, flake8 is passing now hopefully. |
|
The coverage issue should be fixed by 382d453 (which turned up a bug in the deprecation code) |
|
Interesting. Had to approve CI to run again. Wonder if GH forgot this was ok test |
I think someone (@shikharsg maybe) has been approving the runs on each commit. Sorry about all the failing commits, but things are green now. |
|
Thanks for the reviews so far. Anything else I can do to help move this along? |
|
Yeah, I've been approving the running of tests as well. I've not yet understood when it sticks and when it doesn't. (Perhaps because several of us are doing it, it's had less rhyme or reason). I'll look into a repo setting. As for moving it along, I think it's just waiting for a review, no? Anyone in particular come to mind? |
There was a problem hiding this comment.
I think this is good to go in now @TomAugspurger @joshmoore
| @@ -11,6 +11,9 @@ Bug fixes | |||
|
|
|||
| * FSStore: default to normalize_keys=False | |||
| By :user:`Josh Moore <joshmoore>`; :issue:`755`. | |||
| * ABSStore: compatibility with ``azure.storage.python>=12`` | |||
I think it is just because Tom wasn't yet a contributor here. That should be a non-issue going forward |
This continues #620 to update the Azure
ABSStoreto be compatible with newer versions ofazure.storage.blob.In addition to @jhamman's compatibility changes, I made a few changes to make this easier to test. The previous API had users pass various pieces of information like the container name, storage account name, account key, etc. Now we just accept an instance of a
BlobServiceClient. I've deprecated the old way.I figure the less we care about the details of
azure.storage.blobthe better.I haven't attempted a compatibility layer with older versions of azure.storage.blob. IMO, it's not worth the effort. I think the common case of users passing a storage account name, a container name, and an account key will continue to work fine (with a warning).
Closes #618.
TODO: