Skip to content

Make rehearsals of hidden jobs have skip_report: true #346

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 3 commits into from
Jan 8, 2020
Merged

Make rehearsals of hidden jobs have skip_report: true #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ac06d8
Use `cmp.Diff` instead of `diff.Object*Diff`
petr-muller Jan 3, 2020
ec1673d
Remove some dead code from tests
petr-muller Jan 3, 2020
ab17840
Make rehearsals of hidden jobs skip_report: true
petr-muller Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions pkg/rehearse/jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,24 @@ func makeRehearsalPresubmit(source *prowconfig.Presubmit, repo string, prNumber
}
rehearsal.Labels[rehearseLabel] = strconv.Itoa(prNumber)

// Rehearsals of hidden jobs are also hidden jobs, but they are "special"
// by being hidden jobs on a repository that should not have hidden
// jobs (openshift/release). This causes problems because the config makes
// jobs on `openshift/release` to have URLs in GH to the public Deck/Spyglass
// instance, but that instance does not have access to the underlying private
// storage so it cannot show the rehearsal's logs and artifacts.
//
// Additionally, whoever triggers a rehearsal of a hidden job is unlikely to
// have access to the Deck instance that shows hidden jobs, so they would not
// be able to access the logs even if the URL were correct.
//
// Therefore, we make such jobs skip reporting to GH to avoid confusion
//
// Note: "hidden" means the Deck concept of hidden jobs
if rehearsal.Hidden {
rehearsal.SkipReport = true
}

return &rehearsal, nil
}

Expand Down
90 changes: 57 additions & 33 deletions pkg/rehearse/jobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (

"github.com/getlantern/deepcopy"
"github.com/ghodss/yaml"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/sirupsen/logrus"
logrustest "github.com/sirupsen/logrus/hooks/test"

Expand All @@ -22,7 +24,6 @@ import (
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/watch"

Expand All @@ -36,6 +37,8 @@ import (

const testingRegistry = "../../test/multistage-registry/registry"

var ignoreUnexported = cmpopts.IgnoreUnexported(prowconfig.Presubmit{}, prowconfig.Brancher{}, prowconfig.RegexpChangeMatcher{})

func TestReplaceClusterProfiles(t *testing.T) {
makeVolume := func(name string) v1.Volume {
return v1.Volume{
Expand Down Expand Up @@ -127,7 +130,7 @@ func TestReplaceClusterProfiles(t *testing.T) {
}

if !reflect.DeepEqual(tc.expected, names) {
t.Fatal(diff.ObjectDiff(tc.expected, names))
t.Fatal(cmp.Diff(tc.expected, names))
}
})
}
Expand Down Expand Up @@ -232,14 +235,14 @@ func TestInlineCiopConfig(t *testing.T) {
}

if !equality.Semantic.DeepEqual(expectedJob, job) {
t.Errorf("Returned job differs from expected:\n%s", diff.ObjectReflectDiff(expectedJob, job))
t.Errorf("Returned job differs from expected:\n%s", cmp.Diff(expectedJob, job, ignoreUnexported))
}
}
})
}
}

