Allow user to specify the violation payload format (csv / json) #1301
Conversation
This will facilitate reuse of the same method in the EmailViolations class.
Also, add a method that creates the attachment in json format.
Codecov Report
@@ Coverage Diff @@
## 2.0-dev #1301 +/- ##
===========================================
+ Coverage 86.7% 86.97% +0.26%
===========================================
Files 148 148
Lines 10270 10304 +34
===========================================
+ Hits 8905 8962 +57
+ Misses 1365 1342 -23
|
arbeit
changed the title
| str: The output filename for the violations CSV file. | ||
| """ | ||
| now_utc = date_time.get_utc_now_datetime() | ||
| output_timestamp = now_utc.strftime( |
There was a problem hiding this comment.
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.
|
|
||
| Returns: | ||
| attachment: SendGrid attachment object. | ||
| """ | ||
| output_file_name = self._get_output_filename() | ||
| output_file_name = self._get_output_filename( |
There was a problem hiding this comment.
nit: be consistent whether you want to use file_name or filename
| 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, |
| 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, |
There was a problem hiding this comment.
nit: can you put filename=output_file_name, on a new line? Just to be consistent with the indentings here.
| email_map['content'] = content | ||
| email_map['attachment'] = attachment | ||
| data_format = self.notification_config.get('data_format', 'csv') | ||
| if data_format not in ['json', 'csv']: |
There was a problem hiding this comment.
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
| email_map['attachment'] = attachment | ||
| data_format = self.notification_config.get('data_format', 'csv') | ||
| if data_format not in ['json', 'csv']: | ||
| LOGGER.error( |
There was a problem hiding this comment.
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.
| """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']: |
There was a problem hiding this comment.
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.
| data_format = self.notification_config.get('data_format', 'csv') | ||
| if data_format not in ['json', 'csv']: | ||
| LOGGER.error('GCS upload: invalid data format: %s', data_format) | ||
| elif data_format == 'csv': |
There was a problem hiding this comment.
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
Fully tested; switching between CSV and JSON works by specifying a
data_formatconfiguration setting for the notifier in question (with CSV being the default).Fixes issue #1287.