Address reviewer comments

Author: pedjak

test/e2e/features/install.feature

@@ -8,7 +8,7 @@ Feature: Install ClusterExtension
     And "test" catalog serves bundles
     And Service account "olm-sa" with needed permissions is available in test namespace
 
-  Scenario Outline: Install latest available version from the default channel
+  Scenario Outline: Install latest available version
     When ClusterExtension is applied
       """
       apiVersion: olm.operatorframework.io/v1

test/e2e/features_test.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	//"context"
 	"fmt"
 	"log"
 	"os"
@@ -10,9 +9,6 @@ import (
 	"github.com/cucumber/godog"
 	"github.com/cucumber/godog/colors"
 	"github.com/spf13/pflag"
-	ctrl "sigs.k8s.io/controller-runtime"
-	//ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	utils "github.com/operator-framework/operator-controller/internal/shared/util/testutils"
 	"github.com/operator-framework/operator-controller/test/e2e/steps"
@@ -25,11 +21,7 @@ var opts = godog.Options{
 	Concurrency: 1,
 }
 
-var logOpts = zap.Options{}
-
 func init() {
-	flagSet := pflag.CommandLine
-	flagSet.BoolVar(&logOpts.Development, "log.debug", false, "print debug log level")
 	godog.BindCommandLineFlags("godog.", &opts)
 }
 
@@ -38,9 +30,6 @@ func TestMain(m *testing.M) {
 	pflag.Parse()
 	opts.Paths = pflag.Args()
 
-	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&logOpts)))
-
-	//opts.DefaultContext = ctrl.LoggerInto(context.Background(), ctrllog.Log)
 	// run tests
 	sc := godog.TestSuite{
 		TestSuiteInitializer: InitializeSuite,
@@ -53,7 +42,7 @@ func TestMain(m *testing.M) {
 
 		path := os.Getenv("E2E_SUMMARY_OUTPUT")
 		if path == "" {
-			fmt.Printf("Note: E2E_SUMMARY_OUTPUT is unset; skipping summary generation\n")
+			fmt.Println("Note: E2E_SUMMARY_OUTPUT is unset; skipping summary generation")
 		} else {
 			if err := utils.PrintSummary(path); err != nil {
 				// Fail the run if alerts are found
@@ -72,7 +61,7 @@ func TestMain(m *testing.M) {
 }
 
 func InitializeSuite(tc *godog.TestSuiteContext) {
-	tc.BeforeSuite(steps.DetectEnabledFeatureGates)
+	tc.BeforeSuite(steps.BeforeSuite)
 }
 
 func InitializeScenario(sc *godog.ScenarioContext) {

test/e2e/steps/hooks.go

@@ -10,8 +10,11 @@ import (
 	"strconv"
 
 	"github.com/cucumber/godog"
+	"github.com/go-logr/logr"
+	"github.com/spf13/pflag"
 	"k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
 type resource struct {
@@ -35,8 +38,17 @@ const (
 	scenarioContextKey contextKey = "scenario-context"
 )
 
-var featureGates = map[string]bool{
-	"WebhookProviderCertManager": true,
+var (
+	logOpts      = zap.Options{}
+	featureGates = map[string]bool{
+		"WebhookProviderCertManager": true,
+	}
+	logger logr.Logger
+)
+
+func init() {
+	flagSet := pflag.CommandLine
+	flagSet.BoolVar(&logOpts.Development, "log.debug", false, "print debug log level")
 }
 
 func RegisterHooks(sc *godog.ScenarioContext) {
@@ -46,7 +58,9 @@ func RegisterHooks(sc *godog.ScenarioContext) {
 	sc.After(ScenarioCleanup)
 }
 
-func DetectEnabledFeatureGates() {
+func BeforeSuite() {
+	logger = zap.New(zap.UseFlagOptions(&logOpts))
+
 	raw, err := kubectl("get", "deployment", "-n", olmNamespace, olmDeploymentName, "-o", "json")
 	if err != nil {
 		return
@@ -95,9 +109,10 @@ func scenarioCtx(ctx context.Context) *scenarioContext {
 
 func ScenarioCleanup(ctx context.Context, _ *godog.Scenario, err error) (context.Context, error) {
 	sc := scenarioCtx(ctx)
-	for _, p := range sc.backGroundCmds {
-		p.Process.Kill() // nolint: errcheck // we don't care about the error here, we just want to kill the process
-		p.Process.Wait() // nolint: errcheck // same as above, we just want to wait for the process to exit, and do not want to fail the test if it does not
+	for _, bgCmd := range sc.backGroundCmds {
+		if p := bgCmd.Process; p != nil {
+			_ = p.Kill()
+		}
 	}
 	if err != nil {
 		return ctx, err

test/e2e/steps/steps.go

@@ -22,11 +22,11 @@ import (
 	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/prometheus/common/expfmt"
 	"github.com/prometheus/common/model"
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/utils/ptr"
-	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/yaml"
 )
 
@@ -37,7 +37,10 @@ const (
 	tick              = 1 * time.Second
 )
 
-var kubeconfigPath string
+var (
+	kubeconfigPath string
+	k8sCli         string
+)
 
 func RegisterSteps(sc *godog.ScenarioContext) {
 	sc.Step(`^OLM is available$`, OLMisAvailable)
@@ -68,23 +71,29 @@ func RegisterSteps(sc *godog.ScenarioContext) {
 }
 
 func init() {
-	kubeconfigPath = os.Getenv("HOME") + "/.kube/config"
+	flagSet := pflag.CommandLine
+	flagSet.StringVar(&k8sCli, "k8s.cli", "kubectl", "Path to k8s cli")
+	if v, found := os.LookupEnv("KUBECONFIG"); found {
+		kubeconfigPath = v
+	} else {
+		home, err := os.UserHomeDir()
+		if err != nil {
+			panic(fmt.Sprintf("cannot determine user home directory: %v", err))
+		}
+		flagSet.StringVar(&kubeconfigPath, "kubeconfig", filepath.Join(home, ".kube", "config"), "Paths to a kubeconfig. Only required if out-of-cluster.")
+	}
 }
 
-var (
-	logger = log.Log
-)
-
 func kubectl(args ...string) (string, error) {
-	cmd := exec.Command("kubectl", args...)
-	logger.V(1).Info(strings.Join(cmd.Args, " "))
+	cmd := exec.Command(k8sCli, args...)
+	logger.V(1).Info("Running", "command", strings.Join(cmd.Args, " "))
 	cmd.Env = append(os.Environ(), fmt.Sprintf("KUBECONFIG=%s", kubeconfigPath))
 	b, err := cmd.Output()
 	return string(b), err
 }
 
 func kubectlWithInput(yaml string, args ...string) (string, error) {
-	cmd := exec.Command("kubectl", args...)
+	cmd := exec.Command(k8sCli, args...)
 	cmd.Stdin = bytes.NewBufferString(yaml)
 	cmd.Env = append(os.Environ(), fmt.Sprintf("KUBECONFIG=%s", kubeconfigPath))
 	b, err := cmd.Output()
@@ -143,15 +152,13 @@ func ResourceApplyFails(ctx context.Context, errMsg string, yamlTemplate *godog.
 		_, err := kubectlWithInput(yamlContent, "apply", "-f", "-")
 		if err == nil {
 			return false
-			//return fmt.Errorf("expected apply to fail, got: %s", out)
 		}
 		if stdErr := string(func() *exec.ExitError {
 			target := &exec.ExitError{}
 			_ = errors.As(err, &target)
 			return target
 		}().Stderr); !strings.Contains(stdErr, errMsg) {
 			return false
-			//return fmt.Errorf("expected error message %s to be in stderr, got: %s", errMsg, stdErr)
 		}
 		return true
 	})
@@ -436,6 +443,7 @@ func SendMetricsRequest(ctx context.Context, serviceAccount string, endpoint str
 			return false
 		}
 		defer resp.Body.Close()
+
 		if resp.StatusCode == http.StatusOK {
 			b, err := io.ReadAll(resp.Body)
 			if err != nil {