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-960: Expand GCE Firewall options coverage #1144

Closed
wants to merge 3 commits into from

Conversation

maxlip
Copy link
Contributor

@maxlip maxlip commented Nov 13, 2017

Expand GCE Firewall options coverage

Description

Expanding coverage for full range of GCP firewall rule options currently in GA:

  • Ingress or Egress direction
  • Allow or Deny action
  • Set Description and Priority values
  • Define source and/or targets by service account

I've preserved the existing defaults.

Issue Link: https://issues.apache.org/jira/browse/LIBCLOUD-960

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)

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #1144 into trunk will increase coverage by 0.63%.
The diff coverage is 91.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1144      +/-   ##
==========================================
+ Coverage   85.49%   86.12%   +0.63%     
==========================================
  Files         348      348              
  Lines       66579    66956     +377     
  Branches     5925     5947      +22     
==========================================
+ Hits        56924    57669     +745     
+ Misses       7247     6857     -390     
- Partials     2408     2430      +22
Impacted Files Coverage Δ
libcloud/test/compute/test_gce.py 97.68% <100%> (+0.08%) ⬆️
libcloud/compute/drivers/gce.py 76.59% <79.31%> (+0.43%) ⬆️
libcloud/storage/base.py 82.8% <0%> (-5.1%) ⬇️
libcloud/__init__.py 71.42% <0%> (-3.58%) ⬇️
libcloud/test/test_init.py 61.53% <0%> (-2.47%) ⬇️
libcloud/test/compute/test_upcloud.py 90.06% <0%> (-2.14%) ⬇️
libcloud/common/cloudstack.py 86.59% <0%> (-1.83%) ⬇️
libcloud/test/common/test_openstack_identity.py 97.94% <0%> (-0.62%) ⬇️
libcloud/test/compute/test_deployment.py 94.7% <0%> (-0.2%) ⬇️
libcloud/test/__init__.py 86.2% <0%> (-0.12%) ⬇️
... and 29 more

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 dcb0cef...ca5898a. Read the comment docs.

@pquentin
Copy link
Contributor

Thank you for your contribution!

Unfortunately the test coverage of libcloud/compute/drivers/gce.py is getting lower with the change, but we want it to be higher, so that your changes keep working in the future. As you know, this is important as this is not something that is easy to test in real conditions. See https://codecov.io/gh/apache/libcloud/pull/1144/src/libcloud/compute/drivers/gce.py#L3198 for an example of bad coverage. Thanks.

@maxlip
Copy link
Contributor Author

maxlip commented Feb 2, 2018

I think these are Travis or other issues - I didn't touch what's failing. But I added tests!

@pquentin
Copy link
Contributor

pquentin commented Feb 2, 2018

@maxlip Thanks. However you do have lint errors:

libcloud/test/compute/test_gce.py:847:29: E126 continuation line over-indented for hanging indent
libcloud/test/compute/test_gce.py:864:29: E126 continuation line over-indented for hanging indent
libcloud/test/compute/test_gce.py:881:30: E126 continuation line over-indented for hanging indent 

Rebasing on top of trunk would solve the paramiko issue and would allow me to see the coverage easily. Do you think you could do that? Thanks!

@pquentin
Copy link
Contributor

pquentin commented Feb 2, 2018

Sorry, I realized that the paramiko failure is not your fault. You only have the lint to fix. Thanks.

@maxlip
Copy link
Contributor Author

maxlip commented Feb 15, 2018

Just making sure it's clear that linting looks fixed - CI is now all good.
Thanks for your patience with my misreading of CI output :) I can't ever seem to get this tox suite to work on my local, so it can be a challenge.

@@ -8215,7 +8284,7 @@ def _to_firewall(self, firewall):
"""
Return a Firewall object from the JSON-response dictionary.

:param firewall: The dictionary describing the firewall.
:param firewall: The dictionary deFscribing the firewall.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an extra F here. I'll remove it.

@asfgit asfgit closed this in 85180ab Feb 25, 2018
@pquentin
Copy link
Contributor

Thank you @maxlip! Merged in trunk! ✨

I've credited you as "maxlip". Would you like me to use something else?

@maxlip maxlip deleted the LIBCLOUD-960_GCEFW-4Way-SvcAcct branch February 26, 2018 17:56
@maxlip
Copy link
Contributor Author

maxlip commented Feb 26, 2018

Thanks for fixing that Quentin - "maxlip" is good for me.

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

3 participants