Remote Log Storage (take 2) #1137
Conversation
|
Will have a look tomorrow. |
| # send log to S3 | ||
| encrypt = configuration.get('core', 'ENCRYPT_S3_LOGS') | ||
| s3_key.set_contents_from_string(new_log, encrypt_key=encrypt) | ||
| store_s3_log(log, remote_log_location) | ||
| except: | ||
| print('Could not send logs to S3.') |
There was a problem hiding this comment.
Does it make sense to do logging.error() instead of prints for these?
There was a problem hiding this comment.
I was trying to follow the model elsewhere in cli.py -- print instead of logging. I thought maybe it was because the logs are being redirected for the task, so if there's an error you wouldn't actually see it unless you could load the log.
|
@jlowin let me know when you're ready for another pass |
|
@criccomini I like the idea of factoring out the function -- I will try to get to it tomorrow. |
|
@jlowin just checking in |
|
@criccomini just committed a big refactor |
|
Haven't forgotten about this. Just a bit back-logged at the moment. Sorry. |
|
@jlowin can you squash commit? Also, trying to test locally. How do I install the gcp_api package? My Python foo is lacking. I did a |
|
If you first |
squashed: update configuration more descriptive comment split remote log uploads into helper functions for S3 and GCS read logs from s3 read logs from GCS keep old_log as string change name to log_base better logging overwrite in GCS use current configuration var objects could be none; don't check if they exist with method allow s3 encryption from hook fix capitalization typo replace string search with indexing add param docs refactor remote log read/write into utility classes
|
I had some trouble loading the gcp_api hook (maybe because my setup is geared around gcloud -- I'm not sure!) so I really appreciate anything you can do to kick the tires with that package :) |
|
|
|
Awesome, thanks. |
|
Was able to install. Ran some CLI-based tests: This used to fail. Will run a few more checks in the morning, then merge. Thanks for the squash. |
|
|
|
Based on testing, this needs to be: |
|
Other than that, looks good to me. If you fix version, I can merge. |
|
@criccomini thanks, just added that cap |
|
|
Remote Log Storage (take 2)
|
Merged, thanks! Really looking forward to using this. |
Replaces #1125. Depends on the GCSHook in #1119.
This extends the remote log storage to include Google Cloud Storage in addition to S3. It uses either of the two GCP packages, as available (see discussion in #1109, #1118, #1119).
In addition, per @criccomini's suggestion, it refactors the existing (and new) functionality to use hooks. This allows more fine-grained control over the log writing behavior/access.