On a template change, rehearse all cluster types. #122
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
/cc @openshift/developer-productivity-test-platform |
stevekuznetsov
left a comment
stevekuznetsov
left a comment
There was a problem hiding this comment.
I'm still not super certain this is a good idea -- what information does this approach give us that running the job without changing it's command does not? We get a strict subset of information, right? Why not just run the job fully?
@stevekuznetsov Double thinking that, you are right. I guess is not a big deal to run the whole job. I will address that. |
openshift-ci-robot
added
size/L
droslean
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
droslean
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
droslean
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
droslean
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
bbguimaraes
left a comment
bbguimaraes
left a comment
There was a problem hiding this comment.
I think we can do better than random if we analyze the usage of templates, but this is OK as a first step. I left some comments on specific implementation details. Two suggestions about things that made the review more difficult than it could have been:
- Don't mix refactoring/fixes with features, specially in the same commit. Put them in separate commits, ideally at the beginning of the history (or in a separate PR) if they don't depend on the new code.
- Overall, we have been adding a lot of ad-hoc code that is very similar to existing code. We should try to consolidate new and existing code, refactoring as needed.
pkg/rehearse/jobs.go
Outdated
| return nil, nil | ||
| } | ||
|
|
||
| templateData, err := config.GetTemplateData(templates[templateKey]) |
There was a problem hiding this comment.
We decode the same templates (in config.GetTemplateData) countless times. We should store the value when they are first loaded. Many of the new "error handling" ifs we have are because of that function, even though it is guaranteed not to fail if it succeeded once.
There was a problem hiding this comment.
that's sound like a good follow-up
|
@openshift/developer-productivity-test-platform I would like to squash some commits before merging. |
|
Latest changes LGTM. I still think we are adding a lot more tech debt here than necessary, but am OK with addressing it in subsequent PRs. |
droslean
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
droslean
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
droslean
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@openshift/developer-productivity-test-platform: is there anything else that needs to be done here? This looks ready to me and I'd like to get the fixes to the integration tests merged ASAP. |
@bbguimaraes I wanted to have a final look today afternoon, but if it blocks you feel free to merge it |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
/lgtm
I must admit I was a bit hesitant to return to this one because the previous versions were a bit harder to review, but I really like how this one unfolded, I like the current version very much!
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
When a template file will change, rehearse all cluster types by choosing a random job that uses the corresponding template and cluster type.
Ignores the cluster types of jobs from the changed presubmits that uses the template.