fix: type-aware application of template defaults (#14899)

[lfk]

Fixes #14899

Motivation

As illustrated in the referenced issue as well as via the example in the verification section, templateDefaults with container properties cause templates of other types (http, plugin) to fail outright.

Making use of templateDefaults should not prevent us from also using HTTP or Plugin templates, and vice-versa.

The issue can be remedied by ensuring that only relevant Template-type default values are applied while merging.

Modifications

• Implemented IsValid() bool method for TemplateType, similar to what already exists for e.g. PodGCStrategy
• Modified mergedTemplateDefaultsInto to only apply Template-type defaults for matching template types
• Added test case asserting that an HTTPTemplate is unaffected by Container default values

Verification

Successfully executed the following example workflow in a local (devcontainer) environment:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: http-template-defaults-test-
spec:
  entrypoint: main
  hooks:
    exit:
      template: notify-endpoint

  templateDefaults:
    container:
      env:
        - name: USERNAME
          value: "azurediamond"

  templates:
    - name: main
      container:
        image: alpine:latest
        command: [sh, -c, "echo 'Main task running...'; sleep 1"]

    - name: notify-endpoint
      http:
        url: "http://host.docker.internal:9999/notify"
        method: "POST"
        headers:
          - name: "Content-Type"
            value: "application/json"
        body: "{}"

Documentation

n/a

Summary by CodeRabbit

Release Notes

• New Features

  • Added template type validation for improved template handling.

• Bug Fixes

  • Improved template defaults to selectively apply only compatible defaults based on template type, preventing incorrect default inheritance across different template types.

• Tests

  • Added test coverage for template defaults with HTTP templates to ensure type-specific default handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[Joibel]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes address incorrect handling of HTTP templates in the controller. A new IsValid() method validates template types. The template default merging function now filters defaults by target template type using reflection before applying patches, preventing misapplication of container-specific defaults to non-container templates. A new test validates HTTP template type preservation when defaults are applied.

Changes

Cohort / File(s)	Summary
Type Validation pkg/apis/workflow/v1alpha1/workflow_types.go	Added IsValid() method on TemplateType returning true for Container, ContainerSet, Steps, Script, Resource, DAG, Suspend, Data, HTTP, and Plugin; false otherwise.
Default Merging Logic workflow/controller/operator.go	Reworked mergedTemplateDefaultsInto() to add type-aware filtering of template defaults. Uses reflection to identify applicable default fields by matching pointer types and inspecting struct fields. Applies filtered defaults selectively via strategic merge patch, then restores original template type.
Test Coverage workflow/controller/operator_tmpl_default_test.go	Added HTTP template fixture and test case HTTPTmplDefaultWf under TestSetTemplateDefault to verify HTTP templates retain their type when defaults are applied and do not inherit empty Container fields.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• workflow/controller/operator.go: The refactored mergedTemplateDefaultsInto() function introduces reflection-based filtering and rearranges the merge/unmarshal sequence. Careful review needed to ensure correct type-aware default application and no regressions across all template types (Container, Script, DAG, Resource, Plugin, etc.).
• Interaction between type validation and merge logic: Verify that the IsValid() method integrates properly with the filtering logic and that invalid template types are handled gracefully.
• Test coverage: Confirm the new HTTP test case adequately validates the fix and consider edge cases with other non-container template types receiving container-specific defaults.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: implementing type-aware application of template defaults to fix the issue where defaults were being incorrectly applied across different template types.
Description check	✅ Passed	The description follows the template structure with Motivation, Modifications, and Verification sections. It clearly explains the issue, lists the changes made, and provides a concrete example workflow demonstrating the fix.
Linked Issues check	✅ Passed	The PR addresses all key requirements from issue #14899: implements type-aware template defaults application, adds validation method for TemplateType, modifies mergedTemplateDefaultsInto to filter defaults by type, and includes a test case validating HTTP templates are unaffected by Container defaults.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the template defaults type-awareness issue: the IsValid() method addition, mergedTemplateDefaultsInto refactoring, and new test case are all in-scope and necessary for the fix.

✨ Finishing touches

• 📝 Docstrings were successfully generated.

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

workflow/controller/operator_tmpl_default_test.go (1)

273-289: LGTM! Test correctly validates the fix for issue #14899.

This test case effectively verifies that container-specific template defaults are not incorrectly applied to HTTP templates. The assertions properly confirm:

1. The template type remains HTTP after defaults are merged
2. No Container field is populated (remains nil)

This directly addresses the root cause described in the issue where HTTP templates were being treated as container/Pod templates.

Optional enhancement: Consider adding a similar test for Plugin templates, as they were also mentioned in the PR objectives as potentially affected by this issue. However, this is not necessary for the current fix.

workflow/controller/operator.go (1)

4286-4327: The fix correctly addresses the template type filtering issue.

The approach of using reflection to identify and zero out non-applicable template-type defaults is sound. The logic correctly:

1. Identifies pointer-to-struct fields that represent template types
2. Filters out defaults for types that don't match the target template
3. Applies remaining defaults via strategic merge patch
4. Preserves the original template type

The code relies on struct field names exactly matching TemplateType enum values (e.g., field Container → TemplateType("Container")). This coupling is correct and intentional—all Template struct fields (Container, ContainerSet, Script, HTTP, Plugin, DAG, Suspend, Data, Resource) have names that exactly match their corresponding enum values, and the reflection filter correctly selects only pointer-to-struct fields while skipping others like the Steps slice. A brief comment documenting this assumption would help future maintainers understand the implicit coupling between field names and enum values.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0b4a2d and 6234b14.

📒 Files selected for processing (3)

• pkg/apis/workflow/v1alpha1/workflow_types.go (1 hunks)
• workflow/controller/operator.go (1 hunks)
• workflow/controller/operator_tmpl_default_test.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

workflow/controller/operator_tmpl_default_test.go (3)

pkg/apis/workflow/v1alpha1/marshall.go (1)

• MustUnmarshalWorkflow (71-75)

test/e2e/fixtures/given.go (1)

• NoError (244-247)

pkg/apis/workflow/v1alpha1/workflow_types.go (1)

• TemplateTypeHTTP (55-55)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: Build Binaries (cli)
• GitHub Check: Build Binaries (controller)
• GitHub Check: Codegen
• GitHub Check: Unit Tests
• GitHub Check: Lint
• GitHub Check: Windows Unit Tests
• GitHub Check: argo-images (argocli)
• GitHub Check: argo-images (argoexec)

🔇 Additional comments (2)

pkg/apis/workflow/v1alpha1/workflow_types.go (1)

60-75: LGTM! Clean validation method.

The IsValid() implementation correctly validates all known template types and follows the existing pattern established by PodGCStrategy.IsValid() in this file. All 10 valid template types are properly included, and the method appropriately returns false for unknown types.

workflow/controller/operator_tmpl_default_test.go (1)

115-127: LGTM! Well-structured test fixture.

The httpWf constant provides a clean, minimal HTTP workflow fixture for testing template defaults behavior with HTTP templates. The YAML structure is valid and consistent with other test constants in this file.

[Joibel]

Thanks for contributing this

[coderabbitai]

Caution

Docstrings generation - FAILED

No docstrings were generated.

[argo-cd-cherry-pick-bot]

❌ Cherry-pick failed for 3.6. Please check the workflow logs for details.

[argo-cd-cherry-pick-bot]

🍒 Cherry-pick PR created for 3.7: #15173