Skip to content
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

[LIBCLOUD-1037] Add Azurite support for Azure Blob Storage driver #1278

Merged
merged 2 commits into from Jul 17, 2019

Conversation

Projects
None yet
4 participants
@c-w
Copy link
Member

commented Feb 26, 2019

Add Azurite support for Azure Blob Storage driver

Description

As described in LIBCLOUD-1037, the Azure Blob Storage driver currently doesn't support the Azurite storage emulator or Blob Storage on IoT edge.

This pull request made the changes required to support these two variants of Azure Blob Storage:

  • Protect against a missing content md5 since the Azurite emulator doesn't set this value.

  • Protect against a missing metadata section since the Azurite emulator doesn't set this value.

  • Implement routing to a specific account via URL prefix (e.g. /someaccount) instead of hostname (e.g. someaccount.blob.core.windows.net).

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks) Travis passed (build)
  • Documentation Comments inline in code and updated docs folder
  • Tests Captured Azurite XML responses
  • ICLA (required for bigger changes) Code change is small
@codecov-io

This comment has been minimized.

Copy link

commented Feb 26, 2019

Codecov Report

Merging #1278 into trunk will increase coverage by <.01%.
The diff coverage is 88.46%.

Impacted file tree graph

@@           Coverage Diff            @@
##           trunk   #1278      +/-   ##
========================================
+ Coverage   85.9%   85.9%   +<.01%     
========================================
  Files        365     365              
  Lines      75091   75112      +21     
  Branches    6854    6858       +4     
========================================
+ Hits       64504   64527      +23     
+ Misses      7823    7822       -1     
+ Partials    2764    2763       -1
Impacted Files Coverage Δ
libcloud/storage/drivers/azure_blobs.py 69.8% <87.5%> (+1.27%) ⬆️
libcloud/test/storage/test_azure_blobs.py 92% <90%> (-0.04%) ⬇️
libcloud/test/compute/test_upcloud.py 91.39% <0%> (+1.32%) ⬆️

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 15e3c3c...438e984. Read the comment docs.

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch 4 times, most recently from 5e3a5c0 to 2c69959 Feb 27, 2019

@c-w

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

Paging @jmspring who helped with libcloud + Azure pull requests in the past.

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch 2 times, most recently from b059b23 to 4cf8cfa Feb 27, 2019

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch from 4cf8cfa to 4839a33 Apr 8, 2019

@c-w

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@Kami Any chance you could review this? It would be great to land Azurite and IoT Edge Storage support in libcloud since this has been a requirement on a couple of my recent projects.

To increase confidence in the change, you can take a look at the CI run against Azure Storage which proves that there are no regressions and the CI run against IoT Edge Storage and Azurite which proves the new functionality.

Also adding @michaelperel and @cicorias to review.

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch from 4839a33 to 4200af8 May 28, 2019

@michaelperel
Copy link

left a comment

LGTM, added a few nits regarding style

Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py Outdated
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py Outdated
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py Outdated
Show resolved Hide resolved libcloud/storage/drivers/azure_blobs.py

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch 5 times, most recently from 2f9a0ae to f13729a May 29, 2019

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch from f13729a to 5421cb7 Jul 16, 2019

@c-w

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

All integration tests pass with this branch.

@Kami

Kami approved these changes Jul 16, 2019

Copy link
Member

left a comment

LGTM 👍

[LIBCLOUD-1037] Add Azurite support for Azure Blob Storage driver
This requires the following main changes:

- Protect against a missing content md5 since the Azurite emulator
  doesn't set this value.

- Protect against a missing metadata tag since the Azurite emulator
  doesn't set this value.

- Implement routing to a specific account via URL prefix (e.g. /someaccount)
  instead of hostname (e.g. someaccount.blob.core.windows.net).

@c-w c-w force-pushed the CatalystCode:1037_fix-azurite branch from 5421cb7 to 9e1dfe1 Jul 16, 2019

@c-w c-w merged commit 70830df into apache:trunk Jul 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@c-w c-w deleted the CatalystCode:1037_fix-azurite branch Jul 17, 2019

@Kami

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Congrats on the first merge 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.