func makeTestingPresubmit(name, context string, org, repo, branch string) *prowconfig.Presubmit {
func makeTestingPresubmit(name, context, branch string) *prowconfig.Presubmit {
return &prowconfig.Presubmit{
JobBase: prowconfig.JobBase{
Agent: "kubernetes",
Expand Down Expand Up @@ -279,15 +282,20 @@ func TestMakeRehearsalPresubmit(t *testing.T) {
Reporter: prowconfig.Reporter{Context: "ci/prow/test"},
Brancher: prowconfig.Brancher{Branches: []string{"^branch$"}},
}
hiddenPresubmit := &prowconfig.Presubmit{}
deepcopy.Copy(hiddenPresubmit, sourcePresubmit)
hiddenPresubmit.Hidden = true

testCases := []struct {
testID string
refs *pjapi.Refs
original *prowconfig.Presubmit
expectedPresubmit *prowconfig.Presubmit
}{
{
testID: "job that belong to different org/repo than refs",
refs: &pjapi.Refs{Org: "anotherOrg", Repo: "anotherRepo"},
testID: "job that belong to different org/repo than refs",
refs: &pjapi.Refs{Org: "anotherOrg", Repo: "anotherRepo"},
original: sourcePresubmit,
expectedPresubmit: func() *prowconfig.Presubmit {
p := &prowconfig.Presubmit{}
deepcopy.Copy(p, sourcePresubmit)
Expand All @@ -310,8 +318,9 @@ func TestMakeRehearsalPresubmit(t *testing.T) {
}(),
},
{
testID: "job that belong to the same org/repo with refs.",
refs: &pjapi.Refs{Org: "org", Repo: "repo"},
testID: "job that belong to the same org/repo with refs.",
refs: &pjapi.Refs{Org: "org", Repo: "repo"},
original: sourcePresubmit,
expectedPresubmit: func() *prowconfig.Presubmit {
p := &prowconfig.Presubmit{}
deepcopy.Copy(p, sourcePresubmit)
Expand All @@ -326,8 +335,27 @@ func TestMakeRehearsalPresubmit(t *testing.T) {
}(),
},
{
testID: "job that belong to the same org but different repo than refs.",
refs: &pjapi.Refs{Org: "org", Repo: "anotherRepo"},
testID: "hidden job that belong to the same org/repo with refs.",
refs: &pjapi.Refs{Org: "org", Repo: "repo"},
original: hiddenPresubmit,
expectedPresubmit: func() *prowconfig.Presubmit {
p := &prowconfig.Presubmit{}
deepcopy.Copy(p, hiddenPresubmit)

p.Name = "rehearse-123-pull-ci-org-repo-branch-test"
p.Labels = map[string]string{rehearseLabel: "123"}
p.Spec.Containers[0].Args = []string{"arg1", "arg2"}
p.RerunCommand = "/test pj-rehearse"
p.Context = "ci/rehearse/org/repo/branch/test"
p.Optional = true
p.SkipReport = true
return p
}(),
},
{
testID: "job that belong to the same org but different repo than refs.",
refs: &pjapi.Refs{Org: "org", Repo: "anotherRepo"},
original: sourcePresubmit,
expectedPresubmit: func() *prowconfig.Presubmit {
p := &prowconfig.Presubmit{}
deepcopy.Copy(p, sourcePresubmit)
Expand All @@ -353,12 +381,12 @@ func TestMakeRehearsalPresubmit(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.testID, func(t *testing.T) {
rehearsal, err := makeRehearsalPresubmit(sourcePresubmit, testRepo, testPrNumber, tc.refs)
rehearsal, err := makeRehearsalPresubmit(tc.original, testRepo, testPrNumber, tc.refs)
if err != nil {
t.Errorf("Unexpected error in makeRehearsalPresubmit: %v", err)
}
if !equality.Semantic.DeepEqual(tc.expectedPresubmit, rehearsal) {
t.Errorf("Expected rehearsal Presubmit differs:\n%s", diff.ObjectDiff(tc.expectedPresubmit, rehearsal))
t.Errorf("Expected rehearsal Presubmit differs:\n%s", cmp.Diff(tc.expectedPresubmit, rehearsal, ignoreUnexported))
}

})
Expand Down Expand Up @@ -445,8 +473,6 @@ func makeSuccessfulFinishReactor(watcher watch.Interface, jobs map[string][]prow
func TestExecuteJobsErrors(t *testing.T) {
testPrNumber, testNamespace, testRepoPath, testRefs := makeTestData()
targetOrgRepo := "targetOrg/targetRepo"
targetOrg := "targetOrg"
targetRepo := "targetRepo"
testCiopConfigs := config.ByFilename{}

testCases := []struct {
Expand All @@ -456,14 +482,14 @@ func TestExecuteJobsErrors(t *testing.T) {
}{{
description: "fail to Create a prowjob",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
}},
failToCreate: sets.NewString("rehearse-123-job1"),
}, {
description: "fail to Create one of two prowjobs",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", "master"),
}},
failToCreate: sets.NewString("rehearse-123-job2"),
}}
Expand Down Expand Up @@ -507,8 +533,6 @@ func TestExecuteJobsErrors(t *testing.T) {

func TestExecuteJobsUnsuccessful(t *testing.T) {
testPrNumber, testNamespace, testRepoPath, testRefs := makeTestData()
targetOrg := "targetOrg"
targetRepo := "targetRepo"
targetOrgRepo := "targetOrg/targetRepo"
testCiopConfigs := config.ByFilename{}

Expand All @@ -519,20 +543,20 @@ func TestExecuteJobsUnsuccessful(t *testing.T) {
}{{
description: "single job that fails",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
}},
results: map[string]pjapi.ProwJobState{"rehearse-123-job1": pjapi.FailureState},
}, {
description: "single job that aborts",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
}},
results: map[string]pjapi.ProwJobState{"rehearse-123-job1": pjapi.AbortedState},
}, {
description: "one job succeeds, one fails",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", "master"),
}},
results: map[string]pjapi.ProwJobState{
"rehearse-123-job1": pjapi.SuccessState,
Expand Down Expand Up @@ -599,8 +623,8 @@ func TestExecuteJobsPositive(t *testing.T) {
}{{
description: "two jobs in a single repo",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", "master"),
}},
expectedJobs: []pjapi.ProwJobSpec{
makeTestingProwJob(testNamespace,
Expand All @@ -614,8 +638,8 @@ func TestExecuteJobsPositive(t *testing.T) {
}}, {
description: "two jobs in a single repo, same context but different branch",
jobs: map[string][]prowconfig.Presubmit{targetOrgRepo: {
*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", targetOrg, targetRepo, "not-master"),
*makeTestingPresubmit("job1", "ci/prow/job1", "master"),
*makeTestingPresubmit("job2", "ci/prow/job2", "not-master"),
}},
expectedJobs: []pjapi.ProwJobSpec{
makeTestingProwJob(testNamespace,
Expand All @@ -630,8 +654,8 @@ func TestExecuteJobsPositive(t *testing.T) {
{
description: "two jobs in a separate repos",
jobs: map[string][]prowconfig.Presubmit{
targetOrgRepo: {*makeTestingPresubmit("job1", "ci/prow/job1", targetOrg, targetRepo, "master")},
anotherTargetOrgRepo: {*makeTestingPresubmit("job2", "ci/prow/job2", anotherTargetOrg, anotherTargetRepo, "master")},
targetOrgRepo: {*makeTestingPresubmit("job1", "ci/prow/job1", "master")},
anotherTargetOrgRepo: {*makeTestingPresubmit("job2", "ci/prow/job2", "master")},
},
expectedJobs: []pjapi.ProwJobSpec{
makeTestingProwJob(testNamespace,
Expand Down Expand Up @@ -696,7 +720,7 @@ func TestExecuteJobsPositive(t *testing.T) {
sort.Slice(createdJobSpecs, func(a, b int) bool { return createdJobSpecs[a].Job < createdJobSpecs[b].Job })

if !equality.Semantic.DeepEqual(tc.expectedJobs, createdJobSpecs) {
t.Errorf("Created ProwJobs differ from expected:\n%s", diff.ObjectReflectDiff(tc.expectedJobs, createdJobSpecs))
t.Errorf("Created ProwJobs differ from expected:\n%s", cmp.Diff(tc.expectedJobs, createdJobSpecs, ignoreUnexported))
}
})
}
Expand Down Expand Up @@ -1024,7 +1048,7 @@ func TestReplaceCMTemplateName(t *testing.T) {
replaceCMTemplateName(testCase.jobVolumeMounts, testCase.jobVolumes, templates)
expected := testCase.expectedToFind()
if !reflect.DeepEqual(expected, testCase.jobVolumes) {
t.Fatalf("Diff found %v", diff.ObjectReflectDiff(expected, testCase.jobVolumes))
t.Fatalf("Diff found %v", cmp.Diff(expected, testCase.jobVolumes))
}
})
}
Expand Down Expand Up @@ -1110,7 +1134,7 @@ func TestGetClusterTypes(t *testing.T) {
t.Run(tc.id, func(t *testing.T) {
ret := getClusterTypes(tc.jobs)
if !reflect.DeepEqual(tc.want, ret) {
t.Fatal(diff.ObjectDiff(tc.want, ret))
t.Fatal(cmp.Diff(tc.want, ret))
}
})
}
Expand Down Expand Up @@ -1160,7 +1184,7 @@ func TestRemoveConfigResolverFlags(t *testing.T) {
t.Run(testCase.description, func(t *testing.T) {
newArgs := removeConfigResolverFlags(testCase.input)
if !reflect.DeepEqual(testCase.expected, newArgs) {
t.Fatalf("Diff found %v", diff.ObjectReflectDiff(testCase.expected, newArgs))
t.Fatalf("Diff found %v", cmp.Diff(testCase.expected, newArgs))
}
})
}
Expand Down
89 changes: 89 additions & 0 deletions vendor/github.com/google/go-cmp/cmp/cmpopts/equate.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading