Templates: error on missing required value

[andig]

Fix #25930

TODO

• for tests instantiate templates in test mode to avoid required field errors
• verify ui tests

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The required-field check currently only runs when val != nil, so a required parameter that is completely missing or explicitly set to nil will not trigger an error; consider treating nil as missing for required params as well.
• Using reflect.ValueOf(val).IsZero() on every render may be heavier than necessary; if the values are expected to be simple types (e.g., strings, numbers), you could replace reflection with type-specific zero checks to keep this hot path lighter.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The required-field check currently only runs when `val != nil`, so a required parameter that is completely missing or explicitly set to `nil` will not trigger an error; consider treating `nil` as missing for required params as well.
- Using `reflect.ValueOf(val).IsZero()` on every render may be heavier than necessary; if the values are expected to be simple types (e.g., strings, numbers), you could replace reflection with type-specific zero checks to keep this hot path lighter.

## Individual Comments

### Comment 1
<location> `util/templates/template_test.go:48-50` </location>
<code_context>
+		},
+	}
+
+	_, _, err := tmpl.RenderResult(0, map[string]any{
+		"Param": "foo",
+	})
+	require.NoError(t, err)
+
</code_context>

<issue_to_address>
**suggestion (testing):** Assert on the error message content for the failing required case

Since the implementation returns a specific error message (e.g., `missing required \`param\``) when required validation fails, the test should assert on that message using `require.ErrorContains` or `require.EqualError`, not just that an error occurred. This will better guard against regressions where required-field validation changes or is removed.

Suggested implementation:

```golang
func TestRequired(t *testing.T) {
	tmpl := &Template{
		TemplateDefinition: TemplateDefinition{
			Params: []Param{
				{
					Name:     "param",
					Required: true,
				},
			},
		},
	}

	_, _, err := tmpl.RenderResult(0, map[string]any{})
	require.ErrorContains(t, err, "missing required `param`")

	_, _, err = tmpl.RenderResult(0, map[string]any{
		"Param": "foo",
	})
	require.NoError(t, err)
}

```

If the actual error string produced by the implementation differs (e.g., different wording or punctuation), update `"missing required \`param\`"` in the test to exactly match the real error text or a stable substring of it. Also, if the initial failing call currently uses a different argument (e.g., `nil` instead of `map[string]any{}`), apply the `require.ErrorContains` assertion at that call site without changing the arguments unless necessary.
</issue_to_address>

### Comment 2
<location> `util/templates/template_test.go:37-45` </location>
<code_context>
 }
+
+func TestRequired(t *testing.T) {
+	tmpl := &Template{
+		TemplateDefinition: TemplateDefinition{
+			Params: []Param{
+				{
+					Name:     "param",
+					Required: true,
+				},
+			},
+		},
+	}
+
</code_context>

<issue_to_address>
**question (testing):** Clarify or test the key casing behavior between `Param` and `param`

The template param is named `"param"`, but the test passes `"Param"` in the map. If case-insensitive matching is intended, please make that explicit (e.g., a dedicated `TestRequired_CaseInsensitiveKey` or a short comment). If not, align the `Name` and map key to avoid the test accidentally depending on unspecified behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

@naltatis with this change UI tests are failing due to missing required fields. The errors are real and probably due to faulty test cases. Could you take a look if it would be feasible to fix the tests?

[naltatis]

@andig found the issue. one test setup was indeed faulty. can we merge this? then I'll adapt #26149 accordingly.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The required-field validation currently checks reflect.ValueOf(s).IsZero() on the YAML-quoted string, which can mask truly empty values once quoted; consider validating directly on the original val (e.g., nil, empty string, or whitespace-only) before calling yamlQuote to avoid false negatives.
• Using testing.Testing() inside RenderResult couples production behavior to the test runtime; if possible, prefer an explicit parameter or configuration flag to control whether required-field checks run, rather than relying on the global testing state.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The required-field validation currently checks `reflect.ValueOf(s).IsZero()` on the YAML-quoted string, which can mask truly empty values once quoted; consider validating directly on the original `val` (e.g., nil, empty string, or whitespace-only) before calling `yamlQuote` to avoid false negatives.
- Using `testing.Testing()` inside `RenderResult` couples production behavior to the test runtime; if possible, prefer an explicit parameter or configuration flag to control whether required-field checks run, rather than relying on the global testing state.

## Individual Comments

### Comment 1
<location> `util/templates/template.go:361-363` </location>
<code_context>
 				}
