Skip to content
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

Added function for email notifcations #450

Merged
merged 36 commits into from Dec 6, 2019
Merged

Conversation

@CiaranFahy
Copy link
Contributor

CiaranFahy commented Oct 10, 2019

Issue #48

CiaranFahy added 6 commits Oct 9, 2019
up
@CiaranFahy CiaranFahy changed the title Added function for email notifcations #48 Added function for email notifcations Oct 10, 2019
@aarontp

This comment has been minimized.

Copy link
Member

aarontp commented Oct 11, 2019

Thanks! I'll try to review this in the next day or two.

CiaranFahy added 5 commits Oct 11, 2019
Copy link
Member

aarontp left a comment

Thanks for the PR! I've added a few review comments. Are you planning to hook this into the task_manager to send mail when processing requests have completed in a separate PR? or is there more to come on this? Sorry for the slow review, I was unexpectedly out longer than anticipated.

turbinia/config/__init__.py Outdated Show resolved Hide resolved
turbinia/config/turbinia_config_tmpl.py Outdated Show resolved Hide resolved
turbinia/config/turbinia_config_tmpl.py Outdated Show resolved Hide resolved
turbinia/config/turbinia_config_tmpl.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
@CiaranFahy

This comment has been minimized.

Copy link
Contributor Author

CiaranFahy commented Oct 31, 2019

Yeh I plan on hooking this into task_manager in a later pull request, because I still need to figure out how.

CiaranFahy added 2 commits Oct 31, 2019
@aarontp

This comment has been minimized.

Copy link
Member

aarontp commented Oct 31, 2019

Sounds good. I see you made some updates since my initial review, let me know if/when the PR is ready for re-review.

BTW, The call to send the notification can probably be done somewhere around here:

self.remove_jobs(request_id)

I think you can get the requester from the Task that is passed in to that method. Ideally it would send the 'status' report in the email, but you might need to plumb in the client similar to here:

from turbinia.client import TurbiniaClient

Or maybe read directly from Datastore. Let me know if you want me to dig into that more.

CiaranFahy added 3 commits Oct 31, 2019
@CiaranFahy

This comment has been minimized.

Copy link
Contributor Author

CiaranFahy commented Nov 1, 2019

@aarontp Thank you, It's ready for another review.

Copy link
Member

aarontp left a comment

Thanks @CiaranFahy. I noticed a few formatting related things (4 vs 2 spaces, lines too long, etc) that should be picked up by the linter. Could you please run pylint on notify.py with the pylintrc file in the Turbinia repo (you can do pylintrc --rcfile .pylintrc notify.py) and fix the things in the output of that? Also, can you make sure that the tests are passing? It looks like the tests failing is also related to some formatting things, so you may need to run yapf on the file with the .style.yapf config. You might run yapf first because it should fix a lot of the things that the pylint will pick up. Once that's done I'll take another look at it. Thanks much!

turbinia/turbiniactl.py Outdated Show resolved Hide resolved
CiaranFahy added 4 commits Nov 2, 2019
@CiaranFahy

This comment has been minimized.

Copy link
Contributor Author

CiaranFahy commented Nov 2, 2019

Thank you @aarontp , I've ran yapf, and fixed the formatting with the help of pylint.

Copy link
Member

aarontp left a comment

Hello: Thanks for the update. I've added a few comments, but mostly just small things. I think a couple of the older comments are still relevant too.

turbinia/lib/libcloudforensics.py Outdated Show resolved Hide resolved
turbinia/config/turbinia_config_tmpl.py Outdated Show resolved Hide resolved
turbinia/notify.py Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
#Puts message in MIME format
msg = MIMEMultipart()
msg['From'] = config.EMAIL_ADDRESS
msg['To'] = config.EMAIL_RECIEVING_ADDRESS

This comment has been minimized.

Copy link
@aarontp

aarontp Nov 6, 2019

Member

(sorry if this is a dupe comment, I thought I added this before, but I don't see it now)

It would be nice to add this to the input parameters to the function so that you can pass in the requester info, and send the notification directly to the person who initiated the Turbinia request. The requester will only be the username though, so you might need to add EMAIL_DOMAIN to the config.

turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/notify.py Outdated Show resolved Hide resolved
CiaranFahy added 6 commits Nov 6, 2019
@CiaranFahy

This comment has been minimized.

Copy link
Contributor Author

CiaranFahy commented Nov 6, 2019

@aarontp Thank You, I got the CI build to work now, you can review it again

Edit: and I pressed the close button by accident.

@CiaranFahy CiaranFahy closed this Nov 6, 2019
@CiaranFahy CiaranFahy reopened this Nov 6, 2019
@aarontp

This comment has been minimized.

Copy link
Member

aarontp commented Nov 19, 2019

Hi @CiaranFahy : It looks like a few of the previous comments still need to be addressed, could you take a look? I tried to resolve the ones that have already been addressed, but if the comment is no longer relevant you can hit "resolve conversation". I think the big one is just adding a parameter for the address the mail will be sent to (so we can default to the person who made the request).

CiaranFahy added 4 commits Nov 21, 2019
s
@aarontp

This comment has been minimized.

Copy link
Member

aarontp commented Dec 3, 2019

Hi @CiaranFahy : I see you've added a few commits since the last review. Please ping me in a comment when you're ready for re-review. Thanks much!

@CiaranFahy

This comment has been minimized.

Copy link
Contributor Author

CiaranFahy commented Dec 3, 2019

Hi @aarontp it's ready for re-review, However for some reason the yapf style enforcement fails on the Travis ci build while it runs cleanly on my machine.

Copy link
Member

aarontp left a comment

Hi, thanks for sticking in there with this review process. It's looking pretty good, just a couple small things. Thanks much!

turbinia/notify.py Outdated Show resolved Hide resolved
turbinia/turbiniactl.py Outdated Show resolved Hide resolved
turbinia/turbiniactl.py Outdated Show resolved Hide resolved
turbinia/turbiniactl.py Outdated Show resolved Hide resolved
CiaranFahy added 2 commits Dec 6, 2019
@aarontp
aarontp approved these changes Dec 6, 2019
Copy link
Member

aarontp left a comment

Looks good, thanks for the PR!

Let me know if you plan to integrate this into the task manager to send updates when the requests have completed.

@aarontp aarontp merged commit 23ea30b into google:master Dec 6, 2019
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.