Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

add service account fields to firewall type throughout forseti #659

Merged
merged 5 commits into from
Sep 26, 2017

Conversation

RajUmadas
Copy link
Contributor

@RajUmadas RajUmadas commented Sep 19, 2017

Thanks for opening a Pull Request!

Here's a handy checklist to ensure your PR goes smoothly.

  • I signed Google's Contributor License Agreement
  • My code conforms to Google's python style.
  • My PR at a minimum doesn't decrease unit-test coverage (if applicable).
  • My PR has been functionally tested.
  • All of the unit-tests still pass.
  • Running pylint --rcfile=pylintrc passes.

These guidelines and more can be found in our contributing guidelines.

@blueandgold blueandgold self-requested a review September 19, 2017 15:07
@blueandgold
Copy link
Contributor

Looks great overall, thanks for adding this! A couple of comments:

Is there a need to also want to update the sql query here?
https://github.com/GoogleCloudPlatform/forseti-security/blob/dev/google/cloud/security/common/data_access/sql_queries/select_data.py#L102

A few unit tests for the IAP scanner looks like they need to be updated. Is that something you can tackle? Or if not, we can help.

@RajUmadas
Copy link
Contributor Author

@blueandgold thanks. Ill take a stab at the IAP tests today.

@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #659 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             dev     #659      +/-   ##
=========================================
+ Coverage   83.7%   83.71%   +<.01%     
=========================================
  Files        165      165              
  Lines       8354     8355       +1     
=========================================
+ Hits        6993     6994       +1     
  Misses      1361     1361
Impacted Files Coverage Δ
...rity/common/data_access/sql_queries/select_data.py 100% <ø> (ø) ⬆️
...ty/common/data_access/sql_queries/create_tables.py 100% <ø> (ø) ⬆️
...le/cloud/security/common/data_access/csv_writer.py 92.3% <ø> (ø) ⬆️
...le/cloud/security/common/gcp_type/firewall_rule.py 100% <100%> (ø) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #659 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             dev     #659      +/-   ##
=========================================
+ Coverage   83.7%   83.71%   +<.01%     
=========================================
  Files        165      165              
  Lines       8354     8355       +1     
=========================================
+ Hits        6993     6994       +1     
  Misses      1361     1361
Impacted Files Coverage Δ
...rity/common/data_access/sql_queries/select_data.py 100% <ø> (ø) ⬆️
...ty/common/data_access/sql_queries/create_tables.py 100% <ø> (ø) ⬆️
...le/cloud/security/common/data_access/csv_writer.py 92.3% <ø> (ø) ⬆️
...le/cloud/security/common/gcp_type/firewall_rule.py 100% <100%> (ø) ⬆️

Copy link
Contributor

@blueandgold blueandgold left a comment

Choose a reason for hiding this comment

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

Super awesome, thanks for the fixing the query and the tests!

@blueandgold blueandgold merged commit 4bc3905 into forseti-security:dev Sep 26, 2017
@blueandgold
Copy link
Contributor

PS: this should go out into this week's release. Thanks again!

@gbrindisi
Copy link
Contributor

@RajUmadas @blueandgold thanks for this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants