-
Notifications
You must be signed in to change notification settings - Fork 831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix clean up of old virtual services #1618
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing 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 |
Thu Mar 26 18:38:44 UTC 2020 impatient try |
Thu Mar 26 18:38:49 UTC 2020 impatient try |
/test integration |
Thu Mar 26 20:16:09 UTC 2020 impatient try |
Fri Mar 27 11:03:41 UTC 2020 impatient try |
Fri Mar 27 11:03:42 UTC 2020 impatient try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! I've added a couple small comments, but they are mainly thoughts or nitpick-ish so feel free to ignore them @cliveseldon.
return ready, err | ||
} | ||
for _, vsvcDeleted := range deleted { | ||
r.Recorder.Eventf(instance, corev1.EventTypeNormal, constants.EventsDeleteVirtualService, "Delete VirtualService %q", vsvcDeleted.GetName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: on the event message, should we say Deleted
instead of Delete
?
Name: namespaceName, | ||
}, | ||
} | ||
Expect(k8sClient.Create(context.Background(), namespace)).Should(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably move most of this setup (e.g. creating the namespace, defining a SeldonDeployment
spec, setting default timeouts, etc.) into a BeforeEach
block shared by all the tests in operator/controllers/seldondeployment_controller_test.go
.
This is just a thought though and it probably falls outside the scope of this PR, so feel free to ignore.
/test integration |
Fri Mar 27 13:29:46 UTC 2020 impatient try |
/test integration |
Sat Mar 28 14:30:37 UTC 2020 impatient try |
Sat Mar 28 14:30:40 UTC 2020 impatient try |
Sat Mar 28 14:32:50 UTC 2020 impatient try |
Sat Mar 28 18:31:34 UTC 2020 impatient try |
Sat Mar 28 18:31:34 UTC 2020 impatient try |
/test notebook |
/test notebooks |
Sat Mar 28 19:04:24 UTC 2020 impatient try |
Sat Mar 28 19:04:25 UTC 2020 impatient try |
Sat Mar 28 19:05:11 UTC 2020 impatient try |
Sun Mar 29 08:16:43 UTC 2020 impatient try |
Sun Mar 29 08:16:48 UTC 2020 impatient try |
/test notebooks |
Sun Mar 29 08:23:36 UTC 2020 impatient try |
Sun Mar 29 10:42:14 UTC 2020 impatient try |
Sun Mar 29 10:42:32 UTC 2020 impatient try |
/test notebooks |
Sun Mar 29 10:50:34 UTC 2020 impatient try |
@cliveseldon: The following test failed, say
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. I understand the commands that are listed here. |
/test integration |
Sun Mar 29 14:02:29 UTC 2020 impatient try |
Fixes #1594