-
Notifications
You must be signed in to change notification settings - Fork 107
CLOUDP-82115: Implement delete retry loop #149
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
Conversation
antonlisovenko
left a comment
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.
Just one suggestion to deeper check the error on removal.
Thanks for finding the bug with Spec.Name!!
|
|
||
| for time.Now().Before(timeout) { | ||
| _, err = atlasClient.Clusters.Delete(context.Background(), project.Status.ID, cluster.Spec.Name) | ||
| if err != nil { |
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.
what do you think if we check here the error that is returned in case the cluster is already removed - and break the loop in this case?
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.
Good catch!
|
|
||
| for time.Now().Before(timeout) { | ||
| _, err = atlasClient.Projects.Delete(context.Background(), project.Status.ID) | ||
| if err != nil { |
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.
the same as above
| By("Removing Atlas Cluster " + createdCluster.Name) | ||
| Expect(k8sClient.Delete(context.Background(), createdCluster)).To(Succeed()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdCluster.Name), 600, interval).Should(BeTrue()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdCluster.Spec.Name), 600, interval).Should(BeTrue()) |
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.
🥇
| By("Removing Atlas Project " + createdProject.Status.ID) | ||
| Eventually(removeAtlasProject(createdProject.Status.ID), 600, interval).Should(BeTrue()) | ||
| Expect(k8sClient.Delete(context.Background(), createdProject)).To(Succeed()) | ||
| Eventually(checkAtlasProjectRemoved(createdProject.Status.ID), 60, interval).Should(BeTrue()) |
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.
[q] do you know if we still need Eventually here? If there are no clusters then the project is supposed to get removed immediately or am I missing something?
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.
Some time could pass between a call to k8sClient.Delete and the actual API call. Also, we could always stumble upon a network issue and fail prematurely
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.
ah, right, we remove the resource in K8s, not in Atlas
| for time.Now().Before(timeout) { | ||
| _, err = atlasClient.Projects.Delete(context.Background(), project.Status.ID) | ||
| var apiError *mongodbatlas.ErrorResponse | ||
| if errors.As(err, &apiError) && apiError.ErrorCode == atlas.ClusterNotFound { |
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.
should it be atlas.ProjectNotFound?
antonlisovenko
left a comment
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.
👍
No description provided.