[WIP] new test type design #85
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbguimaraes 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 |
openshift-ci-robot
added
the
approved
pkg/steps/new_e2e.go
Outdated
| errs = append(errs, fmt.Errorf("failed to create secret: %v", err)) | ||
| } | ||
| if errs == nil { | ||
| if err = s.runSteps(ctx, s.tests, secret); err != nil { |
There was a problem hiding this comment.
This should remain err := ?
pkg/steps/new_e2e.go
Outdated
| if secret, err = createSecret(s.secretClient, s.jobSpec.Namespace, s.artifactDir); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to create secret: %v", err)) | ||
| } | ||
| if errs == nil { |
There was a problem hiding this comment.
The logic here is getting a bit nuts. Why not just return when we can't make the secret?
There was a problem hiding this comment.
Depends if we want post if pre failed.
pkg/steps/new_e2e.go
Outdated
| var errs []error | ||
| if err := s.runSteps(ctx, s.pre); err != nil { | ||
| var err error | ||
| if err = s.runSteps(ctx, s.pre, ""); err != nil { |
There was a problem hiding this comment.
Don't we need the secret in the pre-steps so the installer can commit the KUBECONFIG to it in the first place?
There was a problem hiding this comment.
The main reason for repurposing artifacts is because the installer image doesn't have oc in it. If we can enforce that pre steps must be able to modify a secret, I'll gladly remove this.
pkg/steps/new_e2e.go
Outdated
| ObjectMeta: meta.ObjectMeta{Name: "installer"}, | ||
| Data: make(map[string][]byte, len(files)), | ||
| } | ||
| for _, f := range files { |
There was a problem hiding this comment.
This seems fragile -- if the secret is write-able (or if we use a configmap) the pre-steps can just self-manage this
pkg/steps/new_e2e.go
Outdated
| return nil | ||
| } | ||
|
|
||
| type openshiftInstallerTestStep struct { |
There was a problem hiding this comment.
I don't understand what is going on in this commit
There was a problem hiding this comment.
First of all, in the best scenario it is intended to be removed. Up to this point, the implementation is completely generic and has no E2E- (or even Openshift-) specific bits.
The code in openshiftInstallerTestStep is everything I still don't know how to make generic that is required for the E2E AWS test.. It is also a possible implementation where different test types would share the step back-end but have test-specific extensions, but I would prefer if we could make all that part of configuration / step implementation.
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
added
size/L
3 participants
First prototype of the new test type. This version is capable of reproducing the most common template test (AWS E2E). See the corresponding WIP branch here.
Some parts of the code are still specific to E2E tests, but I've isolated them in the last commit and am open to suggestions on how to make them more generic.
Other than that, next steps are (sup)porting more test types, adding more tests (which we can finally have), and making sure we have 100% feature parity with templates.