SEAB-5604: submit github delivery event to s3 #162
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
upsertGitHubTag/deployment/index.js
Outdated
| const client = new S3Client({}); | ||
| const command = new PutObjectCommand({ | ||
| Bucket: process.env.BUCKET_NAME, | ||
| Key: deliveryId, |
There was a problem hiding this comment.
This will put all objects in the "root" of the bucket. Is that what we want? If we know the deliveryId, then it works. If we don't know it, then it's going to be hard to find.
Is it worth putting them in keys by date/org or org/date?
I don't know the answer, just raising the question. It depends on how we expect to use this.
There was a problem hiding this comment.
It is now changed to date/repository/deliveryid
There was a problem hiding this comment.
I had to change it to date/deliveryid as there can be multiple repos in a single delivery.
| @@ -253,6 +254,23 @@ function processEvent(event, callback) { | |||
| " from GitHub.", | |||
| }); | |||
| } | |||
| // If bucket name is not null (had to put this for the integration test) | |||
| if (process.env.BUCKET_NAME) { | |||
There was a problem hiding this comment.
This is called after we invoke callback above...should it be before? Doesn't the lambda use callback to return a result? (general question, callbacks confuse me)
There was a problem hiding this comment.
It looks like it will still execute after the callback: https://stackoverflow.com/questions/49688927/how-do-i-stop-execution-of-a-aws-lambda-after-a-callback. Although maybe you want to log earlier to avoid confusion. It doesn't look like it will affect performance either way
There was a problem hiding this comment.
In looking at Kathy's question, I noticed the method is getting pretty big (we don't have a linter in place), so I'd optionally suggest creating a method out of this if block, e.g., logPayloadToS3().
There was a problem hiding this comment.
How strongly do we feel about moving my s3 code to be before the callback? I avoided putting my code to s3 before the callback to avoid too many if-blocks because we want to avoid submitting the event if the event type is not supported. So, I put return in the else condition (line 257) above such that the new s3 code wouldn't be ran.
| @@ -253,6 +254,23 @@ function processEvent(event, callback) { | |||
| " from GitHub.", | |||
| }); | |||
| } | |||
| // If bucket name is not null (had to put this for the integration test) | |||
| if (process.env.BUCKET_NAME) { | |||
There was a problem hiding this comment.
It looks like it will still execute after the callback: https://stackoverflow.com/questions/49688927/how-do-i-stop-execution-of-a-aws-lambda-after-a-callback. Although maybe you want to log earlier to avoid confusion. It doesn't look like it will affect performance either way
| @@ -253,6 +254,23 @@ function processEvent(event, callback) { | |||
| " from GitHub.", | |||
| }); | |||
| } | |||
| // If bucket name is not null (had to put this for the integration test) | |||
| if (process.env.BUCKET_NAME) { | |||
There was a problem hiding this comment.
In looking at Kathy's question, I noticed the method is getting pretty big (we don't have a linter in place), so I'd optionally suggest creating a method out of this if block, e.g., logPayloadToS3().
| return; | ||
| } | ||
| // If bucket name is not null (had to put this for the integration test) | ||
| if (process.env.BUCKET_NAME) { |
There was a problem hiding this comment.
I make this part of the logPayloadToS3 method, logPayloadtoS3(body, bucketPath, deliveryId).
How strongly do we feel about moving my s3 code to be before the callback? I avoided putting my code to s3 before the callback to avoid too many if-blocks because we want to avoid submitting the event if the event type is not supported. So, I put return in the else condition (line 257) above such that the new s3 code wouldn't be ran.
Then if you want to do it before the callback, or in each of the conditions, you're only adding one line.
I don't feel too strongly about going before. Maybe add a comment before the method invocation that it will execute even if the callback has been invoked, since that caused some confusion in the PR review.
There was a problem hiding this comment.
With @coverbeck and @kathy-t comments (my feedback has been addressed)
There was a problem hiding this comment.
Thanks for persevering, the existing code is super-duper confusing. If I remember correctly, there's actually two varieties of callbacks, both referenced as callback and invoked at various points in the code. And then, to muddy things further, there's the handleCallback function, a better name for which would be handleResponse.
Description
This PR adds code to the existing github lambda to send the message to a S3 bucket. The lambda uploads the event in the path
date/deliveryidIssue
https://ucsc-cgl.atlassian.net/browse/SEAB-5604
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!