[Bug]: deployctl deploy executes entrypoint before setting --env vars

[h4l]

Problem description

When creating a deployment from the CLI with deployctl deploy we can set environment variables with --env / --env-file. During deployment, Deno Deploy appears to execute the entrypoint module without the envars set.

• If the entrypoint requires the vars to be set, deployment fails.
• If the entrypoint can execute without the vars set, deployment succeeds and the CLI output indicates envars are set/saved after the point that it executes the entrypoint.

Steps to reproduce

Create a module/app that requires environment variables to work:

// main.ts
const name = Deno.env.get("GREETING_NAME");
if (!name) throw new Error("GREETING_NAME is not set");

Deno.serve((): Response => {
  return new Response(`Hello ${name}\n`);
});

Deploy it with the environment variables set. Deployment fails due to the module being executed without the vars set:

$ deployctl deploy --include '*.ts' --env=GREETING_NAME=Foo --project=required-env 
ℹ Using config file '/workspaces/xxx/deploy_playground/deno.json'
✔ Deploying to project required-env.
✔ Entrypoint: /workspaces/xxx/deploy_playground/main.ts
ℹ Uploading all files from the current dir (/workspaces/xxx/deploy_playground)
✔ Found 2 assets.
✔ Uploaded 2 new assets.
✖ Deployment failed.
error: The deployment failed: UNCAUGHT_EXCEPTION

Error: GREETING_NAME is not set
    at file:///src/main.ts:2:18

If the throw is removed, deploy proceeds and shows ⠸ Setting environment variables... after the point that it failed previously.

Expected behavior

Environment variables provided should be set when the entrypoint is executed.

Environment

$ deployctl --version
deployctl 1.12.0
$ deno --version
deno 1.43.1 (release, aarch64-unknown-linux-gnu)
v8 12.4.254.12
typescript 5.4.3

Possible solution

If vars can't be set before executing the entrypoint, maybe provide a way for an app to know it's being executed during deployment as a test, so that it can avoid loading config & failing if it's not available?

Additional context

No response

[h4l]

I wonder if I'm misunderstanding the intention of deployment-level envars. Maybe they are not really intended for configuration? I notice that deployctl deployments show shows the names of env vars, but not their values. So it'd be hard to tell how a deployment was configured, if deploy-level env vars were used for config.

Also the example in the deployctl deploy --help output shows setting a metadata-like var DEPLOYMENT_TS — the kind of thing that would be optional:

You can set env variables for deployments to have access using Deno.env. You can use --env to set individual
environment variables, or --env-file to load one or more environment files. These options can be combined
and used multiple times:

    deployctl deploy --env-file --env-file=.other-env --env=DEPLOYMENT_TS=$(date +%s)

Be aware that the env variables set with --env and --env-file are merged with the env variables configured for the project.
If this does not suit your needs, please report your feedback at
https://github.com/denoland/deploy_feedback/issues/

And env vars set at the project level (e.g via the deploy website) are set during deploy.

[magurotuna]

Thanks for the report, this is indeed a bug (or rather current limitation with deployctl).

To be clear, there are two levels where we can configure env vars; one is project level and the other is deployment level. As you noticed, setting env vars in the settings page of the Deno Deploy dashboard is the project level configuration, whereas what deployctl deploy --env option is configuring is the deployment level. The final env vars that are applied to a particular deployment are the result of merging project-level and deployment-level config.

The current limitation we have with deployment-level env var configuration is as follows: what happens internally in deployctl deploy command is it first creates a deployment without any specified deployment-level env vars and then it will "redeploy" with the env vars. This is the limitation due to how the backend API works currently - we definitely could modify it so that we won't need to "redeploy" to apply the deployment-level env vars, but it's a little bit of a low priority as of now.

So for now, one possible workaround would be setting the required env vars as the project-level - that is, create a project (or you can reuse the empty project you got as a result of running deployctl deploy --env with failure), set the required env vars as project-level env vars in the settings tab of the dashboard (i.e. https://dash.deno.com/projects/<your-project-name>/settings), and then do deployctl deploy --env again.

[h4l]

Thanks for the explanation & workaround, that's helpful.

[ansipunk]

Something has to change.

This workaround

if (Deno.env.get("MYVAR")) {
  main();
} else {
  Deno.serve((_) => new Response("Deployment stage"));
}

is not something good to have in your codebase.

If you don't have the else part, the deployment will fail with ISOLATE_INTERNAL_FAILURE, probably due to the program exiting right away even with exit code 0. This, by the way, should be communicated clearly, as I simply was lucky with my guess.

I understand the reasoning behind not having environment variables set during the deployment stage - nothing will be falsely triggered. But I believe that instead Deno Deploy should not try to execute the program before actually deploying it, more faith should be put into the user.

You might argue that this should be taken to Deno Deploy feedback or somewhere, but it just works if you attach your repo and set environment variables through the interface.

[csvn]

But I believe that instead Deno Deploy should not try to execute the program before actually deploying it, more faith should be put into the user.

Actually, I believe it's important that it does execute before deploying, as it's otherwise easy to push a commit with e.g. an invalid import path that will then be deployed to prod and taking down the site. If a deploy should be made without executing the code first that should be behind a flag, e.g. deployctl deploy --no-execute ....

[ansipunk]

@csvn that's your problem. Deploy, and if everything is fine, promote to production. Just like Deno Deploy does it if you are using the web interface.

[csvn]

that's your problem

I actually don't have that problem, because we run deno check in GitHub Actions, and always create PR's and do not push to main. But I think the Deno company may want to cater for people who potentially push directly to main as their workflow, and to not break their sites when the code does not even run. The ones who actually experiences downtime are users of the site. It's not a bad idea to have safeguards against broken deploys by default.

Just like Deno Deploy does it if you are using the web interface.

The web interface is only used for small experiments, real sites/project has their code in a Git. So this argument is not a point in favor IMO.

Let's just agree to disagree about this part, since we seem to have differing opinions. I have my environment variables on the project directly, so I have no issues with this currently.

[ansipunk]

@csvn agreeing to disagreeing is fine, but in my opinion arguments like --no-execute defeat that purpose entirely, as well as adding extra branches in code to bypass test execution. Not every program is meant to be ran without any configuration. Maybe it has to be clearly communicated somewhere? Also, as I mentioned earlier, I find it very counterintuitive that the platform will mark the deployment as failed, should it exit with code 0 immediately. Making the program do a dry-run during the deployment just to deploy seems pointless to me. What in the world is the goal of that, if all the code has to be discarded?

[csvn]

Making the program do a dry-run during the deployment just to deploy seems pointless to me. What in the world is the goal of that, if all the code has to be discarded?

One example, that was the reason this issue hit me. I was that running Deno locally and on CI and everything worked fine. But since Deno Deploy is not using the same Deno executable, it was lagging behind in versions, which caused code using with assertion in import to fail:

import json from "./foo.json" with { type: "json" };

So in my case, I was glad it did not do a broken deploy and break prod just because of this syntax error. Invalid syntax, import issues, or missing features in Deno Deploy did not match the latest Deno CLI version. I learned then that I should be careful in updating CI Deno CLI version, because it's hard do know what version Deno Deploy is using.

It's not great if it fails to start up due to env variables, but IMO it's good that it tries to resolve all files and syntax of code that will run, at least for now.

I also think part of the reason they do a "dry run" is because Deno Deploy downloads all dependencies from deno.json or import map, and the dry run is to make sure it can start and all code is in place.

Maybe it has to be clearly communicated somewhere?

Fully agree it would be great if docs covered more of this, I've also had some things (like the above) I encountered that made things harder to figure out.