Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for testrgrid alert emails on release-blocking jobs #19892

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

hasheddan
Copy link
Contributor

Adds a check that release-blocking jobs have testgrid alert emails. Logs
violations for now, but should be updated to error on violations when
all jobs pass.

Signed-off-by: hasheddan [email protected]

xref: kubernetes/sig-release#1216 #19844 (comment)

/assign @justaugustus @cpanato

Adds a check that release-blocking jobs have testgrid alert emails. Logs
violations for now, but should be updated to error on violations when
all jobs pass.

Signed-off-by: hasheddan <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 9, 2020
@hasheddan
Copy link
Contributor Author

/cc @kubernetes/ci-signal

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/config Issues or PRs related to code in /config labels Nov 9, 2020
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 9, 2020
@RobertKielty
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2020
@hasheddan
Copy link
Contributor Author

/assign @spiffxp

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chases2, hasheddan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 813f232 into kubernetes:master Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 10, 2020
@cpanato
Copy link
Member

cpanato commented Nov 10, 2020

Thanks for this @hasheddan

@spiffxp
Copy link
Member

spiffxp commented Nov 10, 2020

Sorry I missed this. We should revert this, because there is already a test that does this, and catches more corner cases. It is possible to configure test grid alerts outside of prowjob files, so just checking prow job annotations is insufficient.

Instead, we should build on this test:

func TestReleaseBlockingJobsMustHaveTestgridDescriptions(t *testing.T) {
// TODO(spiffxp): start with master, enforce for all release branches
re := regexp.MustCompile("^sig-release-master-(blocking|informing)$")
for _, dashboard := range cfg.Dashboards {
if !re.MatchString(dashboard.Name) {
continue
}
suffix := re.FindStringSubmatch(dashboard.Name)[1]
for _, dashboardtab := range dashboard.DashboardTab {
intro := fmt.Sprintf("dashboard_tab %v/%v is release-%v", dashboard.Name, dashboardtab.Name, suffix)
if dashboardtab.Name == "" {
t.Errorf("%v: - Must have a name", intro)
}
if dashboardtab.TestGroupName == "" {
t.Errorf("%v: - Must have a test_group_name", intro)
}
if dashboardtab.Description == "" {
t.Errorf("%v: - Must have a description", intro)
}
// TODO(spiffxp): enforce for informing as well
if suffix == "informing" {
if !strings.HasPrefix(dashboardtab.Description, "OWNER: ") {
t.Logf("NOTICE: %v: - Must have a description that starts with OWNER: ", intro)
}
if dashboardtab.AlertOptions == nil {
t.Logf("NOTICE: %v: - Must have alert_options (ensure informing dashboard is listed first in testgrid-dashboards)", intro)
} else if dashboardtab.AlertOptions.AlertMailToAddresses == "" {
t.Logf("NOTICE: %v: - Must have alert_options.alert_mail_to_addresses", intro)
}
} else {
if dashboardtab.AlertOptions == nil {
t.Errorf("%v: - Must have alert_options (ensure blocking dashboard is listed first in testgrid-dashboards)", intro)
} else if dashboardtab.AlertOptions.AlertMailToAddresses == "" {
t.Errorf("%v: - Must have alert_options.alert_mail_to_addresses", intro)
}
}
}
}
}

It requires a testgrid.proto file to be generated (using test grid files and prow job files as source), and then looks at the test grid options. If you invoke this with bazel, that proto file is generated automatically (see #18599 (comment))

@hasheddan
Copy link
Contributor Author

@spiffxp thanks for the info and apologies for missing this existing test. I'll revert and then update the existing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants