-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(csv): add CSVOutput class #4315
base: master
Are you sure you want to change the base?
chore(csv): add CSVOutput class #4315
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4315 +/- ##
==========================================
+ Coverage 86.98% 87.15% +0.16%
==========================================
Files 843 843
Lines 26309 26396 +87
==========================================
+ Hits 22886 23005 +119
+ Misses 3423 3391 -32 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great piece of work @pedrooot
I left some comments since I'm thinking how to handle a complete output with all the findings and I think I'll remove all the changes in compliance since it is out of the scope of this PR.
prowler/lib/outputs/common.py
Outdated
@@ -22,78 +23,93 @@ def get_provider_data_mapping(provider) -> dict: | |||
return data | |||
|
|||
|
|||
def generate_provider_output(provider, finding, csv_data) -> FindingOutput: | |||
def generate_output(provider, finding, output_options) -> Finding: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we move this into the Finding
class?
Please also add tests and a docsting to the function to explain inputs, outputs, and what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for docstring
prowler/lib/outputs/common.py
Outdated
return finding_output | ||
|
||
|
||
def generate_provider_output(provider, finding, output_data) -> Finding: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we move this into the Finding
class?
Please also add tests and a docsting to the function to explain inputs, outputs, and what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for docstring
prowler/lib/outputs/common_models.py
Outdated
|
||
|
||
class Output(ABC): | ||
_data: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this output holds all the findings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
from prowler.lib.outputs.csv.csv import write_csv | ||
|
||
|
||
@pytest.fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a Test Class here and maybe move it to a folder with the same name as the code within Prowler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
informational = "informational" | ||
|
||
|
||
class CSVRow(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used anymore.
assert "us-west-1" in content | ||
assert "Description of the finding" in content | ||
assert "High" in content | ||
assert "http://example.com" in content |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
http://example.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this using ==
Description
It's needed to modularize the code for the specific csv output.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.