+
+				// validate required fields from yaml
+				if p.IsRequired() && (renderMode == RenderModeUnitTest ||
+					renderMode == RenderModeInstance && !testing.Testing()) {
+					if rv := reflect.ValueOf(s); rv.IsZero() {
+						return nil, nil, fmt.Errorf("missing required `%s`", p.Name)
+					}
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Using the testing package in production code to gate behavior is surprising and may be brittle.

The condition `renderMode == RenderModeInstance && !testing.Testing()` ties runtime behavior to whether the process is running under `go test`, which is an unusual and brittle pattern for non-test code and can cause environment-specific behavior.

If you need to disable required-field validation in some contexts (like tests), prefer an explicit configuration/flag on `Template` or `RenderResult` instead of relying on `testing.Testing()`. That keeps behavior predictable and controllable for all callers and tooling.

Suggested implementation:

```golang
	"strings"
	"text/template"

	"github.com/Masterminds/sprig/v3"

	// SkipInstanceRequiredValidation can be set by callers (for example, in tests)
	// to disable required-field validation when rendering instances.
	var SkipInstanceRequiredValidation bool

```

```golang
				// validate required fields from yaml
				// Required-field validation is controlled explicitly via renderMode and
				// the SkipInstanceRequiredValidation flag rather than relying on
				// testing.Testing(), to keep behavior predictable for all callers.
				if p.IsRequired() && (renderMode == RenderModeUnitTest ||
					(renderMode == RenderModeInstance && !SkipInstanceRequiredValidation)) {

```

The added `SkipInstanceRequiredValidation` variable is declared at file scope in the imports block above; you may want to move it to a more appropriate location (e.g. near the type declarations for `Template` or related types) to match your project's style.

In your tests, you can now explicitly control behavior, for example:

```go
func TestSomething(t *testing.T) {
    old := SkipInstanceRequiredValidation
    SkipInstanceRequiredValidation = true
    defer func() { SkipInstanceRequiredValidation = old }()

    // ... test logic ...
}
```

If you prefer not to use a package-level variable, you can instead add a field (e.g. `SkipRequiredValidation bool`) on the appropriate struct (`Template`, `RenderResult`, or similar) and thread that through to this validation condition.
</issue_to_address>

### Comment 2
<location> `util/templates/template.go:363` </location>
<code_context>
+				// validate required fields from yaml
+				if p.IsRequired() && (renderMode == RenderModeUnitTest ||
+					renderMode == RenderModeInstance && !testing.Testing()) {
+					if rv := reflect.ValueOf(s); rv.IsZero() {
+						return nil, nil, fmt.Errorf("missing required `%s`", p.Name)
+					}
</code_context>

<issue_to_address>
**suggestion:** Reflect-based zero check on a string can be simplified and made clearer.

Since `s` is already a `string`, `reflect.ValueOf(s).IsZero()` is equivalent to `s == ""` but with extra overhead and indirection. A direct string check (optionally combined with `strings.TrimSpace` if you want to treat whitespace-only as missing) would be simpler, clearer, and faster.

Suggested implementation:

```golang
				// validate required fields from yaml
				if p.IsRequired() && (renderMode == RenderModeUnitTest ||
					renderMode == RenderModeInstance && !testing.Testing()) {
					if s == "" {
						return nil, nil, fmt.Errorf("missing required `%s`", p.Name)
					}
				}

```

If `reflect` is not used anywhere else in `util/templates/template.go`, remove the `reflect` import from the import block to keep the file clean and avoid an unused import error.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}