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

Globally Rename find_policy_violations() to find_violations() #2172

Merged
merged 5 commits into from Nov 5, 2018

Conversation

angelsungoogle
Copy link
Contributor

@angelsungoogle angelsungoogle commented Nov 1, 2018

#1634

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.

@angelsungoogle
Copy link
Contributor Author

I tested by executing the commands below on both dev and this new branch to compare the results. There is a small section where the order of the violations are swapped. I'd like to check for your input on whether this is an allowable difference. Please see attached for the csv file of the violation outputs.
Steps:

  1. forseti inventory create
  2. forseti model create --inventory_index_id <inventory_id> <model_name>
  3. forseti model use <model_name>
  4. forseti scanner run

testResults.tar.gz

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #2172 into dev will not change coverage.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##              dev    #2172   +/-   ##
=======================================
  Coverage   88.75%   88.75%           
=======================================
  Files         173      173           
  Lines       13164    13164           
=======================================
  Hits        11684    11684           
  Misses       1480     1480
Impacted Files Coverage Δ
...loud/forseti/scanner/scanners/iam_rules_scanner.py 80.43% <0%> (ø) ⬆️
...ner/scanners/instance_network_interface_scanner.py 0% <0%> (ø) ⬆️
...forseti/scanner/scanners/cloudsql_rules_scanner.py 82.05% <0%> (ø) ⬆️
...d/forseti/scanner/scanners/bucket_rules_scanner.py 80.48% <0%> (ø) ⬆️
...oud/forseti/scanner/audit/bigquery_rules_engine.py 92.85% <100%> (ø) ⬆️
...r/audit/instance_network_interface_rules_engine.py 92.64% <100%> (ø) ⬆️
...d/forseti/scanner/audit/ke_version_rules_engine.py 88.88% <100%> (ø) ⬆️
...loud/forseti/scanner/audit/buckets_rules_engine.py 81.92% <100%> (ø) ⬆️
...orseti/scanner/scanners/forwarding_rule_scanner.py 60.97% <100%> (ø) ⬆️
...oud/forseti/scanner/audit/firewall_rules_engine.py 94.14% <100%> (ø) ⬆️
... and 11 more

@angelsungoogle
Copy link
Contributor Author

confirmed that there are no difference after sorting. The violations before and after the renaming is identical (except for violation id and timestamp)

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.

Awesome, thank you for cleaning this up! Been looking forward to this for a long time! Just one nit about removing all the TODOs.

@@ -70,8 +70,7 @@ def build_rule_book(self, global_configs=None):
self.rule_book = BigqueryRuleBook(self._load_rule_definitions())

# TODO: The naming is confusing and needs to be fixed in all scanners.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this TODO, since they will not apply anymore after you are done. :)
Can you please remove this everywhere else in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all #TODO items related to renaming are deleted. Please note that I deleted the #TODO items along with the \n line change at the end of each line. Because of this, the function definition in the next line appears to be changed in the comparison, but it is actually only moved one line up.

@angelsungoogle
Copy link
Contributor Author

Hey Henry @blueandgold, another freshly cooked revision!

@angelsungoogle angelsungoogle self-assigned this Nov 5, 2018
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.

Awesome, thanks for cleaning this up!

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.

None yet

4 participants