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-954] ec2: Allow cn-north-1 even without pricing #1127

Closed
wants to merge 4 commits into from

Conversation

pquentin
Copy link
Contributor

@pquentin pquentin commented Oct 11, 2017

ec2: Allow cn-north-1 even without pricing

Description

See https://issues.apache.org/jira/browse/LIBCLOUD-954 and 1e1d77f

@fjros and @duanshiqiang, what do you think?

Status

  • 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)

@fjros
Copy link
Contributor

fjros commented Oct 11, 2017

Thanks @pquentin, a nice trade-off :-)

Maybe this should be clearly stated in the docs, since in practice there's at least one api method (I think) not supported for the region.

@duanshiqiang
Copy link

I think this is good :)

@pquentin
Copy link
Contributor Author

@fjros Thanks. I tried a better approach: setting price to None when it's not available as with cn-north-1. Also improved the docs. Please tell me what you think. I highly appreciate your timely feedback!

@codecov-io
Copy link

Codecov Report

Merging #1127 into trunk will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1127      +/-   ##
==========================================
+ Coverage   85.45%   85.45%   +<.01%     
==========================================
  Files         346      346              
  Lines       66272    66278       +6     
  Branches     5899     5900       +1     
==========================================
+ Hits        56631    56637       +6     
  Misses       7239     7239              
  Partials     2402     2402
Impacted Files Coverage Δ
libcloud/compute/drivers/ec2.py 74.06% <100%> (+0.05%) ⬆️
libcloud/test/compute/test_ec2.py 97.91% <100%> (ø) ⬆️

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 a1c0e60...65fb59b. Read the comment docs.

@fjros
Copy link
Contributor

fjros commented Oct 12, 2017

@pquentin Great!! As far as I'm concerned, this fixes the issue.

Just a minor note... Maybe _get_size_price() should directly return None instead of raising an exception. In that way, the behavior would be consistent across the codebase. I mean, just moving the try..except block from list_sizes() to _get_size_price(). What do you think?

@pquentin
Copy link
Contributor Author

Ah, yes, that would be better! I'll do that.

@pquentin
Copy link
Contributor Author

Thinking a bit more about it, that would affect other drivers. I would not want to return None silently in those cases because we would not be alerted of an issue in tests. The change as it is currently only affects EC2.

@asfgit asfgit closed this in 820ff6f Oct 13, 2017
@pquentin
Copy link
Contributor Author

Merged in trunk.

@fjros Please fell free to open a new PR or JIRA issue if you think of a better way. Thanks again.
@duanshiqiang Can you please close your JIRA issue if that fixes your problem? Thanks.

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.

None yet

4 participants