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-944] Add support for private IP addresses in GCE instance creation #1107

Closed
wants to merge 10 commits into from
Closed

[LIBCLOUD-944] Add support for private IP addresses in GCE instance creation #1107

wants to merge 10 commits into from

Conversation

gmacf
Copy link
Contributor

@gmacf gmacf commented Sep 7, 2017

Add support for private IP addresses in GCE instance creation

Description

GCE driver doesn't currently allow private IPs to be specified when creating instances. Updated gce.py driver to allow the optional parameter internal_ip which can be used when creating GCE instances.

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)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 88.966% when pulling 27d79ea on vongazman:LIBCLOUD-944_private_ip into f1bff48 on apache:trunk.

@pquentin
Copy link
Contributor

Thank you @vongazman for your contribution. Great first-time GitHub contribution! The changes make perfect sense! 👍

However, this pull requests lacks tests. I realize that the existing external_ip parameter is not tested, but I believe you should add tests at least for internal_ip. You can look at the DimensionData tests to see how you can update the fixtures and test code to do that.

@codecov-io
Copy link

codecov-io commented Oct 27, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1107      +/-   ##
==========================================
+ Coverage   85.48%   85.48%   +<.01%     
==========================================
  Files         348      348              
  Lines       66443    66456      +13     
  Branches     5914     5916       +2     
==========================================
+ Hits        56796    56809      +13     
  Misses       7239     7239              
  Partials     2408     2408
Impacted Files Coverage Δ
libcloud/compute/drivers/gce.py 76.05% <100%> (+0.01%) ⬆️
libcloud/test/compute/test_gce.py 97.57% <100%> (+0.01%) ⬆️

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 5e1972e...a8276e2. Read the comment docs.

Copy link
Contributor

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you for adding tests! There's one issue left with DELETE. Thanks.

headers):
if method == 'DELETE':
body = self.fixtures.load(
'regions_us-central1_addresses_testaddress_delete.json')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you had started to test the DELETE path but ended up not to? Either add the file or remove the reference to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I removed the reference to it.

@asfgit asfgit closed this in a94f928 Oct 31, 2017
@pquentin
Copy link
Contributor

Thanks @vongazman! ✨ I've included this in trunk.

(Reworked the commits to remove the intermediate ones that would have made bisecting difficult, and signed them off as part of the standard procedure.)

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