-
Notifications
You must be signed in to change notification settings - Fork 20
Rehearse jobs that use changed ci-operator configs #101
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,7 @@ func getRehersalsHelper(logger *logrus.Entry, prNumber int) ([]*prowconfig.Presu | |
| candidateConfigPath := filepath.Join(candidatePath, config.ConfigInRepoPath) | ||
| candidateJobConfigPath := filepath.Join(candidatePath, config.JobConfigInRepoPath) | ||
| candidateCiopConfigPath := filepath.Join(candidatePath, config.CiopConfigInRepoPath) | ||
| masterCiopConfigPath := filepath.Join(masterPath, config.CiopConfigInRepoPath) | ||
| masterConfigPath := filepath.Join(masterPath, config.ConfigInRepoPath) | ||
| masterJobConfigPath := filepath.Join(masterPath, config.JobConfigInRepoPath) | ||
|
|
||
|
|
@@ -146,12 +147,19 @@ func getRehersalsHelper(logger *logrus.Entry, prNumber int) ([]*prowconfig.Presu | |
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load ci-operator config: %v", err) | ||
| } | ||
| ciopMasterConfig, err := config.CompoundLoad(masterCiopConfigPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load ci-operator config: %v", err) | ||
| } | ||
|
|
||
| changedPresubmits := diffs.GetChangedPresubmits(prowConfig, prowPRConfig, logger) | ||
| if len(changedPresubmits) == 0 { | ||
| return nil, fmt.Errorf("Empty changedPresubmits was not expected") | ||
| } | ||
|
|
||
| changedCiopConfigs := diffs.GetChangedCiopConfigs(ciopMasterConfig, ciopPrConfig, logger) | ||
| changedPresubmits.AddAll(diffs.GetPresubmitsForCiopConfigs(prowPRConfig, changedCiopConfigs, logger)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that you needed to do this in the test, too, is smelly
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, that test basically mimics our main(). |
||
|
|
||
| rehearsals := rehearse.ConfigureRehearsalJobs(changedPresubmits, ciopPrConfig, prNumber, rehearse.Loggers{Job: logger, Debug: logger}, false) | ||
| return rehearsals, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| prowconfig "k8s.io/test-infra/prow/config" | ||
| ) | ||
|
|
||
| type Presubmits map[string][]prowconfig.Presubmit | ||
|
|
||
| // AddAll adds all jobs from a different instance. | ||
| // The method assumes two jobs with a matching name for the same repository | ||
| // are identical, so so if a presubmit with a given name already exists, it | ||
| // is kept as is. | ||
| func (p Presubmits) AddAll(jobs Presubmits) { | ||
| for repo := range jobs { | ||
| if _, ok := p[repo]; !ok { | ||
| p[repo] = []prowconfig.Presubmit{} | ||
| } | ||
|
|
||
| for _, sourceJob := range jobs[repo] { | ||
| p.Add(repo, sourceJob) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add a presubmit for a given repo. | ||
| // The method assumes two jobs with a matching name are identical, so if | ||
| // a presubmit with a given name already exists, it is kept as is. | ||
| func (p Presubmits) Add(repo string, job prowconfig.Presubmit) { | ||
| for _, destJob := range p[repo] { | ||
| if destJob.Name == job.Name { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| p[repo] = append(p[repo], job) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "k8s.io/apimachinery/pkg/util/diff" | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| prowconfig "k8s.io/test-infra/prow/config" | ||
| ) | ||
|
|
||
| func TestPresubmitsAddAll(t *testing.T) { | ||
| testCases := []struct { | ||
| description string | ||
| source Presubmits | ||
| destination Presubmits | ||
| expected Presubmits | ||
| }{{ | ||
| description: "merge empty structure into empty structure", | ||
| }, { | ||
| description: "merge empty structure into non-empty structure", | ||
| source: Presubmits{}, | ||
| destination: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-repo"}}, | ||
| }}, | ||
| expected: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-repo"}}, | ||
| }}, | ||
| }, { | ||
| description: "merge non-empty structure into empty structure", | ||
| source: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-repo"}}, | ||
| }}, | ||
| destination: Presubmits{}, | ||
| expected: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-repo"}}, | ||
| }}, | ||
| }, { | ||
| description: "merge different jobs for a single repo, result should have both", | ||
| source: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-repo"}}, | ||
| }}, | ||
| destination: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-repo"}}}}, | ||
| expected: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-repo"}}, | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-repo"}}, | ||
| }}, | ||
| }, { | ||
| description: "merge jobs for different repos, result should have both", | ||
| source: Presubmits{"org/source-repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-source-repo"}}, | ||
| }}, | ||
| destination: Presubmits{"org/destination-repo": { | ||
| prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-destination-repo"}}}}, | ||
| expected: Presubmits{ | ||
| "org/source-repo": {prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "source-job-for-org-source-repo"}}}, | ||
| "org/destination-repo": {prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "destination-job-for-org-destination-repo"}}}, | ||
| }, | ||
| }, { | ||
| description: "merge jobs with same name for a single repo, result has the one originally in destination", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we need unique names to work this might be better as an error, but meh
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It cannot be an error. In rehearse, when both ci-operator config and a presubmit that uses it change, we'll want to rehearse the same job for two reasons, so it ends up being returned by two different "what jobs do we need to rehearse" methods, and those results need to be merged.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah |
||
| source: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{ | ||
| JobBase: prowconfig.JobBase{Name: "same-job-for-org-repo"}, | ||
| AlwaysRun: true, | ||
| }, | ||
| }}, | ||
| destination: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{ | ||
| JobBase: prowconfig.JobBase{Name: "same-job-for-org-repo"}, | ||
| AlwaysRun: false, | ||
| }}}, | ||
| expected: Presubmits{"org/repo": { | ||
| prowconfig.Presubmit{ | ||
| JobBase: prowconfig.JobBase{Name: "same-job-for-org-repo"}, | ||
| AlwaysRun: false, | ||
| }, | ||
| }}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.description, func(t *testing.T) { | ||
| tc.destination.AddAll(tc.source) | ||
|
|
||
| if !reflect.DeepEqual(tc.destination, tc.expected) { | ||
| t.Errorf("Presubmits differ from expected after AddAll:\n%s", diff.ObjectDiff(tc.expected, tc.destination)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestPresubmitsAdd(t *testing.T) { | ||
| testCases := []struct { | ||
| description string | ||
| presubmits Presubmits | ||
| repo string | ||
| job prowconfig.Presubmit | ||
| expected Presubmits | ||
| }{{ | ||
| description: "add job to new repo", | ||
| presubmits: Presubmits{}, | ||
| repo: "org/repo", | ||
| job: prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "job-for-org-repo"}}, | ||
| expected: Presubmits{"org/repo": {prowconfig.Presubmit{JobBase: prowconfig.JobBase{Name: "job-for-org-repo"}}}}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.description, func(t *testing.T) { | ||
| tc.presubmits.Add(tc.repo, tc.job) | ||
|
|
||
| if !reflect.DeepEqual(tc.expected, tc.presubmits) { | ||
| t.Errorf("Presubmits differ from expected after Add:\n%s", diff.ObjectDiff(tc.expected, tc.presubmits)) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ const ( | |
| CiopConfigInRepoPath = "ci-operator/config" | ||
| // TemplatesPath is the path of the templates from release repo | ||
| TemplatesPath = "ci-operator/templates" | ||
| // Name of the configmap that stores all ci-operator configs | ||
| CiOperatorConfigsCMName = "ci-operator-configs" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not true -- we are sharded. See the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aware of that, I want to tackle that tomorrow. IMHO it's worth a separate PR. I only moved this constant, not created it. Rehearse is a little broken after the sharding anyways.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #104 |
||
| ) | ||
|
|
||
| // ReleaseRepoConfig contains all configuration present in release repo (usually openshift/release) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the intention here is to add all the jobs that are using the changed ci-op config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the jobs that are using some of the changed ci-op configs, there could be more...
AddAllprevents job being present twice, so if a job was already intoRehearsebecause it was changed directly, it's not added again.