Add tmp_dir to task and let output_manager copy locally#282
Conversation
|
The CodeFactor error is "Probable insecure usage of temp file/directory", but I think that's a FP. Not sure what the best way to clear that is. |
|
|
||
| def __init__(self): | ||
| self._output_writers = None | ||
| self.initialized = False |
There was a problem hiding this comment.
Please rename this variable, as this is representing "readiness" or something distinct from initialization (note that this an init method).
Alternatively, can you do the setup in one step, rather than two? Multiple levels of initialization makes things a bit tricky.
There was a problem hiding this comment.
Yeah, good point. I changed it to is_setup. I had the setup separately from the init because I usually try to separate anything non-trivial into a separate method.
| self.local_output_dir = os.path.join(self.base_output_dir, self.unique_dir) | ||
| if not os.path.exists(self.local_output_dir): | ||
| def create_output_dir(self, base_path=None): | ||
| base_path = base_path if base_path else self.base_output_dir |
There was a problem hiding this comment.
This is overriding a method in the base class which has a relevant docstring. So far I haven't been copying those in this case, but let me know if you think we should be doing that differently.
| blob.upload_from_filename(source_path, client=self.client) | ||
| return os.path.join('gs://', self.bucket, destination_path) | ||
|
|
||
| def copy_from(self, source_path): |
There was a problem hiding this comment.
+docstring, as this raises an exception that isn't in the interface. I'd like to see docstrings for all these methods, but I'm not sure what the Turbinia style preference is.
There was a problem hiding this comment.
Done. Let's talk out of band about the docstrings for overridden methods. I agree that they should be changed if the overridden method does anything differently, but in the case where everything is the same, I generally prefer to not copy them. No idea if that is considered idiomatic or not, :).
There was a problem hiding this comment.
Chatting sounds good, this is OK for now.
aarontp
left a comment
There was a problem hiding this comment.
Thanks for the quick review! PTAL. If we want to make changes to how we handle docstrings for subclassed objects, let's do that in a separate PR.
|
|
||
| def __init__(self): | ||
| self._output_writers = None | ||
| self.initialized = False |
There was a problem hiding this comment.
Yeah, good point. I changed it to is_setup. I had the setup separately from the init because I usually try to separate anything non-trivial into a separate method.
| self.local_output_dir = os.path.join(self.base_output_dir, self.unique_dir) | ||
| if not os.path.exists(self.local_output_dir): | ||
| def create_output_dir(self, base_path=None): | ||
| base_path = base_path if base_path else self.base_output_dir |
There was a problem hiding this comment.
This is overriding a method in the base class which has a relevant docstring. So far I haven't been copying those in this case, but let me know if you think we should be doing that differently.
| blob.upload_from_filename(source_path, client=self.client) | ||
| return os.path.join('gs://', self.bucket, destination_path) | ||
|
|
||
| def copy_from(self, source_path): |
There was a problem hiding this comment.
Done. Let's talk out of band about the docstrings for overridden methods. I agree that they should be changed if the overridden method does anything differently, but in the case where everything is the same, I generally prefer to not copy them. No idea if that is considered idiomatic or not, :).
| blob.upload_from_filename(source_path, client=self.client) | ||
| return os.path.join('gs://', self.bucket, destination_path) | ||
|
|
||
| def copy_from(self, source_path): |
There was a problem hiding this comment.
Chatting sounds good, this is OK for now.
This changes the following: