Spin up should check required variables #3213

seun-ja · 2025-08-05T16:08:48Z

The PR checks if the variable default isn't provided when required. The check is handled by a new hook: VariableSorterExecutorHooks.

Inspired by @itowlson's comment, new metadata has been added to Providers.

itowlson · 2025-08-05T19:19:30Z

crates/loader/src/local.rs

        })
    }

+    fn env_checker((key, val): (String, Variable)) -> anyhow::Result<(String, Variable)> {


This function name needs to be more descriptive. What is it checking?

itowlson · 2025-08-05T19:20:21Z

crates/loader/src/local.rs

        })
    }

+    fn env_checker((key, val): (String, Variable)) -> anyhow::Result<(String, Variable)> {


Not clear why this takes a tuple rather than two parameters?

itowlson · 2025-08-05T19:22:13Z

crates/loader/src/local.rs

        let variables = variables
            .into_iter()
-            .map(|(name, v)| Ok((name.to_string(), locked_variable(v)?)))
+            .map(|(name, v)| Self::env_checker((name.to_string(), locked_variable(v)?)))


Doing this here means appears to mean we can't even build the LockedApp without having the variables available. How does this worth with e.g. spin registry push or spin cloud deploy?

itowlson · 2025-08-05T19:24:43Z

crates/loader/src/local.rs


+    fn env_checker((key, val): (String, Variable)) -> anyhow::Result<(String, Variable)> {
+        if val.default.is_none() {
+            if std::env::var(env_key(None, key.as_ref())).is_err() {


You can't assume that variables will come from the host environment. They could come from Vault or Azure KeyVault - for example passwords or tokens would typically be stashed in vaults and it would be rude to demand users create dummy EVs for them. Variable sources are defined in the runtime config file - you can't validate them until you have the runtime config providers lined up.

itowlson · 2025-08-05T19:29:54Z

crates/loader/src/local.rs


+    fn env_checker((key, val): (String, Variable)) -> anyhow::Result<(String, Variable)> {
+        if val.default.is_none() {
+            if std::env::var(env_key(None, key.as_ref())).is_err() {


How do you know that the user is using the default prefix?

itowlson · 2025-08-05T19:30:57Z

crates/loader/src/local.rs


+    fn env_checker((key, val): (String, Variable)) -> anyhow::Result<(String, Variable)> {
+        if val.default.is_none() {
+            if std::env::var(env_key(None, key.as_ref())).is_err() {


The environment provider appears to support a dotenv_path setting to merge values from a file - not clear that this is respected here?

itowlson · 2025-08-05T19:32:18Z

crates/loader/src/local.rs

        let variables = variables
            .into_iter()
-            .map(|(name, v)| Ok((name.to_string(), locked_variable(v)?)))
+            .map(|(name, v)| Self::env_checker((name.to_string(), locked_variable(v)?)))


Maybe a nit, but doing this in the loader produces an error message that implies we can't understand the manifest (usually meaning invalid TOML or incorrect schema): that's potentially misleading.

karthik2804 · 2025-08-06T10:18:35Z

I think adding a new hook before execution of the CLI for the variables factor in crates/trigger/src/cl as discussed offline earlier might be a better place to run the check.

seun-ja · 2025-08-06T19:26:56Z

Interestingly, the test failing in CI doesn't fail locally. Investigating it at the moment.

itowlson

I feel like I stuffed up communication in my previous review, sorry. The Spin variables system allows the user to configure variables to be read from multiple different sources. You cannot rely on the environment being the only source. You need to rethink this PR to completely forget about the environment. It needs to go through a configured ProviderResolver or whatever it's called at the moment, the thing that gets set up in the VariablesFactor and checks all configured sources for values. That will do the environment lookup, and any other lookups - the validation code should not be attempting a partial emulation of its behaviour, you should just use what exists.

itowlson · 2025-08-07T19:13:22Z

Cargo.toml

 spin-runtime-factors = { path = "crates/runtime-factors" }
 spin-telemetry = { path = "crates/telemetry", features = [
-  "tracing-log-compat",
+    "tracing-log-compat",


There's a few of these unrelated layout changes - maybe turn off auto formatting for this file?

itowlson · 2025-08-07T19:15:16Z

crates/factors-test/src/lib.rs

    let toml_str = toml::to_string(manifest).context("failed serializing manifest")?;
    let dir = tempfile::tempdir().context("failed creating tempdir")?;
+    // `foo` variable is set to require. As we're not providing a default value, env is checked.
+    std::env::set_var(env_key(None, "foo"), "baz");


I still say we should be able to transform a TOML manifest into a LockedApp without doing validation on variables.

itowlson · 2025-08-07T19:16:14Z

crates/trigger/src/cli/variables.rs

+use spin_factors_executor::ExecutorHooks;
+use spin_variables::{VariableProviderConfiguration, VariableSourcer};
+
+/// Implements TriggerHooks, sorting required variables


Sorting? Maybe comment on why they need sorting?

Yeah, sorting wasn't the right choice of words. It doesn't sort, but basically checks for variables based on the configuration available for each variable provider. Guess the right word would be "checker"

itowlson · 2025-08-07T19:17:12Z

crates/trigger/src/cli/variables.rs

+use spin_variables::{VariableProviderConfiguration, VariableSourcer};
+
+/// Implements TriggerHooks, sorting required variables
+pub struct VariableSorterExecutorHooks {


Not sure about the naming here. Why is it an "Executor" hook?

The naming choice was to reflect its implementation of the ExecutorHooks trait

Oh, it's called ExecutorHooks now... fair enough then but probably good to change the comment! grin

itowlson · 2025-08-07T19:18:22Z

crates/trigger/Cargo.toml

 spin-telemetry = { path = "../telemetry" }
+spin-variables = { path = "../variables" }
 tokio = { workspace = true, features = ["fs", "rt"] }
+toml = { workspace = true }


I think Ryan had tried to push TOML dependencies into the runtime config crate, so that the trigger crate was abstracted from the physical representation of the runtime config? (For e.g. SpinKube scenarios.) I am not sure though.

itowlson · 2025-08-07T19:19:35Z

crates/trigger/src/cli/variables.rs

+}
+
+impl VariableSourcer for VariableSorterExecutorHooks {
+    fn variable_env_checker(&self, key: String, val: spin_app::Variable) -> anyhow::Result<()> {


variable_env_checker is a noun. I think the idea here is something like check_variable_has_value or check_variable_resolves.

itowlson · 2025-08-07T19:21:18Z

crates/trigger/src/cli/variables.rs

+            return self.check(key, val, dotenv_path, prefix);
+        }
+
+        Err(anyhow::anyhow!("No environment variable provider found"))


Not sure what "provider" refers to in this context. Have I misunderstood the purpose of this function and it's actually trying to check if the env provider is present?

I'd change that. It's meant to be "no default variable provided"

itowlson · 2025-08-07T19:24:50Z

crates/variables/src/lib.rs

+            _ = std::env::set_current_dir(path);
+        }
+
+        match std::env::var(env_key(prefix, &key)) {


You can't do this. There are providers other than the environment one. It is legal for a variable to come from Vault or Azure KeyVault or the static provider rather than the environment one. I tried to stress this in my previous review, sorry - this is fundamental.

New logic only allows no default variable before runtime if Provider it is of a Dynamic type Signed-off-by: Aminu Oluwaseun Joshua <[email protected]>

seun-ja · 2025-09-10T14:49:18Z

Closing this PR in favour of #3265. The commit history is messy and almost impossible to resolve

itowlson requested changes Aug 5, 2025

View reviewed changes

seun-ja requested a review from itowlson August 7, 2025 17:46

itowlson requested changes Aug 7, 2025

View reviewed changes

seun-ja force-pushed the spin-up-should-check-required-variables branch from fed8aaa to 7501996 Compare August 27, 2025 14:36

seun-ja marked this pull request as draft August 27, 2025 14:37

seun-ja marked this pull request as ready for review September 10, 2025 13:27

seun-ja force-pushed the spin-up-should-check-required-variables branch from a7f928c to 435c22b Compare September 10, 2025 14:06

seun-ja requested a review from itowlson September 10, 2025 14:07

seun-ja force-pushed the spin-up-should-check-required-variables branch from 435c22b to 2fed8a1 Compare September 10, 2025 14:14

Adds new dynamism metadata for Providers

b483012

New logic only allows no default variable before runtime if Provider it is of a Dynamic type Signed-off-by: Aminu Oluwaseun Joshua <[email protected]>

seun-ja force-pushed the spin-up-should-check-required-variables branch from 2fed8a1 to b483012 Compare September 10, 2025 14:20

seun-ja closed this Sep 10, 2025

Spin up should check required variables #3213

Spin up should check required variables #3213

Uh oh!

Conversation

seun-ja commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthik2804 commented Aug 6, 2025

Uh oh!

seun-ja commented Aug 6, 2025

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seun-ja Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seun-ja commented Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seun-ja commented Aug 5, 2025 •

edited

Loading

seun-ja Sep 9, 2025 •

edited

Loading