Skip to content

Fix template execution test failure#4

Open
abhinavshashank wants to merge 1 commit intoece9bb928fa4-amg-DO-NOT-DELETEfrom
v10-cve-fix-corrected
Open

Fix template execution test failure#4
abhinavshashank wants to merge 1 commit intoece9bb928fa4-amg-DO-NOT-DELETEfrom
v10-cve-fix-corrected

Conversation

@abhinavshashank
Copy link

@abhinavshashank abhinavshashank commented Feb 16, 2026

Fix template execution test failure

Fixes the cause of failure for TestEmailNotifierIntegration test where executeTextString parsed template names as text instead of executing them. Email subjects came back empty because "default.title" was treated as literal text to parse, not a template name to execute.

TestEmailNotifierIntegration - expected "[FIRING:1] (AlwaysFiring warning)", got empty string.

Test failure: https://github.com/aws-observability/aws-grafana/actions/runs/22005371748/job/63593865329?pr=1145

@abhinavshashank abhinavshashank changed the title Fix executeTextString to properly execute named templates with size l… Fix template execution test failure Feb 16, 2026
Copy link
Member

@chriscerie chriscerie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please link the failing test for context
  • How is this tested?

Comment on lines +384 to +394
// resetTimeNow resets the global variable timeNow to the default value, which is time.Now
func resetTimeNow() {
timeNow = time.Now
}

func mockTimeNow(constTime time.Time) func() {
timeNow = func() time.Time {
return constTime
}
return resetTimeNow
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used by templates/template_data_test.go which was added in the original CVE fix PR (#2) calls mockTimeNow() but the function wasn't
defined anywhere. Found this out when aws-grafana CI ran tests.

}
textTmpl := tmpltext.New("").Option("missingkey=zero")
textTmpl, err := textTmpl.Parse(text)
func executeTextStringWithLimit(tmpl *Template, name string, data *ExtendedData) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these names changing? Is this change backported?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are our changes, not a direct backport from upstream.

The upstream v11.6.x fix (grafana/alerting@35f3427554ec) uses tmpl.Text() to get the base
template before applying size limits. But tmpl.Text() doesn't exist in v10's
prometheus-alertmanager (requires Go 1.22+, v10 uses Go 1.21).

So changed the fix to call tmpl.ExecuteTextString(name, data) (the existing v10 method)
and check output size through LimitedWriter afterward. S

The func rename was just to clarify that this now forces size limits.

@abhinavshashank
Copy link
Author

  • Please link the failing test for context
     Tests failing: https://github.com/aws-observability/aws-grafana/actions/runs/22005371748/job/63593865329?pr=1145
  • How is this tested?
      Full aws-grafana CI verification will happen after this is merged and the aws-grafana PR (#1145) is updated with the new commit.

@chriscerie
Copy link
Member

Full aws-grafana CI verification will happen after this is merged and the aws-grafana PR (#1145) is updated with the new commit.

Please build an image with this commit to test in preprod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants