test(e2e): trim the teardown-skip comments

The explanations were longer than the code they described. The reasoning lives
in #6777 and in the PR.

Refs: external-secrets/external-secrets#6777
Signed-off-by: Alexander Chernov <alexander@chernov.it>

.github/workflows/e2e-reusable.yml

@@ -231,12 +231,9 @@ jobs:
       # both into the e2e pod; entrypoint.sh runs ginkgo with them.
       TEST_SUITES: ${{ matrix.suite }}
       GINKGO_LABELS: ${{ matrix.labels }}
-      # Each leg gets a fresh kind cluster and throws it away with the runner, so
-      # uninstalling the global addons on the way out costs about a minute and
-      # buys nothing. Safe here specifically because TEST_SUITES above is a
-      # single suite: the suites share a cluster, and two of them install an
-      # "eso" release with different values, so the helper ignores this whenever
-      # more than one suite is scheduled.
+      # The kind cluster goes away with the runner, so uninstalling the global
+      # addons costs about a minute and buys nothing. Safe because TEST_SUITES
+      # above is a single suite; the helper refuses this for several.
       E2E_SKIP_GLOBAL_TEARDOWN: "true"
     steps:
       - uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0

docs/contributing/process.md

@@ -91,11 +91,9 @@ run managed tests.
 make test.e2e GINKGO_LABELS='gcp&&!managed'
 ```
 
-Setting `E2E_SKIP_GLOBAL_TEARDOWN=true` leaves the global addons (the ESO release
-itself) installed when the suite finishes, which saves about a minute. Only do
-that against a throwaway cluster you are about to delete: it is exported like the
-other variables above, so it applies to whatever your kube context points at.
-CI sets it on the kind legs, and `make test.managed` clears it.
+`E2E_SKIP_GLOBAL_TEARDOWN=true` leaves the ESO release installed when the suite
+finishes, saving about a minute. Only for a throwaway cluster: it applies to
+whatever your kube context points at. `make test.managed` clears it.
 
 #### Managed Kubernetes e2e tests
 

e2e/Makefile

@@ -8,9 +8,8 @@ DOCKER_BUILD_ARGS     ?=
 export E2E_IMAGE_NAME ?= ghcr.io/external-secrets/external-secrets-e2e
 export GINKGO_LABELS ?= !managed
 export TEST_SUITES ?= provider generator flux argocd
-# Leave the global addons installed on the way out. Only sensible for a cluster
-# you are about to delete; ignored when TEST_SUITES names more than one suite,
-# since the suites share a cluster. Off by default, CI sets it per leg.
+# Leave the global addons installed on the way out. Only for a cluster you are
+# about to delete. Off by default; CI sets it per leg.
 export E2E_SKIP_GLOBAL_TEARDOWN ?=
 
 export OCI_IMAGE_NAME = ghcr.io/external-secrets/external-secrets
@@ -50,11 +49,8 @@ test.run: ## Load prebuilt image tarballs into kind and run the e2e suite
 	kind load image-archive --name="external-secrets" $(E2E_ARTIFACT_DIR)/e2e.tar
 	./run.sh
 
-# This target runs against whatever the current kube context points at, which is
-# not ours to leave dirty. Makefile exports are file scoped, so without this an
-# exported E2E_SKIP_GLOBAL_TEARDOWN would reach the recipe and leave ESO
-# installed on a real cluster. override also beats a command-line assignment,
-# which a plain target-specific value would not.
+# Runs against the current kube context, which is not ours to leave dirty.
+# override beats both an inherited and a command-line value.
 test.managed: override E2E_SKIP_GLOBAL_TEARDOWN =
 test.managed: e2e-image ## Run e2e tests against current kube context
 	$(MAKE) -C ../ docker.build \

e2e/README.md

@@ -158,18 +158,10 @@ make -C e2e matrix.plan
 # run a single provider locally (overrides the Makefile defaults)
 make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed"
 
-# skip uninstalling the global addons on the way out, for a cluster you are
-# about to delete anyway. Saves about a minute per run on the provider,
-# generator and argocd suites; every kind leg in e2e-reusable.yml sets it, and
-# e2e-managed.yml deliberately does not.
-#
-# Refused, with a line on stderr, when TEST_SUITES names more than one suite:
-# entrypoint.sh runs those against one cluster and two of them install an "eso"
-# release with different values. That guard only sees its own process, so two
-# separate single-suite runs against one cluster would still collide.
-#
-# Do not use it against a cluster you did not create. make test.managed clears
-# it for that reason.
+# leave the global addons installed, for a cluster you are about to delete.
+# Saves about a minute; the kind legs set it, e2e-managed.yml does not.
+# Refused (stderr) when TEST_SUITES names several suites, and that guard sees
+# only its own process, so two single-suite runs on one cluster still collide.
 make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed" \
   E2E_SKIP_GLOBAL_TEARDOWN=true
 ```

e2e/framework/addon/addon.go

@@ -81,28 +81,11 @@ func UninstallGlobalAddons() {
 	}
 }
 
-// skipGlobalTeardownVar opts a run out of uninstalling the global addons.
-// e2e/run.sh forwards it into the e2e pod; CI sets it per leg.
 const skipGlobalTeardownVar = "E2E_SKIP_GLOBAL_TEARDOWN"
 
