Update security vulnerability alerts with Airtable#25
Conversation
ashley-hebler
left a comment
ashley-hebler
left a comment
There was a problem hiding this comment.
This is a wonderful proof on concept! Totally fine to leave this connected to the test Airtable base for now since we don't have much time left. We can carry it over the finish line if needed. Adding airtable records (with all these field filled out at that!) is such a huge process improvement for us; thank you for starting this.
Some quick wins if there's time would be:
- Getting
readme.mdfile with the env vars started (something like this)- The local testing command (with the functions framework) might be nice to list in there too
- Deleting the original folder and assets
security-vulnerabilitiesso we know which is the source of truth - Addressing some of the inline comments like the
process_requestmethod name and missing imports (just so that we can hit the ground running when testing locally)
A few improvements for future iterations:
I don't expect you to get to these, but just wanted to log some ideas while they're fresh
- Switch out the Airtable User ID token with a Personal API token
- Sounds like they're deprecating the User ID token eventually
- Add a "created on" date as text in the summary of the Airtable record
- We'd see this in the version history, but it might be nice to see it clearly for reference and triaging
- Reference the Airtable record URL in the Slack notification
- This would take some reordering in the sequence of events, but it might be cool to have!
Sorry to get you this feedback so late in the game and no pressure to get through it all. You've certainly set us up for success!
| import logging | ||
| logging.basicConfig(level=logging.DEBUG) | ||
|
|
||
| from airtable import airtable |
There was a problem hiding this comment.
Looks like you worked your way around having to use this dependency, which is great because it's fewer dependencies to manage! I run into a missing import error spinning this up with the functions-framework so we might want to remove this line.
|
|
||
| @functions_framework.http | ||
| @app.route('/', methods=['POST']) | ||
| def main(request): |
There was a problem hiding this comment.
the functions-framework barks at me that this should maybe be process_request
functions_framework.exceptions.MissingTargetException: File /Users/ashleyhebler/Documents/Github/tacobots/security-vulnerability-alerts/main.py is expected to contain a function named 'process_request'. Found: 'create_record_airtable', 'load_dotenv', 'main', 'write_slack_message' instead
| if severity == "medium" or severity == "low" or payload["action"] != "created": | ||
| return | ||
|
|
||
| print(create_record_airtable(payload)) |
There was a problem hiding this comment.
Is the print statement there for debugging? Since this is sorta of a core method call in the flow, I think it'd make sense to call it explicitly.
| } | ||
|
|
||
| response = requests.request("POST", url, headers=headers, data=json.dumps(data)) | ||
| return response |
There was a problem hiding this comment.
I noticed that when I had the AIRTABLE_BASE_ID var wrong (I totally missed it on the API docs page 🤦♀️) this failed pretty silently. It wasn't until I printed the response with print(response.text.encode('utf8')) and saw...
DEBUG:urllib3.connectionpool:https://api.airtable.com:443 "POST /v0/<MY_WRONG_BASE_ID>/table1 HTTP/1.1" 404 21
b'{"error":"NOT_FOUND"}'
...that I figured out what was going on. I don't think is needed right away, but a future improvement would be to check for a status code with response.status_code == 200 or something like that and surface the error message if it's not that.
| functions-framework==3.* | ||
| flask==2.1.0 | ||
| slackclient==2.9.4 | ||
| python-dotenv==0.21.0 No newline at end of file |
There was a problem hiding this comment.
I think requests is missing here. I had to install it to get the invoke_function.py script to run
2 participants
What's this PR do?
This PR updates the Security Vulnerability Alerts Slackbot to implement the following changes:
Why are we doing this? How does it help us?
How should this be manually tested?
This update uses Google Functions Framework in order to test locally. It requires three environment variables that should be placed in a .env file: SLACK_API_TOKEN, AIRTABLE_TOKEN, and AIRTABLE_BASE_ID.
The SLACK_API_TOKEN will be generated through the Slackbot API.
The AIRTABLE_TOKEN can be generated using the following instructions: https://support.airtable.com/docs/how-do-i-get-my-api-key
The AIRTABLE_BASE_ID can be found in the Airtable API.
The following command will create a server on localhost: functions-framework --target main --debug
After the server is running, there are two ways to test the Slackbot:
Further detail is provided in the README.md
How should this change be communicated to end users?
Are there any smells or added technical debt to note?
What are the relevant tickets?
Have you done the following, if applicable:
(optional: add explanation between parentheses)
TODOs / next steps: