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.

Sign up for GitHub

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

Jump to bottom

SEAB-5604: submit github delivery event to s3 #162

Merged
merged 18 commits into from
Apr 18, 2024
Merged

SEAB-5604: submit github delivery event to s3 #162

Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0300334
initial
hyunnaye Mar 27, 2024
03cb382
unused
hyunnaye Mar 27, 2024
48ced08
import fix
hyunnaye Mar 27, 2024
ca13b79
import fix
hyunnaye Mar 28, 2024
cf42ead
if case
hyunnaye Apr 2, 2024
02f3af1
Merge branch 'develop' into feature/seab-5604
hyunnaye Apr 12, 2024
cdd4480
pr review
hyunnaye Apr 15, 2024
7a8df45
forgot semicolon
hyunnaye Apr 15, 2024
80e9063
pr review
hyunnaye Apr 16, 2024
adf9b6e
pr review
hyunnaye Apr 16, 2024
aa12586
pr review
hyunnaye Apr 16, 2024
d176c4a
pr review
hyunnaye Apr 16, 2024
cdb6b7d
eslint
hyunnaye Apr 16, 2024
d02b3d2
eslint
hyunnaye Apr 16, 2024
de3f3a9
folder path fix
hyunnaye Apr 17, 2024
ebbbcfc
new helper function
hyunnaye Apr 18, 2024
8d8381d
new helper function
hyunnaye Apr 18, 2024
91f627b
eslint
hyunnaye Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions upsertGitHubTag/deployment/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"use strict";
const { PutObjectCommand, S3Client } = require("@aws-sdk/client-s3");

const url = require("url");
const https = require("https");
Expand Down Expand Up @@ -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) {
hyunnaye marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

@hyunnaye hyunnaye Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Send payload to s3
const client = new S3Client({});
hyunnaye marked this conversation as resolved.
Show resolved Hide resolved
const command = new PutObjectCommand({
Bucket: process.env.BUCKET_NAME,
Key: deliveryId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now changed to date/repository/deliveryid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change it to date/deliveryid as there can be multiple repos in a single delivery.

Body: JSON.stringify(body),
ContentType: "application/json",
});
try {
const response = client.send(command);
console.log(response);
hyunnaye marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
console.error(err);
hyunnaye marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

// Handle response from Dockstore webservice
Expand Down
Loading