-// SkipGlobalTeardown reports whether a suite should leave the global addons
-// installed instead of uninstalling them on the way out.
-//
-// CI runs every leg against a kind cluster that is discarded immediately
-// afterwards, so uninstalling the ESO release costs about a minute per leg and
-// buys nothing. It is also the only step that can fail a leg whose specs all
-// passed: the release owns the CRDs, so `helm uninstall --wait` waits for CRD
-// deletion, which blocks on finalizers that the controller being removed by the
-// same uninstall is no longer around to release.
-//
-// Off unless asked for, so a run against a cluster it does not own (notably
-// `make test.managed`) keeps cleaning up exactly as before.
-//
-// The TEST_SUITES check is not optional. entrypoint.sh runs each suite binary in
-// turn against one cluster, and the provider and generator suites both install
-// an "eso" release with different values, so a suite that left its release
-// behind would break the next suite's install. A multi-suite run therefore tears
-// down as normal and logs why the request was refused.
+// SkipGlobalTeardown reports whether to leave the global addons installed, for a
+// cluster that is about to be discarded. Off unless asked for, and refused when
+// several suites share the cluster, since two of them install the same release.
 func SkipGlobalTeardown() bool {
 	raw, ok := os.LookupEnv(skipGlobalTeardownVar)
 	if !ok || raw == "" {
@@ -110,9 +93,8 @@ func SkipGlobalTeardown() bool {
 	}
 	skip, err := strconv.ParseBool(raw)
 	if err != nil {
-		// Fall back to tearing down, which is the safe answer, and say so
-		// loudly. Failing here instead would unwind the whole AfterSuite, so a
-		// typo would leave the cluster with neither a teardown nor its logs.
+		// Failing here would unwind the whole AfterSuite, losing the teardown
+		// and the logs, so fall back to tearing down and say so.
 		teardownLogf("%s is not a boolean (%q), so the teardown will run: %v",
 			skipGlobalTeardownVar, raw, err)
 		return false
@@ -120,9 +102,8 @@ func SkipGlobalTeardown() bool {
 	if !skip {
 		return false
 	}
-	// Only covers suites sharing one process's cluster, which is what
-	// entrypoint.sh does. Two separate single-suite runs pointed at the same
-	// cluster are indistinguishable from here and would still collide.
+	// Only sees this process. Two separate single-suite runs against one cluster
+	// would still collide.
 	if suites := strings.Fields(os.Getenv("TEST_SUITES")); len(suites) > 1 {
 		teardownLogf("%s ignored: suites %q share one cluster, so the global "+
 			"addons have to come out between them", skipGlobalTeardownVar,
@@ -134,12 +115,8 @@ func SkipGlobalTeardown() bool {
 	return true
 }
 
-// teardownLogf reports a teardown decision on stderr.
-//
-// Not log.Logf: that writes to GinkgoWriter, and ginkgo drops a passing node's
-// writer output unless the suite runs with -v, which CI does not. These lines
-// have to survive a green run, since they are the only evidence of whether the
-// skip took effect.
+// teardownLogf logs to stderr, not log.Logf: ginkgo drops GinkgoWriter output
+// for a passing node without -v, and these lines must survive a green run.
 func teardownLogf(format string, args ...any) {
 	fmt.Fprintf(os.Stderr, format+"\n", args...)
 }

e2e/suites/argocd/suite_test.go

@@ -41,8 +41,7 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	// The pre-deletion below exists only so the uninstall that follows it can
-	// complete, so it is skipped together with it.
+	// The pre-deletion serves only the uninstall, so it is skipped with it.
 	if !addon.SkipGlobalTeardown() {
 		_, _, cl := util.NewConfig()
 		By("Deleting any pending generator states")

e2e/suites/flux/suite_test.go

@@ -41,13 +41,9 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	// uninstallFlux is gated together with the global addons, not left running.
-	// FluxHelmRelease.Uninstall deletes the HelmRelease and waits for its
-	// finalizers.fluxcd.io finalizer to clear; without that, uninstallFlux's
-	// `kubectl delete -f install.yaml` removes the flux-system namespace while
-	// helm-controller is still needed to release that finalizer, and kubectl
-	// waits on it with a 168h default. That hangs the suite until ginkgo's
-	// timeout, on a leg whose specs all passed.
+	// uninstallFlux is gated too: on its own it deletes the flux-system namespace
+	// while helm-controller is still needed to clear the HelmRelease finalizer
+	// that UninstallGlobalAddons clears first, and kubectl waits 168h on that.
 	if !addon.SkipGlobalTeardown() {
 		cfg := &addon.Config{}
 		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()

e2e/suites/generator/suite_test.go

@@ -44,8 +44,7 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	// The pre-deletion below exists only so the uninstall that follows it can
-	// complete, so it is skipped together with it.
+	// The pre-deletion serves only the uninstall, so it is skipped with it.
 	if !addon.SkipGlobalTeardown() {
 		cfg := &addon.Config{}
 		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()

e2e/suites/provider/suite_test.go

@@ -44,8 +44,7 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 var _ = SynchronizedAfterSuite(func() {
 	// noop
 }, func() {
-	// The pre-deletions below exist only so the uninstall that follows them can
-	// complete, so they are skipped together with it.
+	// The pre-deletions serve only the uninstall, so they are skipped with it.
 	if !addon.SkipGlobalTeardown() {
 		cfg := &addon.Config{}
 		cfg.KubeConfig, cfg.KubeClientSet, cfg.CRClient = util.NewConfig()