Closed goochi1 closed 6 years ago
@austinbyers thank you for changes i have updated as requested however for the optional part for the safe alerts for sns I have not added this in the manage.py file so it wont come as an option.
@austinbyers have you had a chance to have a look?
@goochi1 unit tests are still failing (which you should be able to see by clicking the "x" next to your most recent commit). I'm not sure exactly why they're failing, there may be an issue with the mocks.
After the unit tests pass, the best way to test the optional safe topic ARN is to do a test deploy without the safe topic, and then deploy again after adding it. This will show a few more changes we need - in particular, the analyzer Lambda code needs to check whether a safe topic is defined before trying to send to it.
Sometime within the next few weeks I should have time to help finish this PR if you need it
So when I test it it doesn’t have a x come up :/ will have a look tomorrow or Wednesday
Get Outlook for iOShttps://aka.ms/o0ukef
From: Austin Byers notifications@github.com Sent: Monday, May 14, 2018 6:29:30 PM To: airbnb/binaryalert Cc: goochi1; Mention Subject: Re: [airbnb/binaryalert] Steph (#118)
@goochi1https://github.com/goochi1 unit tests are still failing (which you should be able to see by clicking the "x" next to your most recent commit). I'm not sure exactly why they're failing, there may be an issue with the mocks.
After the unit tests pass, the best way to test the optional safe topic ARN is to do a test deploy without the safe topic, and then deploy again after adding it. This will show a few more changes we need - in particular, the analyzer Lambda code needs to check whether a safe topic is defined before trying to send to it.
Sometime within the next few weeks I should have time to help finish this PR if you need it
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/airbnb/binaryalert/pull/118#issuecomment-388898431, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQuzPnjN4qkHYFquZCwBKprQPmI94VPwks5tyb75gaJpZM4TxeYW.
You should be able to see it in the GitHub UI:
@austinbyers ahh i see and i know what it was, the default region was missing, my bad. Put back now.
Also i have tested with safe documents to ensure it worked. :)
Can you review, thanks for your help with this btw
@austinbyers hey, can't figure out what is causing the x let me know when you get a chance to have a look
So my unit test works, not sure whats happening
@austinbyers changed a few things that i believe were causing issues but i don't understand the new reason it is failing. Can you elaborate?
Sure! Clicking on the Travis link shows a pylint failure:
All of the CI tests are in .travis.yml
which you can run locally. For example, you can run pylint lambda_functions rules tests *.py -j 1
to test it. I'll add a testing script which makes this process easier
@austinbyers cleared it up :)
@goochi1 nice job on getting CI to pass! But I'm fairly certain this will still break if the option to enable safe alerts is disabled. Can you do a test deploy both with and without negative alerting?
Otherwise, I might be able to address it when I get a chance
@austinbyers added in some changed to accept 0 for no safe alters. Let me know if you have anymore feedback :)
@austinbyers did you try running mine? weirdly it seems to fail with the following error now
module.binaryalert_analyzer.aws_lambda_function.function: 1 error(s) occurred:
aws_lambda_function.function: Error creating Lambda function: timeout while waiting for state to become 'success' (timeout: 1m0s)
to: @airbnb/binaryalert-maintainers cc:
size: small|medium|large
resolves #
Background
This Creates an SNS topic for files that get loaded to the s3 bucket that are safe. This was for a project where files are loaded to an s3 and if they are safe they move through the process if not we get notified and an alternate process happens.
Changes
updated analyzer_ to add publish to safe updated binary info alter files that are safe updated main.py to give an alternate route to safe file added new sns for safe and that to lambda file added permisson to push to sns topic
Testing
tested using your tester to check alters work tested manually usings s3 and normal text document