cluster claim: use watch API #1874
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
Part of DPTP-2099 |
openshift-ci
bot
added
needs-rebase
pkg/steps/artifacts_test.go
Outdated
| return true, nil | ||
| }, | ||
| ); err != nil { | ||
| t.Errorf("unexpected error occurred in objectFunc: %v", err) |
There was a problem hiding this comment.
You can't call this from a goroutine - not only will it not kill the test, but if you end up calling this after the test is done, it will panic
pkg/steps/artifacts_test.go
Outdated
| func() (bool, error) { | ||
| ctx := context.TODO() | ||
| pod := &coreapi.Pod{} | ||
| if err := client.Get(ctx, ctrlruntimeclient.ObjectKey{Name: podName, Namespace: ns}, pod); err != nil { |
There was a problem hiding this comment.
What creates this Pod?
There was a problem hiding this comment.
pkg/steps/cluster_claim.go
Outdated
| claimNamespace := clusterPool.Namespace | ||
| claim := &hivev1.ClusterClaim{ | ||
| ret := &hivev1.ClusterClaim{ |
There was a problem hiding this comment.
ret is a bad name as it's not descriptive. Why not claim?
There was a problem hiding this comment.
We keep the created one to return because it will be deleted no matter what happens in the acquire function.
Renamed the var into which is passed to the acquire function
pkg/steps/cluster_claim.go
Outdated
| } | ||
| ret = claim | ||
| logrus.Info("The claimed cluster is ready.") | ||
| clusterDeploymentName := claim.Spec.Namespace |
There was a problem hiding this comment.
What's the utility in this local variable?
There was a problem hiding this comment.
For improve the readability of the code.
Removed.
pkg/steps/cluster_claim.go
Outdated
| logrus.WithField("clusterClaim.Namespace", clusterClaim.Namespace).WithField("clusterClaim.Name", clusterClaim.Name).Debug("Deleting cluster claim.") | ||
| if err := hiveClient.Delete(ctx, clusterClaim); err != nil { | ||
| if err := s.hiveClient.Delete(ctx, clusterClaim); err != nil { |
There was a problem hiding this comment.
Likely worth it to retry a couple times on this?
There was a problem hiding this comment.
Add retry.
Why it could fail in the first place? Do we do the retry on other object deleting?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, stevekuznetsov 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 |
/cc @stevekuznetsov