Skip to content
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

Owned DNS Records will block Managed Zone deletion #162

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

philbrookes
Copy link
Collaborator

No description provided.

@philbrookes philbrookes changed the title [WIP] DNS Records block MZ deletion DNS Records block MZ deletion Jun 25, 2024
@philbrookes philbrookes changed the title DNS Records block MZ deletion Owned DNS Records will block Managed Zone deletion Jun 25, 2024
Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline I'm not really sure if there is a lot of point adding this change considering we can't prevent the secrets being removed, but mostly because we want to remove the ManagedZone API anyway. That said, it looks fine, so can be merged if desired.

} else if recordsExist {
logger.Info("ManagedZone deletion awaiting removal of owned DNS records")
return ctrl.Result{Requeue: true}, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this re-queue? Do the owner records not cause this to reconcile on change anyway?

Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
}
if brokenZone != nil {
err := k8sClient.Delete(ctx, brokenZone)
err := k8sClient.Delete(ctx, brokenZone, client.PropagationPolicy(metav1.DeletePropagationForeground))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure these make any difference unless you set the owner ref up with block owner deletion https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#ownership-and-finalizers

@philbrookes philbrookes added this pull request to the merge queue Jul 8, 2024
Merged via the queue into Kuadrant:main with commit 87c1514 Jul 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants