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

Fix blob storage regression #1110

Merged
merged 6 commits into from Sep 19, 2017

Conversation

@ldipenti
Copy link
Contributor

commented Sep 13, 2017

Azure Blob storage access on alternate cloud environments regression fix.

Description

Hello,
Here's a fix to allow using azure's blob storage on alternate environments, it seems that the regression was introduced here: 5352e71

Status

Replace this: describe the PR status. Examples:

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
@codecov-io

This comment has been minimized.

Copy link

commented Sep 13, 2017

Codecov Report

Merging #1110 into trunk will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1110      +/-   ##
==========================================
- Coverage   85.33%   85.33%   -0.01%     
==========================================
  Files         342      342              
  Lines       65695    65695              
  Branches     5856     5856              
==========================================
- Hits        56060    56058       -2     
- Misses       7250     7252       +2     
  Partials     2385     2385
Impacted Files Coverage Δ
libcloud/compute/drivers/azure_arm.py 44.68% <0%> (ø) ⬆️
libcloud/test/compute/test_ec2.py 97.74% <0%> (-0.17%) ⬇️

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 59b44a0...f33345c. Read the comment docs.

@pquentin

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

The change itself is nice! But it looks like there are unrelated commits?

@ldipenti

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2017

Yes, I'm not sure why. I made this from the same branch as my previous PR #1100, that I thought it was accepted but it appears to be closed and not merged.
That's odd, I checked the apache.org repo and IIRC I saw the changes merged there.

@pquentin

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

PR #1100 was definitely merged, but since the GitHub repository is only a mirror of the Apache repo, the accepted PRs are declined and the relevant code is pushed to the Apache repository (http://git.apache.org/libcloud.git/) which then appears on GitHub.

Please rebase your change on top of the trunk branch, and ensure you do not push any merge commits.

@Kami

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

Thanks.

The change looks good to me, but can you please add corresponding test cases to avoid such regression in the future?

@ldipenti

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2017

Thanks for your comments pquentin & Kami, I'll get back to you later addressing this suggestions.

@asfgit asfgit merged commit f33345c into apache:trunk Sep 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
asfgit pushed a commit that referenced this pull request Sep 19, 2017
asfgit pushed a commit that referenced this pull request Sep 19, 2017
@Kami

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

I went ahead, merged it and added tests (91d14ea, 2a8764b, 91ea091) so in can be included in time for v2.2.1 release.

Thanks again.

@ldipenti

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Thanks Kami!

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