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.

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

Whitelist vercel deploy script #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

masonmcelvain
Copy link

The ifixit-nextjs-dev PR deploy failed a few times in https://github.com/iFixit/ifixit/pull/55534, and it turns out we don't deploy strapi when there are changes to the vercel deploy script. This change deploys strapi in that case, and also clarifies the webhook response message to make debugging a little easier.

CC @jarstelfox

When the nextjs deploy script is edited we deploy preview to Vercel. In order to deploy previews we need Strapi to be running for the branch on govinor, so we'll add this file to the whitelist.
These webhook responses are visible in GitHub's UI and help with debugging, so let's add some detail on why no action was taken.
Copy link

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

@deltuh-vee
Copy link

@masonmcelvain
Does this pull need QA?

Copy link
Contributor

@dhmacs dhmacs left a comment

Choose a reason for hiding this comment

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

I spotted a Typescript issue. Let me know if you want me to complete this pull and ship it (we're still deploying manually here).

As a side note, remember we also have the ability to boot any branch whenever our automatic rules don't cover a specific scenario where we'd like to have a Govinor instance:

https://govinor.com/repositories/a2a4ff62-d9f1-4066-a2e9-5ed9a36f751d/branches/new

Comment on lines +37 to +39
return json({
message: `No action taken for webhook payload action ${webhook.payload.action}`,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes a typescript error:

Screenshot 2024-09-23 at 10 55 10

The reason for this being that the default case should never be reached because of this validation:

const validation = GithubEventSchema.safeParse({ event, payload });
if (!validation.success) {
throw json({ message: "skipped", validation: validation.error }, 200);
}

Admittedly the fact that I'm returning a json response is confusing here, this should just be something like:

Suggested change
return json({
message: `No action taken for webhook payload action ${webhook.payload.action}`,
});
return assertNever(webhook.payload);

Copy link
Member

Choose a reason for hiding this comment

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

There are about 20 actions... hmm... seems like we're missing possibly an important one reopened:
image

@jarstelfox
Copy link

Let me know if you want me to complete this pull and ship it

@dhmacs, @masonmcelvain is out this week. If you have time to tackle this, feel free, but it's not high priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants