Support Jinja control flow in manifests (#93)

leynos · web-flow · commit fed97938a2a9 · 2025-08-12T01:53:57.000+01:00
* Add fixtures for control flow tests

* Enforce string keys in vars and expand Jinja tests

* Add yamllint ignore for Jinja fixtures
diff --git a/.yamllint b/.yamllint
@@ -0,0 +1,3 @@
+extends: default
+ignore: |
+  tests/data/jinja_for*.yml
diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md
@@ -161,9 +161,9 @@ level keys.
   future evolution of the schema while maintaining backward compatibility. This
   version string should be parsed and validated using the `semver` crate.[^4]
 
-- `vars`: A mapping of global key-value string pairs. These variables are
-  available for substitution in rule commands and target definitions and are
-  exposed to the Jinja templating context.
+- `vars`: A mapping of global key-value pairs. Keys must be strings. Values may
+  be strings, numbers, booleans, or sequences. These variables seed the Jinja
+  templating context and drive control flow within the manifest.
 
 - `macros`: An optional list of Jinja macro definitions. Each item provides a
   `signature` string using standard Jinja syntax and a `body` declared with the
@@ -414,7 +414,7 @@ pub struct NetsukeManifest {
     pub netsuke_version: Version,
 
     #[serde(default)]
-    pub vars: HashMap<String, String>,
+    pub vars: HashMap<String, serde_yml::Value>,
 
     #[serde(default)]
     pub rules: Vec<Rule>,
@@ -466,7 +466,7 @@ pub struct Target {
     pub order_only_deps: StringOrList,
 
     #[serde(default)]
-    pub vars: HashMap<String, String>,
+    pub vars: HashMap<String, serde_yml::Value>,
 
     /// Run this target when requested even if a file with the same name exists.
     #[serde(default)]
@@ -585,15 +585,14 @@ Unknown fields are rejected to surface user errors early. `StringOrList`
 provides a default `Empty` variant, so optional lists are trivial to represent.
 The manifest version is parsed using the `semver` crate to validate that it
 follows semantic versioning rules. Global and target variable maps now share
-the `HashMap<String, String>` type for consistency. This keeps YAML manifests
-concise while ensuring forward compatibility. Targets also accept optional
-`phony` and `always` booleans. They default to `false`, making it explicit when
-an action should run regardless of file timestamps. Targets listed in the
-`actions` section are deserialised using a custom helper so they are always
-treated as `phony` tasks. This ensures preparation actions never generate build
-artefacts. Convenience functions in `src/manifest.rs` load a manifest from a
-string or a file path, returning `anyhow::Result` for straightforward error
-handling.
+the `HashMap<String, serde_yml::Value>` type so booleans and sequences are
+preserved for Jinja control flow. Targets also accept optional `phony` and
+`always` booleans. They default to `false`, making it explicit when an action
+should run regardless of file timestamps. Targets listed in the `actions`
+section are deserialised using a custom helper so they are always treated as
+`phony` tasks. This ensures preparation actions never generate build artefacts.
+Convenience functions in `src/manifest.rs` load a manifest from a string or a
+file path, returning `anyhow::Result` for straightforward error handling.
 
 ### 3.5 Testing
 
@@ -666,6 +665,13 @@ lenient undefined behaviour. The resulting YAML is parsed to obtain the global
 variables, which are then injected into the environment before a second, strict
 render pass produces the final manifest for deserialisation.
 
+The parser copies `vars` values into the environment using
+`Value::from_serializable`. This preserves native YAML types so Jinja's
+`{% if %}` and `{% for %}` constructs can branch on booleans or iterate over
+sequences. Keys must be strings; any non-string key causes manifest parsing to
+fail. Attempting to iterate over a non-sequence results in a render error
+surfaced during manifest loading.
+
 ### 4.3 User-Defined Macros
 
 Netsuke allows users to declare reusable Jinja macros directly in the manifest.
diff --git a/docs/roadmap.md b/docs/roadmap.md
@@ -88,7 +88,7 @@ configurations with variables, control flow, and custom functions.
 
 - [ ] **Dynamic Features and Custom Functions:**
 
-  - [ ] Implement support for basic Jinja control structures (`{% if %}` and
+  - [x] Implement support for basic Jinja control structures (`{% if %}` and
         `{% for %}`)
 
   - [ ] Implement the foreach key for target generation.
diff --git a/src/ast.rs b/src/ast.rs
@@ -19,6 +19,10 @@
 
 use semver::Version;
 use serde::{Deserialize, Serialize, de::Deserializer};
+use std::collections::HashMap;
+
+/// Map type for `vars` blocks, preserving YAML values.
+pub type Vars = HashMap<String, serde_yml::Value>;
 
 fn deserialize_actions<'de, D>(deserializer: D) -> Result<Vec<Target>, D::Error>
 where
@@ -30,7 +34,6 @@ where
     }
     Ok(actions)
 }
-use std::collections::HashMap;
 
 /// Top-level manifest structure parsed from a `Netsukefile`.
 ///
@@ -61,7 +64,7 @@ pub struct NetsukeManifest {
 
     /// Global key/value pairs available to recipes.
     #[serde(default)]
-    pub vars: HashMap<String, String>,
+    pub vars: Vars,
 
     /// Named rule templates that can be referenced by targets.
     #[serde(default)]
@@ -187,7 +190,7 @@ pub struct Target {
 
     /// Target-scoped variables available during command execution.
     #[serde(default)]
-    pub vars: HashMap<String, String>,
+    pub vars: Vars,
 
     /// Declares that the target does not correspond to a real file.
     #[serde(default)]
diff --git a/src/manifest.rs b/src/manifest.rs
@@ -5,9 +5,9 @@
 //! They wrap `serde_yml` and add basic file handling.
 
 use crate::ast::NetsukeManifest;
-use anyhow::{Context, Result};
+use anyhow::{Context, Result, bail};
 use minijinja::{Environment, UndefinedBehavior, context, value::Value};
-use std::{collections::HashMap, fs, path::Path};
+use std::{fs, path::Path};
 
 /// Parse a manifest string using Jinja for templating.
 ///
@@ -48,22 +48,16 @@ pub fn from_str(yaml: &str) -> Result<NetsukeManifest> {
 
     let doc: serde_yml::Value =
         serde_yml::from_str(&rendered).context("first-pass YAML parse error")?;
-    let vars = doc
-        .get("vars")
-        .and_then(|v| v.as_mapping())
-        .map(|m| {
-            m.iter()
-                .filter_map(|(k, v)| k.as_str().and_then(|key| v.as_str().map(|val| (key, val))))
-                .map(|(k, v)| (k.to_string(), v.to_string()))
-                .collect::<HashMap<_, _>>()
-        })
-        .unwrap_or_default();
-
-    // Populate the environment with the extracted variables for subsequent
-    // rendering. Undefined variables now trigger errors to surface template
-    // mistakes early.
-    for (key, value) in vars {
-        env.add_global(key, Value::from(value));
+    if let Some(vars) = doc.get("vars").and_then(|v| v.as_mapping()) {
+        // Copy each key-value pair into the environment, preserving native YAML
+        // types so control structures like `{% if %}` and `{% for %}` can operate
+        // on booleans and sequences.
+        for (k, v) in vars {
+            let Some(key) = k.as_str() else {
+                bail!("non-string key in 'vars' mapping: {k:?}");
+            };
+            env.add_global(key, Value::from_serialize(v.clone()));
+        }
     }
 
     env.set_undefined_behavior(UndefinedBehavior::Strict);
diff --git a/tests/data/jinja_for.yml b/tests/data/jinja_for.yml
@@ -0,0 +1,11 @@
+netsuke_version: 1.0.0
+vars:
+  items:
+    - foo
+    - bar
+targets:
+{% for item in items %}
+  - name: "{{ item }}"
+    command: "echo {{ item }}"
+{% endfor %}
+
diff --git a/tests/data/jinja_for_invalid.yml b/tests/data/jinja_for_invalid.yml
@@ -0,0 +1,9 @@
+netsuke_version: 1.0.0
+vars:
+  items: 1
+targets:
+{% for item in items %}
+  - name: "{{ item }}"
+    command: "echo {{ item }}"
+{% endfor %}
+
diff --git a/tests/data/jinja_if.yml b/tests/data/jinja_if.yml
@@ -0,0 +1,6 @@
+netsuke_version: 1.0.0
+vars:
+  enable: true
+targets:
+  - name: hello
+    command: "{% if enable %}echo on{% else %}echo off{% endif %}"
diff --git a/tests/data/jinja_if_disabled.yml b/tests/data/jinja_if_disabled.yml
@@ -0,0 +1,6 @@
+netsuke_version: 1.0.0
+vars:
+  enable: false
+targets:
+  - name: hello
+    command: "{% if enable %}echo on{% else %}echo off{% endif %}"
diff --git a/tests/features/manifest.feature b/tests/features/manifest.feature
@@ -55,3 +55,29 @@ Feature: Manifest Parsing
     Given the manifest file "tests/data/jinja_undefined.yml" is parsed
     When the parsing result is checked
     Then parsing the manifest fails
+
+  Scenario: Rendering Jinja conditionals in a manifest
+    Given the manifest file "tests/data/jinja_if.yml" is parsed
+    When the manifest is checked
+    Then the first target name is "hello"
+    And the first target command is "echo on"
+
+  Scenario: Rendering Jinja conditionals in a manifest (disabled)
+    Given the manifest file "tests/data/jinja_if_disabled.yml" is parsed
+    When the manifest is checked
+    Then the first target name is "hello"
+    And the first target command is "echo off"
+
+  Scenario: Rendering Jinja loops in a manifest
+    Given the manifest file "tests/data/jinja_for.yml" is parsed
+    When the manifest is checked
+    Then the manifest has 2 targets
+    And the target 1 name is "foo"
+    And the target 1 command is "echo foo"
+    And the target 2 name is "bar"
+    And the target 2 command is "echo bar"
+
+  Scenario: Parsing fails when a Jinja loop iterates over a non-list
+    Given the manifest file "tests/data/jinja_for_invalid.yml" is parsed
+    When the parsing result is checked
+    Then parsing the manifest fails
diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs
@@ -3,18 +3,17 @@
 use netsuke::{ast::Recipe, manifest};
 use rstest::rstest;
 
+fn manifest_yaml(body: &str) -> String {
+    format!("netsuke_version: 1.0.0\n{body}")
+}
+
 #[rstest]
 fn renders_global_vars() {
-    let yaml = r"
-netsuke_version: 1.0.0
-vars:
-  who: world
-targets:
-  - name: hello
-    command: echo {{ who }}
-";
-
-    let manifest = manifest::from_str(yaml).expect("parse");
+    let yaml = manifest_yaml(
+        "vars:\n  who: world\ntargets:\n  - name: hello\n    command: echo {{ who }}\n",
+    );
+
+    let manifest = manifest::from_str(&yaml).expect("parse");
     let first = manifest.targets.first().expect("target");
     if let Recipe::Command { command } = &first.recipe {
         assert_eq!(command, "echo world");
@@ -25,24 +24,96 @@ targets:
 
 #[rstest]
 fn undefined_variable_errors() {
-    let yaml = r"
-netsuke_version: 1.0.0
-targets:
-  - name: hello
-    command: echo {{ missing }}
-";
+    let yaml = manifest_yaml("targets:\n  - name: hello\n    command: echo {{ missing }}\n");
 
-    assert!(manifest::from_str(yaml).is_err());
+    assert!(manifest::from_str(&yaml).is_err());
 }
 
 #[rstest]
 fn syntax_error_errors() {
-    let yaml = r"
-netsuke_version: 1.0.0
-targets:
-  - name: hello
-    command: echo {{ who
-";
-
-    assert!(manifest::from_str(yaml).is_err());
+    let yaml = manifest_yaml("targets:\n  - name: hello\n    command: echo {{ who\n");
+
+    assert!(manifest::from_str(&yaml).is_err());
+}
+
+#[rstest]
+#[case(true, "echo on")]
+#[case(false, "echo off")]
+fn renders_if_blocks(#[case] flag: bool, #[case] expected: &str) {
+    let cmd = "{% if flag %}echo on{% else %}echo off{% endif %}";
+    let yaml = manifest_yaml(&format!(
+        concat!(
+            "vars:\n",
+            "  flag: {flag}\n",
+            "targets:\n",
+            "  - name: test\n",
+            "    command: {cmd}\n",
+        ),
+        flag = flag,
+        cmd = cmd,
+    ));
+
+    let manifest = manifest::from_str(&yaml).expect("parse");
+    let first = manifest.targets.first().expect("target");
+    if let Recipe::Command { command } = &first.recipe {
+        assert_eq!(command, expected);
+    } else {
+        panic!("Expected command recipe, got: {:?}", first.recipe);
+    }
+}
+
+#[rstest]
+fn renders_for_loops() {
+    let yaml = manifest_yaml(
+        "vars:\n  items:\n    - a\n    - b\ntargets:\n{% for item in items %}\n  - name: \"{{ item }}\"\n    command: \"echo {{ item }}\"\n{% endfor %}\n",
+    );
+
+    let manifest = manifest::from_str(&yaml).expect("parse");
+    assert_eq!(manifest.targets.len(), 2);
+    let names: Vec<_> = manifest
+        .targets
+        .iter()
+        .map(|t| match &t.name {
+            netsuke::ast::StringOrList::String(s) => s.clone(),
+            other => panic!("Expected String, got: {other:?}"),
+        })
+        .collect();
+    assert_eq!(names, vec!["a", "b"]);
+
+    let commands: Vec<_> = manifest
+        .targets
+        .iter()
+        .map(|t| match &t.recipe {
+            Recipe::Command { command } => command.clone(),
+            other => panic!("Expected command recipe, got: {other:?}"),
+        })
+        .collect();
+    assert_eq!(commands, vec!["echo a", "echo b"]);
+}
+
+#[rstest]
+fn for_loop_non_iterable_errors() {
+    let yaml = manifest_yaml(
+        "vars:\n  items: 1\ntargets:\n{% for item in items %}\n  - name: \"{{ item }}\"\n    command: \"echo {{ item }}\"\n{% endfor %}\n",
+    );
+
+    assert!(manifest::from_str(&yaml).is_err());
+}
+
+#[rstest]
+fn undefined_in_if_errors() {
+    let yaml = manifest_yaml(
+        "targets:\n  - name: test\n    command: {% if missing %}echo hi{% endif %}\n",
+    );
+
+    assert!(manifest::from_str(&yaml).is_err());
+}
+
+#[rstest]
+fn undefined_in_for_errors() {
+    let yaml = manifest_yaml(
+        "targets:\n{% for item in missing %}\n  - name: \"{{ item }}\"\n    command: \"echo {{ item }}\"\n{% endfor %}\n",
+    );
+
+    assert!(manifest::from_str(&yaml).is_err());
 }
diff --git a/tests/steps/manifest_steps.rs b/tests/steps/manifest_steps.rs

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+extends: default`
	`2`	`+ignore: \|`
	`3`	`+ tests/data/jinja_for*.yml`