test(e2e): patch installplans w/ server-side apply #1617
Conversation
openshift-ci-robot
added
the
approved
|
/hold Looks like the patch is still running into issues with |
openshift-ci-robot
added
the
do-not-merge/hold
test/e2e/subscription_e2e_test.go
Outdated
|
|
||
| // Set the InstallPlan's approval mode to Manual | ||
| plan.Spec.Approval = v1alpha1.ApprovalManual | ||
| plan.Spec.Approved = false | ||
| plan, err = crc.OperatorsV1alpha1().InstallPlans(ref.Namespace).Update(context.TODO(), plan, metav1.UpdateOptions{}) | ||
| require.NoError(GinkgoT(), err) | ||
| setTypeMeta := func(p *v1alpha1.InstallPlan) *v1alpha1.InstallPlan { |
There was a problem hiding this comment.
if this is in an eventually, it should work?
|
This is really interesting. Does it make any sense to consider turning this logic into a library such that more of the e2e tests can start to use server-side apply? |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
njhale
force-pushed
the
fix-e2e
branch
2 times, most recently
from
ea1cc4f to
18b5531
Compare
@exdx The function is included with our e2e tests here, so it's already available to use in any of them. |
|
/test e2e-gcp |
| return err | ||
| } | ||
| cp.GetObjectKind().SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind()) | ||
| cp.SetManagedFields(nil) |
There was a problem hiding this comment.
is this needed? shouldn't the ForceOwnership make test become the owner of any changed fields?
There was a problem hiding this comment.
this came out of some red/green/refactor-ing -- an error is returned without it.
Use server-side apply to patch InstallPlans under test in order to prevent update conflict flakes.
|
/retest |
|
|
||
| // Apply returns a function that invokes a change func on an object and performs a server-side apply patch with the result and its status subresource. | ||
| // The given resource must only specify its Name, Namespace, APIVersion, and Kind. | ||
| // The given change function must be aunary, matching the signature: "func(<obj type>) error". |
There was a problem hiding this comment.
| // The given change function must be aunary, matching the signature: "func(<obj type>) error". | |
| // The given change function must be unary, matching the signature: "func(<obj type>) error". |
| } | ||
| var err error | ||
| if rt := changeType.Out(0); !rt.Implements(reflect.TypeOf((*error)(nil)).Elem()) { | ||
| panic(fmt.Sprintf("unexpected return type in change function signature: expected %t, preset %s", err, rt)) |
There was a problem hiding this comment.
| panic(fmt.Sprintf("unexpected return type in change function signature: expected %t, preset %s", err, rt)) | |
| panic(fmt.Sprintf("unexpected return type in change function signature: expected %t, present %s", err, rt)) |
| return err | ||
| } | ||
|
|
||
| reflect.ValueOf(obj).Elem().Set(cpv.Elem()) |
There was a problem hiding this comment.
Does the doc comment make it clear that the value addressed byobj will be updated?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Use server-side apply to patch InstallPlans under test in order to prevent
update conflict flakes.