Skip to content
37 changes: 37 additions & 0 deletions upsertGitHubTag/deployment/index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
"use strict";
const { PutObjectCommand, S3Client } = require("@aws-sdk/client-s3");

const url = require("url");
const https = require("https");
const http = require("http");
const crypto = require("crypto");
const LAMBDA_USER_AGENT = "DockstoreLambda (NodeJs)";
const DELIVERY_ID_HEADER = "X-GitHub-Delivery";
const client = new S3Client({});

// Verification function to check if it is actually GitHub who is POSTing here
const verifyGitHub = (req, payload) => {
Expand Down Expand Up @@ -168,6 +170,9 @@ function processEvent(event, callback) {
if (githubEventType === "installation_repositories") {
// The installation_repositories event contains information about both additions and removals.
console.log("Valid installation event");

logPayloadToS3(body, deliveryId); //upload event to S3

path += "workflows/github/install";
postEndpoint(path, body, deliveryId, (response) => {
const added = body.action === "added";
Expand Down Expand Up @@ -202,6 +207,8 @@ function processEvent(event, callback) {
// A push has been made for some repository (ignore pushes that are deletes)
if (!body.deleted) {
console.log("Valid push event");
logPayloadToS3(body, deliveryId); //upload event to S3

const repository = body.repository.full_name;
const gitReference = body.ref;

Expand Down Expand Up @@ -252,6 +259,36 @@ function processEvent(event, callback) {
githubEventType +
" from GitHub.",
});
return;
}
}

function logPayloadToS3(body, deliveryId) {
// If bucket name is not null (had to put this for the integration test)
if (process.env.BUCKET_NAME) {
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)

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

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

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.

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.

const uploadDate = new Date();
const bucketPath = `${uploadDate.getFullYear()}-${uploadDate.getMonth()}-${uploadDate.getDay()}/${deliveryId}`; //formats path to YYYY-MM-DD/deliveryid

const command = new PutObjectCommand({
Bucket: process.env.BUCKET_NAME,
Key: bucketPath,
Body: JSON.stringify(body),
ContentType: "application/json",
});
try {
const response = client.send(command);
console.log(
"Successfully uploaded payload to bucket. DeliveryID: ",
deliveryId,
response
);
} catch (err) {
console.error(
"Error uploading payload to bucket. DeliveryID: ",
deliveryId,
err
);
}
}
}

Expand Down