-
-
Notifications
You must be signed in to change notification settings - Fork 329
Mutable mapping for Azure Blob #345
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
Conversation
Was missing self.container_name as an argument
Join only accepts one argument, using os.path.join(x,y) formats the string as a valid file path for us.
Quick fix for small bugs in ABSStore class in storage.py
That shouldn't be necessary. Perhaps travis is getting confused because you also have a PR open from your branch into zarr-developers:abs_store (#293). Can #293 be closed now in favour of this PR? |
@alimanfoo thanks, that worked |
I have added a small snippet to tutorial.rst, but that's still using the emulator. So we can choose whether or not to keep that. Apart from that I think this is good to go. |
@@ -24,6 +25,10 @@ matrix: | |||
dist: xenial | |||
sudo: true | |||
|
|||
before_install: | |||
- docker pull arafato/azurite |
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.
In the future, this may move to an azure
org on Docker Hub ( Azure/Azurite#94 ), but it hasn't happened yet. Happy with leaving this as is. Just raising awareness.
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.
thanks for pointing out
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.
Couple of minor comments above. Otherwise LGTM.
Thanks @jakirkham, I have addressed your comments |
Hi, |
I think some other people that have helped in the review (or have not) should give it a look to see if everything meets their expectations. |
Any further input from anyone? I'm keen to merge |
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.
This all looks good to me. Spotted a couple of very minor documentation fixes, but otherwise more than happy to merge.
Co-Authored-By: shikharsg <[email protected]>
Co-Authored-By: shikharsg <[email protected]>
Thanks all :) |
Thanks indeed, fantastic to see this go in 👏 I just realised that I missed @mzjp2 in the credits for this feature, I'll update the release notes directly. |
Thanks all! |
Thanks all! |
[Description of PR]
TODO:
tox -e docs
)