rehearse: Get changed periodics

[droslean]

No description provided.

[droslean]

/cc @petr-muller @bbguimaraes @stevekuznetsov @hongkailiu

[stevekuznetsov]

I think we want the integration tests in this PR

[bbguimaraes]

Much of the refactoring here could have been done in separate commits (or even a separate PR), 7dad8b3 was especially difficult to review.

[droslean]

This should be ready. Only one thing that @stevekuznetsov mentioned, regarding how we want to treat the git-ref in the periodic jobs. I found out that most of the periodics are already using the git-ref and also some of them the have also the extra_refs. In my point of view we should leave them untouched, and leave the configuration to the user.
Also, another problem is how we will give information to the user for the rehearsed periodic job. @petr-muller suggested to have a presubmits that watches a periodic job, which is good idea. Other simplier way is to output in the deck's url.

[stevekuznetsov]

We should update crier upstream to have them report the same way postsubmits and presubmits do if they have extra_refs set. Until then, we should report it in the rehearse main job.

[stevekuznetsov]

I think we need to enforce that --git-ref is not set and to actually set it the same was as we do for presubmits, while setting extra_refs[0] to the rehearsal PR so that we get reports. Are we not also making this opt-in at a job level?

[stevekuznetsov]

@petr-muller @bbguimaraes would love your thoughts to add LGTM here

[petr-muller]

@petr-muller would love your thoughts to add LGTM here

Ack, I plan to have a look today

[bbguimaraes]

LGTM, but I still think we are adding too much duplication, and the kind that can generate bugs and/or make maintenance unnecessarily harder.

I would much rather see functions like these instead of duplicate code (not a mandate, take them as suggestions):

// pkg/diff/diff.go
func jobChanged(name string, job0, job1 *prowconfig.JobBase, logger *logrus.Entry, logFields *logrus.Fields) bool {
       if job1.Agent != string(pjapi.KubernetesAgent) {
               return false
       }
       if job0.Agent != string(pjapi.KubernetesAgent) {
               logger.WithFields(*logFields).WithFields(logrus.Fields{logJobName: name, logDiffs: convertToReadableDiff(job0.Agent, job1.Agent, objectAgent)}).Info(chosenJob)
               return true
       }
       if !equality.Semantic.DeepEqual(job0.Spec, job1.Spec) {
               logger.WithFields(*logFields).WithFields(logrus.Fields{logJobName: name, logDiffs: convertToReadableDiff(job0.Spec, job1.Spec, objectSpec)}).Info(chosenJob)
               return true
       }
       return false
}

// pkg/rehearse/jobs.go
func makeRehearsalJob(job *prowconfig.JobBase, prNumber int, name, repo, branch string) {
       job.Name = fmt.Sprintf("rehearse-%d-%s", prNumber, name)
       gitrefArg := fmt.Sprintf("--git-ref=%s@%s", repo, branch)
       job.Spec.Containers[0].Args = append(job.Spec.Containers[0].Args, gitrefArg)
       if job.Labels == nil {
               job.Labels = make(map[string]string, 1)
       }
       job.Labels[rehearseLabel] = strconv.Itoa(prNumber)
}

[droslean]

@bbguimaraes Will do a follow up for this.

[petr-muller]

1. I found myself agreeing with some of the comments @bbguimaraes made. I'd especially like to see the indirect arguments for JobConfigurer methods being cleaned up.

2. @droslean did you test the whole thing end-to-end (I mostly mean if the rehearsal periodics would really report their status to a PR)?

3. I'm a little confused about the opt-in mechanism we were discussing, where people would label their periodics as safe to rehearse. Unless I missed it, we don't have it? But maybe we don't need it, because we currently only rehearse periodics based on ci-operator with a safe set of options? I'm not sure..

4. IMO This is really missing documentation of what it is supposed to do (what periodics would be rehearsed? which wouldn't?), what are the limitations and what are the plans for next steps :/

5. I'm wondering if it would be useful to take a step back, define better what jobs are we supposed to rehearse and how, break this down to fewer smaller PRs and review them separately. Being this big, I'm not feeling exactly well about merging it, and I'm not sure it rehearses jobs that the users need to rehearse.

[petr-muller]

I think this API would be more readable if presubmitsToRehearse and periodicsToRehearse would be parameters to the methods that work with them instead of members that are only used in a single method.

[droslean]

After discussing this with @petr-muller we decide to break this PR in smaller ones to make the periodics introduction to the rehearse tool more stable and easy to follow up.

[petr-muller]

/lgtm
/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droslean, petr-muller

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [droslean,petr-muller]

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[petr-muller]

/refresh

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.