feat: astro:env validateSecrets

florian-lefebvre commented 3 months ago

Changes

Adds a new experimental.env.validateSecrets flag. It checks secrets on buildStart (dev and build, maybe more?)

Testing

Adds a test

Docs

Updates config reference and adds changeset

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 72ec674984e0c4f9fd8fa69684c341a80b4f29c2

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

sarah11918 commented 2 months ago

@florian-lefebvre re: naming -- I don't have a problem with the name, as it's clear! Chris might also be a good person to ask!

florian-lefebvre commented 2 months ago

Excellent summary!

Does this option mean runtime errors for on-demand rendered requests can no longer happen? Or even if you check secrets are present during a build, can they still be missing in the final on-demand server context?

I don't think so. Technically, you could update a secret between a build and the deployment.

Also, something I've been thinking about having start in the option name: some people could think it means they are validated when the production servers starts, which is not the case

delucis commented 2 months ago

Thanks for confirming @florian-lefebvre! OK, so given your answer, I’m wondering a bit who this new option is intended to help?

In dev and static builds (points 4 & 5 in my summary), this option means they can fail fast, so failures happen quicker but the builds would have failed anyway eventually.

In production builds with on-demand rendering (point 6), this option does the same, but provides no protection on deployment for the reasons you mention.

So is a separate option needed to just move when in the process the failure happens? What was the user request that led to this? It feels like the more important failure to protect against would be in the production environment, but this is not adding extra protection there.

If the option does seem valuable, maybe even something like env.failFast or similar could be an idea for naming? Although not sure it does much to explain the when.

florian-lefebvre commented 2 months ago

fyi that's a feature i had in astro-env and that saved me quite a few times. A common thing is you add a new required secret var, you make a PR and you forget to add it to your host (and ofc it crashes once in prod). This feature allows to fail early.

I agree that the issue is to convey the "when" 😅

delucis commented 2 months ago

that's a feature i had in astro-env

You mean this PR? Or the behaviour I was describing?

I still think answering “What was the user request that led to this PR?” would help naming if you could try!

florian-lefebvre commented 2 months ago

I mean this behavior was in the now deprecated astro-env integration that led to astro:env. I think the user request is: I want my build to fail if any variable (public or secret) is not valid. I don't want to discover it at runtime

delucis commented 2 months ago

I want my build to fail if any variable (public or secret) is not valid. I don't want to discover it at runtime

Isn’t that already the case?

Based on my summary above the only case where currently it won’t fail during a build is:

A variable only used in the on-demand SSR runtime

And you said above that this PR won’t change that part — on-demand SSR can still fail at runtime because it runs in a different environment from the build.

florian-lefebvre commented 2 months ago

yes but some hosts expose the same secrets on both build and runtime (eg. netlify) so that's why it's interesting imo. Basically

Before this PR, invalid secret is only discovered at build time with a 500 error
After this PR and the option enabled, build fails because variable is invalid. Of course an error can still occur at runtime if the secret is updated between the build and the deployment

Idk if that clarifies anything

delucis commented 2 months ago

Thanks, yeah that confirms what I suspected:

moves the check earlier in the build process
catches issues IF you happen to be deploying in a way where the environment is consistent between build and prod

OK, last question (thanks for your patience 😅):

Does “validation” already only happen at “start”?
In other words: public variables get validated at dev/build start, but private variables are not ever currently “validated”, they just fail at runtime?

If that’s the case, maybe this option just gets simplified to

env: {
  validateSecrets: true,
}

florian-lefebvre commented 2 months ago

Thanks for all the questions that's always great! Yeah I think I like your idea

delucis commented 2 months ago

Nice! OK, then that concludes my questions — thanks again for going through them all 🙌

withastro / astro