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

Add acceptance test cleanup #3375

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Conversation

curtbushko
Copy link
Contributor

@curtbushko curtbushko commented Dec 13, 2023

Changes proposed in this PR

  • We often have acceptance tests that fail because resources are left behind. At this time I do not trust that helm delete is deleting everything it created.
  • This is a first pass attempt to nuke all the helm things for a release.

How I've tested this PR

  • Ran through acceptance tests (all green)

How I expect reviewers to test this PR

👀

Checklist

@curtbushko curtbushko force-pushed the curtbushko/delete-all-resources branch 2 times, most recently from 4ff0b61 to fa98d44 Compare December 15, 2023 17:20
@curtbushko curtbushko self-assigned this Dec 15, 2023
@curtbushko curtbushko added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. backport/1.3.x labels Dec 15, 2023
@curtbushko curtbushko marked this pull request as ready for review December 15, 2023 17:38
@wilkermichael
Copy link
Contributor

Just a heads up that we're entering a code-freeze today at some point (for the backports) so maybe hold off on merging until you check with @t-eckert

Copy link
Contributor

@wilkermichael wilkermichael left a comment

Choose a reason for hiding this comment

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

Awesome job!

I am hopeful that this will help with some of those "resources still exist" errors 🤞

@@ -222,6 +223,66 @@ func (h *HelmCluster) Destroy(t *testing.T) {
}
}

// Delete any deployments that have h.releaseName in their name.
deployments, err := h.kubernetesClient.AppsV1().Deployments(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Just a thought but might be worth having a function or something that we call that just generates "release="+h.releaseName since it's used everywhere. Less chance of accidentally mispelling/creating wrong string in the future.

@@ -286,6 +347,51 @@ func (h *HelmCluster) Destroy(t *testing.T) {
}
}

// Verify that all deployments have been deleted.
deployments, err = h.kubernetesClient.AppsV1().Deployments(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any steps we can take if these resources still exist?

require.NoError(r, err)
for _, deployment := range deployments.Items {
if strings.Contains(deployment.Name, h.releaseName) {
r.Errorf("Found deployment which should have been deleted: %s", deployment.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to verify resource deletion... this will hopefully help us diagnose which resource is causing things not to be deleted properly in the future.

@curtbushko
Copy link
Contributor Author

I will wait until after code freeze to merge this just so that we do not cause any problems.

@t-eckert
Copy link
Contributor

I will wait until after code freeze to merge this just so that we do not cause any problems.

Ack.

Add acceptance test cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. backport/1.3.x pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants