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

fix: lazy validate env variables in the theme submit route #606

Merged
merged 1 commit into from Mar 14, 2023

Conversation

tony-sull
Copy link
Contributor

This moves the check for DISCORD_ environment variables to a runtime check when the form is submitted

Why?

Netlify PR preview builds from community members won't include private environment variables, breaking the build immediately

@netlify
Copy link

netlify bot commented Mar 13, 2023

Deploy Preview for astro-www-2 ready!

Name Link
🔨 Latest commit 8fa0628
🔍 Latest deploy log https://app.netlify.com/sites/astro-www-2/deploys/6410955968a6300008fa8cf7
😎 Deploy Preview https://deploy-preview-606--astro-www-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -5,13 +5,15 @@ const schema = z.object({
SUBMISSION_DISCORD_WEBHOOK_THREAD_ID: z.string(),
})

const result = schema.safeParse(import.meta.env)
if (!result.success) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing this wholesale maybe we move it behind a import.meta.env.CONTEXT === 'production' check? This would then only error for production builds (could maybe warn in other situations?)

Docs: https://docs.netlify.com/configure-builds/environment-variables/#read-only-variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah that's interesting! I try to avoid depending on Netlify's built-in variables to avoid issues when they don't exist in astro dev, but that actually makes sense in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out the fallback code gets a little wonky here checking import.meta.env.CONTEXT in addition to using safeParse to check for errors

Going to leave the easier fix here for now to unblock community PRs and open add it to the list of backlog tasks to revisit

Copy link
Contributor

@itsMapleLeaf itsMapleLeaf left a comment

Choose a reason for hiding this comment

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

Good change, I'm personally cool with this

@tony-sull tony-sull merged commit 32c1bb8 into main Mar 14, 2023
6 checks passed
@tony-sull tony-sull deleted the fix/lazy-env-vars branch March 14, 2023 15:48
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.

None yet

3 participants