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

Allow user to specify the violation payload format (csv / json) #1301

Merged
merged 22 commits into from
Mar 27, 2018
Merged

Allow user to specify the violation payload format (csv / json) #1301

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
fe71b1a
Added test for GCS upload with json/csv respectively
arbeit Mar 24, 2018
5f717a4
pylint fixes
arbeit Mar 26, 2018
3c63f37
implement csv/json uploads, fix tests/test data
arbeit Mar 26, 2018
769018c
minor fix
arbeit Mar 26, 2018
0aac45c
test for invalid data format
arbeit Mar 26, 2018
1f7b29b
Make _get_output_filename() observe the data format
arbeit Mar 26, 2018
28ee1c4
Make _get_output_filename() simpler
arbeit Mar 26, 2018
b5b92f8
Move _get_output_filename() into the base class
arbeit Mar 26, 2018
01c8b27
Use the base class _get_output_filename() method
arbeit Mar 26, 2018
59a595c
implement csv/json format; fix tests & test data
arbeit Mar 26, 2018
7f61d0c
pylint fix
arbeit Mar 26, 2018
368df57
Better test coverage
arbeit Mar 26, 2018
5af92d2
better naming
arbeit Mar 26, 2018
cecf5c9
Merge branch '2.0-dev' into cvs-or-json-1287
arbeit Mar 26, 2018
3119daf
review comments, batch #1
arbeit Mar 26, 2018
31f1175
review comments, batch #2
arbeit Mar 26, 2018
9af2655
fix custom exception class
arbeit Mar 26, 2018
56a75b4
review comments, batch #3
arbeit Mar 26, 2018
b6eef3a
minor fix
arbeit Mar 26, 2018
17d11fa
Remove redundant error log
arbeit Mar 26, 2018
210e9c7
Update email_violations.py
blueandgold Mar 26, 2018
b8c50cb
Merge branch '2.0-dev' into cvs-or-json-1287
blueandgold Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions google/cloud/forseti/notifier/notifiers/base_notification.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

import abc

from google.cloud.forseti.common.util import date_time
from google.cloud.forseti.common.util import logger
from google.cloud.forseti.common.util import string_formats

LOGGER = logger.get_logger(__name__)

Expand Down Expand Up @@ -55,3 +57,20 @@ def __init__(self, resource, cycle_timestamp,
def run(self):
"""Runs the notifier."""
pass

def _get_output_filename(self, filename_template):
"""Create the output filename.

Args:
filename_template (string): template to use for the output filename

Returns:
str: The output filename for the violations CSV file.
"""
now_utc = date_time.get_utc_now_datetime()
output_timestamp = now_utc.strftime(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you should just call this variable, utc_now_datetime since that is what you got back from above, and also shows why strftime can be called here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

string_formats.TIMESTAMP_TIMEZONE_FILES)

output_filename = filename_template.format(
self.resource, self.cycle_timestamp, output_timestamp)
return output_filename
69 changes: 44 additions & 25 deletions google/cloud/forseti/notifier/notifiers/email_violations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@

"""Email notifier to perform notifications"""

import tempfile

from google.cloud.forseti.common.data_access import csv_writer
from google.cloud.forseti.common.util import email
from google.cloud.forseti.common.util import date_time
from google.cloud.forseti.common.util import email
from google.cloud.forseti.common.util import errors as util_errors
from google.cloud.forseti.common.util import logger
from google.cloud.forseti.common.util import parser
from google.cloud.forseti.common.util import string_formats
from google.cloud.forseti.notifier.notifiers import base_notification

Expand Down Expand Up @@ -52,28 +55,14 @@ def __init__(self, resource, cycle_timestamp,
self.mail_util = email.EmailUtil(
self.notification_config['sendgrid_api_key'])

def _get_output_filename(self):
"""Create the output filename.

Returns:
str: The output filename for the violations json.
"""
now_utc = date_time.get_utc_now_datetime()
output_timestamp = now_utc.strftime(
string_formats.TIMESTAMP_TIMEZONE_FILES)
output_filename = string_formats.VIOLATION_CSV_FMT.format(
self.resource,
self.cycle_timestamp,
output_timestamp)
return output_filename

def _make_attachment(self):
"""Create the attachment object.
def _make_attachment_csv(self):
"""Create the attachment object in csv format.

Returns:
attachment: SendGrid attachment object.
"""
output_file_name = self._get_output_filename()
output_file_name = self._get_output_filename(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: be consistent whether you want to use file_name or filename

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

string_formats.VIOLATION_CSV_FMT)
with csv_writer.write_csv(resource_name='violations',
data=self.violations,
write_header=True) as csv_file:
Expand All @@ -86,6 +75,25 @@ def _make_attachment(self):

return attachment

def _make_attachment_json(self):
"""Create the attachment object json format.

Returns:
attachment: SendGrid attachment object.
"""
output_file_name = self._get_output_filename(
string_formats.VIOLATION_JSON_FMT)
with tempfile.NamedTemporaryFile() as tmp_violations:
tmp_violations.write(parser.json_stringify(self.violations))
tmp_violations.flush()
LOGGER.info('JSON filename: %s', tmp_violations.name)
attachment = self.mail_util.create_attachment(
file_location=tmp_violations.name,
content_type='text/csv', filename=output_file_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the content_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you put filename=output_file_name, on a new line? Just to be consistent with the indentings here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

content_id='Violations')

return attachment

def _make_content(self):
"""Create the email content.

Expand Down Expand Up @@ -121,11 +129,21 @@ def _compose(self, **kwargs):

email_map = {}

attachment = self._make_attachment()
subject, content = self._make_content()
email_map['subject'] = subject
email_map['content'] = content
email_map['attachment'] = attachment
data_format = self.notification_config.get('data_format', 'csv')
if data_format not in ['json', 'csv']:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to suggest breaking out ['json', 'csv'] into a separate variable like supported_data_format, then it's easy to maintain and read.

supported_data_format = ['json', 'csv']
if data_format not in supported_data_format:
    blah blah blah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

LOGGER.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nicer to raise a custom exception here. Perhaps something like UnsupportedNotificationDataFormat, and make the caller handle this via try/except.

Then, you can remove the else here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

'Email violations: invalid data format: %s', data_format)
else:
attachment = None
if data_format == 'csv':
attachment = self._make_attachment_csv()
else:
attachment = self._make_attachment_json()
subject, content = self._make_content()
email_map['subject'] = subject
email_map['content'] = content
email_map['attachment'] = attachment

return email_map

def _send(self, **kwargs):
Expand Down Expand Up @@ -156,4 +174,5 @@ def _send(self, **kwargs):
def run(self):
"""Run the email notifier"""
email_notification = self._compose()
self._send(notification=email_notification)
if email_notification:
self._send(notification=email_notification)
57 changes: 37 additions & 20 deletions google/cloud/forseti/notifier/notifiers/gcs_violations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@

"""Upload violations to GCS."""

import tempfile

from google.cloud.forseti.common.data_access import csv_writer
from google.cloud.forseti.common.gcp_api import storage
from google.cloud.forseti.common.util import date_time
from google.cloud.forseti.common.util import logger
from google.cloud.forseti.common.util import parser
from google.cloud.forseti.common.util import string_formats
from google.cloud.forseti.notifier.notifiers import base_notification

Expand All @@ -29,30 +30,46 @@
class GcsViolations(base_notification.BaseNotification):
"""Upload violations to GCS."""

def _get_output_filename(self):
"""Create the output filename.
def _upload_json(self, gcs_upload_path):
"""Upload violations in json format.

Returns:
str: The output filename for the violations CSV file.
Args:
gcs_upload_path (string): the GCS upload path.
"""
now_utc = date_time.get_utc_now_datetime()
output_timestamp = now_utc.strftime(
string_formats.TIMESTAMP_TIMEZONE_FILES)
output_filename = string_formats.VIOLATION_CSV_FMT.format(
self.resource, self.cycle_timestamp, output_timestamp)
return output_filename
with tempfile.NamedTemporaryFile() as tmp_violations:
tmp_violations.write(parser.json_stringify(self.violations))
tmp_violations.flush()
storage_client = storage.StorageClient()
storage_client.put_text_file(tmp_violations.name, gcs_upload_path)

def run(self):
"""Generate the temporary CSV file and upload to GCS."""
def _upload_csv(self, gcs_upload_path):
"""Upload violations in csv format.

Args:
gcs_upload_path (string): the GCS upload path.
"""
with csv_writer.write_csv(resource_name='violations',
data=self.violations,
write_header=True) as csv_file:
LOGGER.info('CSV filename: %s', csv_file.name)
storage_client = storage.StorageClient()
storage_client.put_text_file(csv_file.name, gcs_upload_path)

gcs_upload_path = '{}/{}'.format(
self.notification_config['gcs_path'],
self._get_output_filename())

if gcs_upload_path.startswith('gs://'):
storage_client = storage.StorageClient()
storage_client.put_text_file(csv_file.name, gcs_upload_path)
def run(self):
"""Generate the temporary (CSV xor JSON) file and upload to GCS."""
if self.notification_config['gcs_path'].startswith('gs://'):
data_format = self.notification_config.get('data_format', 'csv')
if data_format not in ['json', 'csv']:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the second time this is checked, see my comment above in _make_content(). I think it's better to check and raise here early on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

LOGGER.error('GCS upload: invalid data format: %s', data_format)
elif data_format == 'csv':
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to break up the nested if-if-elif-else here.

if not self.notification_config['gcs_path'].startswith('gs://'):
    log.error()
    return

if data_format not in supported_data_format:
    log.error()
    raise

if data_format == 'json':
    send json
else:
    send csv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

gcs_upload_path = '{}/{}'.format(
self.notification_config['gcs_path'],
self._get_output_filename(
string_formats.VIOLATION_CSV_FMT))
self._upload_csv(gcs_upload_path)
else:
gcs_upload_path = '{}/{}'.format(
self.notification_config['gcs_path'],
self._get_output_filename(
string_formats.VIOLATION_JSON_FMT))
self._upload_json(gcs_upload_path)
Loading