test(e2e): wait for deployment to exist in csv replacement test#833
Conversation
openshift-ci-robot
added
approved
| require.NoError(t, err) | ||
|
|
||
| // Wait for new deployment to exist | ||
| err = waitForDeployment(t, c, strategyNew.DeploymentSpecs[0].Name) |
There was a problem hiding this comment.
Looks like you're missing require.NoError(t, err) below it.
|
/test unit |
|
@soltysh: changing LGTM is restricted to assignees, and only operator-framework/operator-lifecycle-manager repo collaborators may be assigned issues. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
openshift-ci-robot
added
size/M
|
/test unit |
|
/retest deleting namespaces after a test seems to be hanging :/ |
| logger.WithError(err).Debug("error removing from queue") | ||
| } | ||
|
|
||
| if clusterServiceVersion.IsCopied() { |
There was a problem hiding this comment.
This looks good. One structural thing is that we could close over a var copied *bool initialized to false, set it below when we check clusterServiceVersion.IsCopied(), and use that here to avoid checking again (not that it's very resource intensive).
| defer func(csv v1alpha1.ClusterServiceVersion) { | ||
| logger.Debug("removing csv from queue set") | ||
| a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace()) | ||
| if err := a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace()); err != nil { |
There was a problem hiding this comment.
Nice! It seems like it was missing from the original, but aren't CSV keys in other queues as well (e.g. csvCopyQueue and csvGCQueue)? Should we handle removing those as well? Could we wrap the delete handlers in our queue abstractions to have them automatically dropped after they return?
There was a problem hiding this comment.
These are good ideas for a follow up :)
I was going to add it here but we don't even expose the other queues outside of their indexers, so that would be a bigger change
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, njhale, soltysh 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 |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
5 participants
Fixes a flake I noticed on #830
Might not be the only one, so I'll append more fixes if needed