From 1b48dc744c8babb98c4693cc9f1b46de91c653c2 Mon Sep 17 00:00:00 2001
From: Marcin Owsiany <porridge@redhat.com>
Date: Tue, 14 Apr 2026 10:25:46 +0200
Subject: [PATCH 1/3] Remaining review comments.

---
 cmd/deploy.go                               | 10 ++++------
 cmd/subshell.go                             |  3 ++-
 internal/clusterdefaults/clusterdefaults.go |  4 ++++
 internal/deployer/deploy_via_helm.go        |  2 ++
 internal/deployer/deploy_via_operator.go    |  8 ++++++--
 internal/deployer/deployer.go               | 10 +++++++---
 internal/deployer/kubectl.go                |  2 +-
 internal/deployer/operator.go               |  2 ++
 internal/deployer/operator_olm.go           | 12 ++++++------
 internal/deployer/override.go               |  2 +-
 internal/env/env.go                         |  4 ++++
 internal/helpers/tag.go                     |  2 +-
 internal/imagecache/imagecache.go           |  2 +-
 internal/portforward/portforward.go         |  2 +-
 tests/e2e/e2e_test.go                       |  4 ++--
 15 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/cmd/deploy.go b/cmd/deploy.go
index 7f3aee80..1b16b0d6 100644
--- a/cmd/deploy.go
+++ b/cmd/deploy.go
@@ -225,16 +225,14 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to set feature flags: %w", err)
 	}
 
-	// Resolve "auto" resources based on cluster type
-	resolvedResources := resources
-	if resources == "auto" {
-		resolvedResources = resolveAutoResources(env.GetCurrentClusterType(), log)
+	if resources == "auto" { // validate the user-supplied value earlier than here
+		resources = resolveAutoResources(env.GetCurrentClusterType(), log)
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
 	defer cancel()
 
-	if err := d.Deploy(ctx, components, resolvedResources, exposure); err != nil {
+	if err := d.Deploy(ctx, components, resources, exposure); err != nil {
 		return fmt.Errorf("deployment failed: %w", err)
 	}
 
@@ -256,7 +254,7 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 
 // resolveAutoResources determines the appropriate resource tier based on cluster type
 func resolveAutoResources(clusterType env.ClusterType, log *logger.Logger) string {
-	var resolvedResources string
+	var resolvedResources string // should probably be a first-class type, not a free-form string...
 
 	switch clusterType {
 	case env.LocalKind:
diff --git a/cmd/subshell.go b/cmd/subshell.go
index 4a9b5c1b..b48f4288 100644
--- a/cmd/subshell.go
+++ b/cmd/subshell.go
@@ -116,7 +116,7 @@ defaults
     timeout server  30s
 
 frontend http_front
-    bind *:8080
+    bind *:8080 // this should probably be configurable?
     default_backend https_back
 
 backend https_back
@@ -131,6 +131,7 @@ backend https_back
 	configFile.Close()
 
 	cmd := exec.Command("haproxy", "-f", configPath)
+	// What about stdin? We probably should prevent haproxy from reading from the terminal just in case...
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 
diff --git a/internal/clusterdefaults/clusterdefaults.go b/internal/clusterdefaults/clusterdefaults.go
index a2f5693c..07627e07 100644
--- a/internal/clusterdefaults/clusterdefaults.go
+++ b/internal/clusterdefaults/clusterdefaults.go
@@ -34,6 +34,10 @@ func (ct ClusterType) String() string {
 	}
 }
 
+// Maybe I'm missing something, but this manager/detector/applicator abstraction seems massively over-engineered
+// since the is only one concrete implementation. AFAICT this could all be just a single function that the deployer
+// calls with log, kubeconfig, resources, exposure and portForward.
+
 // DeploymentDefaults holds the recommended defaults for a cluster type
 type DeploymentDefaults struct {
 	Resources          string // Resource preset (e.g., "small", "default")
diff --git a/internal/deployer/deploy_via_helm.go b/internal/deployer/deploy_via_helm.go
index bc96c110..711a6605 100644
--- a/internal/deployer/deploy_via_helm.go
+++ b/internal/deployer/deploy_via_helm.go
@@ -447,6 +447,8 @@ func extractMainImageReferences(renderedYAML string) []string {
 	lines := strings.Split(renderedYAML, "\n")
 	for _, line := range lines {
 		// Look for lines like: image: "quay.io/stackrox-io/main:4.8.2"
+		// TODO: this assumes image names will always be quoted with `"` and there will be a single space between `:` and `"`, we should make this more robust
+		// It might be better to parse the yaml and walk the resulting map, returning string values of `image` keys...
 		if strings.Contains(line, `image: "`) {
 			parts := strings.SplitN(line, `image: "`, 2)
 			if len(parts) == 2 {
diff --git a/internal/deployer/deploy_via_operator.go b/internal/deployer/deploy_via_operator.go
index d6b6e5ac..3a2455db 100644
--- a/internal/deployer/deploy_via_operator.go
+++ b/internal/deployer/deploy_via_operator.go
@@ -178,7 +178,7 @@ func (d *Deployer) getDeployedOperatorImage(ctx context.Context) (string, error)
 	return image, nil
 }
 
-// prepareNamespace creates pull secrets in the namespace
+// prepareNamespace creates pull secrets in the namespace if needed
 func (d *Deployer) prepareNamespace(ctx context.Context, namespace string) error {
 	d.logger.Infof("Preparing namespace %s", namespace)
 
@@ -443,6 +443,7 @@ func (d *Deployer) waitForCentralReady(ctx context.Context, timeout int) error {
 			}
 		}
 
+		// nit: using `kubectl wait` (which in turn - I hope - uses a watch) instead of polling would allow us to not waste time here
 		time.Sleep(checkInterval)
 	}
 
@@ -525,7 +526,8 @@ func (d *Deployer) fetchCentralCACert(ctx context.Context) error {
 		return fmt.Errorf("failed to decode CA cert: %w", err)
 	}
 
-	d.roxCACertFile = "/tmp/roxie-ca-cert.pem"
+	d.roxCACertFile = "/tmp/roxie-ca-cert.pem" // possible symlink race vulnerability
+	// this is a REALLY weird way to write a temporary file in go
 	writeCmd := exec.CommandContext(ctx, "sh", "-c", fmt.Sprintf("cat > %s", d.roxCACertFile))
 	writeCmd.Stdin = bytes.NewReader(caCert)
 	if err := writeCmd.Run(); err != nil {
@@ -562,6 +564,7 @@ func (d *Deployer) configureCentralEndpoint(ctx context.Context, exposure string
 			return fmt.Errorf("failed to get LoadBalancer endpoint: %w", err)
 		}
 		// Remove https:// prefix if present (waitForLoadBalancer returns https://ip:443)
+		// This is silly, why add these affixes there, and then strip here?!
 		d.centralEndpoint = strings.TrimPrefix(endpoint, "https://")
 		d.centralEndpoint = strings.TrimSuffix(d.centralEndpoint, ":443")
 		d.centralEndpoint = d.centralEndpoint + ":443"
@@ -784,6 +787,7 @@ func (d *Deployer) waitForSecuredClusterReady(ctx context.Context, timeout int)
 			}
 
 			// collector seems to be crashing on some local cluster types/versions.
+			// skip the check only on local clusters, then?
 		}
 
 		if allReady {
diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go
index c6947423..dc2b7f3e 100644
--- a/internal/deployer/deployer.go
+++ b/internal/deployer/deployer.go
@@ -32,6 +32,9 @@ var (
 
 	pauseReconcileAnnotationKey = "stackrox.io/pause-reconcile"
 
+	// at some point this will get out of date
+	// if we filter by the app.../part-of label anyway, then maybe we should just delete all resource kinds present on cluster?
+	// also we should use the fully-qualified types
 	allInstallableCentralResourceKinds = []string{
 		"applications",
 		"clusterroles",
@@ -343,7 +346,7 @@ func (d *Deployer) SetCombinedOverrideFile(overrideFile string) error {
 func setOverrideSetExpressions(overrides map[string]interface{}, prefix string, overrideSetExpressions []string) ([]string, error) {
 	remainingSetExpressions := make([]string, 0)
 	for _, expr := range overrideSetExpressions {
-		parts := splitAtFirstEquals(expr)
+		parts := splitAtFirstEquals(expr) // would https://pkg.go.dev/strings#Cut work instead?
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("invalid override expression '%s': expected format 'key.path=value'", expr)
 		}
@@ -586,8 +589,8 @@ func (d *Deployer) Deploy(ctx context.Context, components component.Component, r
 	return nil
 }
 
-// prepareCredentials prepares and verifies Docker credentials early to fail fast.
-// The verified credentials are stored for later use.
+// prepareCredentials prepares and verifies Docker credentials early to allow failing fast.
+// The verified credentials are stored in the Deployer object for later use.
 func (d *Deployer) prepareCredentials() error {
 	d.logger.Dimf("Preparing and verifying Docker credentials...")
 
@@ -1251,6 +1254,7 @@ func (d *Deployer) checkPodProgressInNamespace(ctx context.Context, namespace st
 	}
 }
 
+// plenty of code in common with the central variant that should probably be extracted
 func (d *Deployer) PrintSecuredClusterDeploymentSummary() {
 	component := "Secured Cluster"
 	mainImageTag := d.mainImageTag
diff --git a/internal/deployer/kubectl.go b/internal/deployer/kubectl.go
index 0fd8aa23..7badbc3d 100644
--- a/internal/deployer/kubectl.go
+++ b/internal/deployer/kubectl.go
@@ -26,7 +26,7 @@ type KubectlResult struct {
 }
 
 // runKubectl executes a kubectl command with automatic retries on transient errors
-func (d *Deployer) runKubectl(ctx context.Context, opts KubectlOptions) (*KubectlResult, error) {
+func (d *Deployer) runKubectl(ctx context.Context, opts KubectlOptions) (*KubectlResult, error) { // perhaps this should not return a pointer so that the lack of nil checks elsewhere is robust?
 	if opts.MaxAttempts <= 0 {
 		opts.MaxAttempts = 3
 	}
diff --git a/internal/deployer/operator.go b/internal/deployer/operator.go
index 919209d8..bc487a9b 100644
--- a/internal/deployer/operator.go
+++ b/internal/deployer/operator.go
@@ -107,6 +107,8 @@ func (d *Deployer) identifyCRDFileNames(bundleDir string) ([]string, error) {
 			return nil
 		}
 
+		// The following detection logic does not seem particularly robust.
+		// We should probably parse the YAML and check api group and kind fields.
 		name := strings.ToLower(info.Name())
 		if strings.Contains(name, "customresourcedefinition") ||
 			strings.Contains(name, "platform.stackrox.io") ||
diff --git a/internal/deployer/operator_olm.go b/internal/deployer/operator_olm.go
index 62eda9c6..5c5de587 100644
--- a/internal/deployer/operator_olm.go
+++ b/internal/deployer/operator_olm.go
@@ -83,7 +83,7 @@ func (d *Deployer) checkOLMInstalled(ctx context.Context) error {
 
 	for _, crd := range requiredCRDs {
 		_, err := d.runKubectl(ctx, KubectlOptions{
-			Args: []string{"get", "crd", crd},
+			Args: []string{"get", "crd", crd}, // actually this is not the right way to check, since a CRD could be in a broken state; we should use the api-resources subcommand instead
 		})
 		if err != nil {
 			return fmt.Errorf("OLM not installed: CRD %s not found. Please install OLM first", crd)
@@ -124,7 +124,7 @@ func (d *Deployer) createCatalogSource(ctx context.Context, indexImage string) e
 		},
 	}
 
-	// Add security context config if supported (OCP 4.14+).
+	// Add security context config if supported.
 	if hasSecurityContextConfig {
 		spec := catalogSource["spec"].(map[string]interface{})
 		spec["grpcPodConfig"] = map[string]interface{}{
@@ -158,7 +158,7 @@ func (d *Deployer) catalogSourceSupportsSecurityContextConfig(ctx context.Contex
 		return false, err
 	}
 
-	return strings.Contains(result.Stdout, "securityContextConfig"), nil
+	return strings.Contains(result.Stdout, "securityContextConfig"), nil // this is overly optimistic and would incorrectly succeed if an api version that contains this had "serving: false"
 }
 
 // createOperatorGroup creates the OperatorGroup.
@@ -251,7 +251,7 @@ func (d *Deployer) waitForAndApproveInstallPlan(ctx context.Context) error {
 	}
 
 	if time.Since(start) >= timeout {
-		return errors.New("timeout waiting for InstallPlan to be created")
+		return errors.New("timeout waiting for InstallPlan to be created") // some more info on what was wrong would be useful: a dump of the subscription or at least its name so that the user can investigate
 	}
 
 	// Sanity check:Verify currentCSV matches expected version.
@@ -321,7 +321,7 @@ func (d *Deployer) waitForCSVSuccess(ctx context.Context) error {
 		time.Sleep(5 * time.Second)
 	}
 
-	return fmt.Errorf("timeout waiting for CSV to succeed")
+	return fmt.Errorf("timeout waiting for CSV to succeed") // same as above
 }
 
 // detectOperatorDeploymentMode detects how the operator is currently deployed.
@@ -344,7 +344,7 @@ func (d *Deployer) detectOperatorDeploymentMode(ctx context.Context) (bool, Oper
 		result, err := d.runKubectl(ctx, KubectlOptions{
 			Args: []string{"get", "deployment", operatorDeploymentName, "-n", operatorNamespace, "-o", "jsonpath={.metadata.labels}"},
 		})
-		if err == nil && strings.Contains(result.Stdout, "olm.owner") {
+		if err == nil && strings.Contains(result.Stdout, "olm.owner") { // This is not very robust. Better retrieve a specific label in the `get` command?
 			return true, OperatorModeOLM
 		}
 		// Deployment exists without OLM labels = non-OLM deployment.
diff --git a/internal/deployer/override.go b/internal/deployer/override.go
index f44890d3..bfd4746b 100644
--- a/internal/deployer/override.go
+++ b/internal/deployer/override.go
@@ -34,7 +34,7 @@ func setNestedValue(m map[string]interface{}, path string, value interface{}) er
 			// Check if existing value is a map
 			if existingMap, isMap := existing.(map[string]interface{}); isMap {
 				current = existingMap
-			} else {
+			} else { // shouldn't this be an error instead? I think it's more likely someone made a typo...
 				// Existing value is not a map, we need to replace it
 				newMap := make(map[string]interface{})
 				current[key] = newMap
diff --git a/internal/env/env.go b/internal/env/env.go
index 9d632cea..098c5188 100644
--- a/internal/env/env.go
+++ b/internal/env/env.go
@@ -328,6 +328,10 @@ func fetchAPIResources() ([]string, error) {
 }
 
 func IsInStackroxRepository() bool {
+	// This assumes that:
+	// - origin is the name of the upstream remote (not true if someone cloned their fork)
+	// - the git transport was used
+	// How about instead looking for "# StackRox Kubernetes Security Platform" in README?
 	cmd := exec.Command("git", "remote", "get-url", "origin")
 	outputBytes, err := cmd.Output()
 	if err != nil {
diff --git a/internal/helpers/tag.go b/internal/helpers/tag.go
index a0d1903b..7fee83ff 100644
--- a/internal/helpers/tag.go
+++ b/internal/helpers/tag.go
@@ -9,7 +9,7 @@ import (
 )
 
 const (
-	defaultMainImageTag = "4.9.2"
+	defaultMainImageTag = "4.9.2" // Is the plan to keep bumping this on new ACS releases?
 )
 
 func LookupMainImageTag(log *logger.Logger) (string, error) {
diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go
index e072389a..0fe86539 100644
--- a/internal/imagecache/imagecache.go
+++ b/internal/imagecache/imagecache.go
@@ -33,7 +33,7 @@ func New(log *logger.Logger, cacheFile string, maxEntries int) *ImageCache {
 		if err != nil {
 			home = "."
 		}
-		cacheFile = filepath.Join(home, ".roxie.image_cache")
+		cacheFile = filepath.Join(home, ".roxie.image_cache") // how about using something XDG-compliant like ~/.cache/roxie/images?
 	}
 
 	if maxEntries <= 0 {
diff --git a/internal/portforward/portforward.go b/internal/portforward/portforward.go
index 3c864a85..cb502f1d 100644
--- a/internal/portforward/portforward.go
+++ b/internal/portforward/portforward.go
@@ -106,7 +106,7 @@ func (m *Manager) Start(namespace, serviceName string, remotePort, preferredLoca
 			if err == nil {
 				syscall.Kill(-pgid, syscall.SIGTERM)
 			}
-			cmd.Wait()
+			cmd.Wait() // AFAICT this can get stuck forever if the process blocks SIGTERM...
 		}
 		return "", fmt.Errorf("port-forward did not become ready")
 	}
diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go
index 82f17608..23520acd 100644
--- a/tests/e2e/e2e_test.go
+++ b/tests/e2e/e2e_test.go
@@ -17,7 +17,7 @@ import (
 )
 
 const (
-	defaultMainImageTag = "4.8.2"
+	defaultMainImageTag = "4.8.2" // this is a bit old...
 	deployTimeout       = 30 * time.Minute
 	teardownTimeout     = 10 * time.Minute
 )
@@ -67,7 +67,7 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-func teardownAllDeployments() error {
+func teardownAllDeployments() error { // maybe put the helper functions in a separate file?
 	fmt.Println("=== Tearing down all deployments before running tests ===")
 
 	ctx, cancel := context.WithTimeout(context.Background(), teardownTimeout)

From 9fea9d58492470b0d783895104ad7c6f8cbbe83b Mon Sep 17 00:00:00 2001
From: Marcin Owsiany <porridge@redhat.com>
Date: Tue, 14 Apr 2026 10:34:25 +0200
Subject: [PATCH 2/3] changed to TODOs

---
 cmd/deploy.go                               |  6 ++++--
 cmd/subshell.go                             |  5 +++--
 internal/clusterdefaults/clusterdefaults.go |  7 ++++---
 internal/deployer/deploy_via_helm.go        |  6 ++++--
 internal/deployer/deploy_via_operator.go    | 12 +++++++-----
 internal/deployer/deployer.go               | 10 ++++++----
 internal/deployer/kubectl.go                |  4 +++-
 internal/deployer/operator.go               |  4 ++--
 internal/deployer/operator_olm.go           | 21 +++++++++++++++------
 internal/deployer/override.go               |  4 +++-
 internal/env/env.go                         |  2 +-
 internal/helpers/tag.go                     |  3 ++-
 internal/imagecache/imagecache.go           |  3 ++-
 internal/portforward/portforward.go         |  3 ++-
 tests/e2e/e2e_test.go                       |  6 ++++--
 15 files changed, 62 insertions(+), 34 deletions(-)

diff --git a/cmd/deploy.go b/cmd/deploy.go
index 1b16b0d6..efb7c501 100644
--- a/cmd/deploy.go
+++ b/cmd/deploy.go
@@ -225,7 +225,8 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to set feature flags: %w", err)
 	}
 
-	if resources == "auto" { // validate the user-supplied value earlier than here
+	// TODO(#91): validate the user-supplied value earlier than here
+	if resources == "auto" {
 		resources = resolveAutoResources(env.GetCurrentClusterType(), log)
 	}
 
@@ -254,7 +255,8 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 
 // resolveAutoResources determines the appropriate resource tier based on cluster type
 func resolveAutoResources(clusterType env.ClusterType, log *logger.Logger) string {
-	var resolvedResources string // should probably be a first-class type, not a free-form string...
+	// TODO(#91): should probably be a first-class type, not a free-form string...
+	var resolvedResources string
 
 	switch clusterType {
 	case env.LocalKind:
diff --git a/cmd/subshell.go b/cmd/subshell.go
index b48f4288..93199c5c 100644
--- a/cmd/subshell.go
+++ b/cmd/subshell.go
@@ -116,7 +116,7 @@ defaults
     timeout server  30s
 
 frontend http_front
-    bind *:8080 // this should probably be configurable?
+    bind *:8080  # TODO(#91): this should probably be configurable?
     default_backend https_back
 
 backend https_back
@@ -131,7 +131,8 @@ backend https_back
 	configFile.Close()
 
 	cmd := exec.Command("haproxy", "-f", configPath)
-	// What about stdin? We probably should prevent haproxy from reading from the terminal just in case...
+	// TODO(#91): What about stdin? We probably should prevent haproxy from reading from
+	// the terminal just in case...
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 
diff --git a/internal/clusterdefaults/clusterdefaults.go b/internal/clusterdefaults/clusterdefaults.go
index 07627e07..7d555bff 100644
--- a/internal/clusterdefaults/clusterdefaults.go
+++ b/internal/clusterdefaults/clusterdefaults.go
@@ -34,9 +34,10 @@ func (ct ClusterType) String() string {
 	}
 }
 
-// Maybe I'm missing something, but this manager/detector/applicator abstraction seems massively over-engineered
-// since the is only one concrete implementation. AFAICT this could all be just a single function that the deployer
-// calls with log, kubeconfig, resources, exposure and portForward.
+// TODO(#91): Maybe I'm missing something, but this manager/detector/applicator abstraction
+// seems massively over-engineered since the is only one concrete implementation. AFAICT this
+// could all be just a single function that the deployer calls with log, kubeconfig, resources,
+// exposure and portForward.
 
 // DeploymentDefaults holds the recommended defaults for a cluster type
 type DeploymentDefaults struct {
diff --git a/internal/deployer/deploy_via_helm.go b/internal/deployer/deploy_via_helm.go
index 711a6605..a410cdf2 100644
--- a/internal/deployer/deploy_via_helm.go
+++ b/internal/deployer/deploy_via_helm.go
@@ -447,8 +447,10 @@ func extractMainImageReferences(renderedYAML string) []string {
 	lines := strings.Split(renderedYAML, "\n")
 	for _, line := range lines {
 		// Look for lines like: image: "quay.io/stackrox-io/main:4.8.2"
-		// TODO: this assumes image names will always be quoted with `"` and there will be a single space between `:` and `"`, we should make this more robust
-		// It might be better to parse the yaml and walk the resulting map, returning string values of `image` keys...
+		// TODO(#91): this assumes image names will always be quoted with `"` and there will be
+		// a single space between `:` and `"`, we should make this more robust. It might be
+		// better to parse the yaml and walk the resulting map, returning string values of
+		// `image` keys...
 		if strings.Contains(line, `image: "`) {
 			parts := strings.SplitN(line, `image: "`, 2)
 			if len(parts) == 2 {
diff --git a/internal/deployer/deploy_via_operator.go b/internal/deployer/deploy_via_operator.go
index 3a2455db..2b73baa7 100644
--- a/internal/deployer/deploy_via_operator.go
+++ b/internal/deployer/deploy_via_operator.go
@@ -443,7 +443,8 @@ func (d *Deployer) waitForCentralReady(ctx context.Context, timeout int) error {
 			}
 		}
 
-		// nit: using `kubectl wait` (which in turn - I hope - uses a watch) instead of polling would allow us to not waste time here
+		// TODO(#91): using `kubectl wait` (which in turn - I hope - uses a watch) instead of
+		// polling would allow us to not waste time here
 		time.Sleep(checkInterval)
 	}
 
@@ -526,8 +527,9 @@ func (d *Deployer) fetchCentralCACert(ctx context.Context) error {
 		return fmt.Errorf("failed to decode CA cert: %w", err)
 	}
 
-	d.roxCACertFile = "/tmp/roxie-ca-cert.pem" // possible symlink race vulnerability
-	// this is a REALLY weird way to write a temporary file in go
+	// TODO(#91): possible symlink race vulnerability
+	d.roxCACertFile = "/tmp/roxie-ca-cert.pem"
+	// TODO(#91): this is a REALLY weird way to write a temporary file in go
 	writeCmd := exec.CommandContext(ctx, "sh", "-c", fmt.Sprintf("cat > %s", d.roxCACertFile))
 	writeCmd.Stdin = bytes.NewReader(caCert)
 	if err := writeCmd.Run(); err != nil {
@@ -564,7 +566,7 @@ func (d *Deployer) configureCentralEndpoint(ctx context.Context, exposure string
 			return fmt.Errorf("failed to get LoadBalancer endpoint: %w", err)
 		}
 		// Remove https:// prefix if present (waitForLoadBalancer returns https://ip:443)
-		// This is silly, why add these affixes there, and then strip here?!
+		// TODO(#91): This is silly, why add these affixes there, and then strip here?!
 		d.centralEndpoint = strings.TrimPrefix(endpoint, "https://")
 		d.centralEndpoint = strings.TrimSuffix(d.centralEndpoint, ":443")
 		d.centralEndpoint = d.centralEndpoint + ":443"
@@ -787,7 +789,7 @@ func (d *Deployer) waitForSecuredClusterReady(ctx context.Context, timeout int)
 			}
 
 			// collector seems to be crashing on some local cluster types/versions.
-			// skip the check only on local clusters, then?
+			// TODO(#91): skip the check only on local clusters, then?
 		}
 
 		if allReady {
diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go
index dc2b7f3e..b957b74d 100644
--- a/internal/deployer/deployer.go
+++ b/internal/deployer/deployer.go
@@ -32,8 +32,8 @@ var (
 
 	pauseReconcileAnnotationKey = "stackrox.io/pause-reconcile"
 
-	// at some point this will get out of date
-	// if we filter by the app.../part-of label anyway, then maybe we should just delete all resource kinds present on cluster?
+	// TODO(#91): at some point this will get out of date. If we filter by the app.../part-of
+	// label anyway, then maybe we should just delete all resource kinds present on cluster?
 	// also we should use the fully-qualified types
 	allInstallableCentralResourceKinds = []string{
 		"applications",
@@ -346,7 +346,8 @@ func (d *Deployer) SetCombinedOverrideFile(overrideFile string) error {
 func setOverrideSetExpressions(overrides map[string]interface{}, prefix string, overrideSetExpressions []string) ([]string, error) {
 	remainingSetExpressions := make([]string, 0)
 	for _, expr := range overrideSetExpressions {
-		parts := splitAtFirstEquals(expr) // would https://pkg.go.dev/strings#Cut work instead?
+		// TODO(#91): would https://pkg.go.dev/strings#Cut work instead?
+		parts := splitAtFirstEquals(expr)
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("invalid override expression '%s': expected format 'key.path=value'", expr)
 		}
@@ -1254,7 +1255,8 @@ func (d *Deployer) checkPodProgressInNamespace(ctx context.Context, namespace st
 	}
 }
 
-// plenty of code in common with the central variant that should probably be extracted
+// TODO(#91): plenty of code in common with the central variant that should probably be
+// extracted
 func (d *Deployer) PrintSecuredClusterDeploymentSummary() {
 	component := "Secured Cluster"
 	mainImageTag := d.mainImageTag
diff --git a/internal/deployer/kubectl.go b/internal/deployer/kubectl.go
index 7badbc3d..1205253d 100644
--- a/internal/deployer/kubectl.go
+++ b/internal/deployer/kubectl.go
@@ -26,7 +26,9 @@ type KubectlResult struct {
 }
 
 // runKubectl executes a kubectl command with automatic retries on transient errors
-func (d *Deployer) runKubectl(ctx context.Context, opts KubectlOptions) (*KubectlResult, error) { // perhaps this should not return a pointer so that the lack of nil checks elsewhere is robust?
+// TODO(#91): perhaps this should not return a pointer so that the lack of nil checks elsewhere
+// is robust?
+func (d *Deployer) runKubectl(ctx context.Context, opts KubectlOptions) (*KubectlResult, error) {
 	if opts.MaxAttempts <= 0 {
 		opts.MaxAttempts = 3
 	}
diff --git a/internal/deployer/operator.go b/internal/deployer/operator.go
index bc487a9b..bb2d7936 100644
--- a/internal/deployer/operator.go
+++ b/internal/deployer/operator.go
@@ -107,8 +107,8 @@ func (d *Deployer) identifyCRDFileNames(bundleDir string) ([]string, error) {
 			return nil
 		}
 
-		// The following detection logic does not seem particularly robust.
-		// We should probably parse the YAML and check api group and kind fields.
+		// TODO(#91): The following detection logic does not seem particularly robust. We should
+		// probably parse the YAML and check api group and kind fields.
 		name := strings.ToLower(info.Name())
 		if strings.Contains(name, "customresourcedefinition") ||
 			strings.Contains(name, "platform.stackrox.io") ||
diff --git a/internal/deployer/operator_olm.go b/internal/deployer/operator_olm.go
index 5c5de587..7ca29cbb 100644
--- a/internal/deployer/operator_olm.go
+++ b/internal/deployer/operator_olm.go
@@ -82,8 +82,10 @@ func (d *Deployer) checkOLMInstalled(ctx context.Context) error {
 	}
 
 	for _, crd := range requiredCRDs {
+		// TODO(#91): actually this is not the right way to check, since a CRD could be in a
+		// broken state; we should use the api-resources subcommand instead
 		_, err := d.runKubectl(ctx, KubectlOptions{
-			Args: []string{"get", "crd", crd}, // actually this is not the right way to check, since a CRD could be in a broken state; we should use the api-resources subcommand instead
+			Args: []string{"get", "crd", crd},
 		})
 		if err != nil {
 			return fmt.Errorf("OLM not installed: CRD %s not found. Please install OLM first", crd)
@@ -124,7 +126,7 @@ func (d *Deployer) createCatalogSource(ctx context.Context, indexImage string) e
 		},
 	}
 
-	// Add security context config if supported.
+	// TODO(#91): Add security context config if supported.
 	if hasSecurityContextConfig {
 		spec := catalogSource["spec"].(map[string]interface{})
 		spec["grpcPodConfig"] = map[string]interface{}{
@@ -158,7 +160,9 @@ func (d *Deployer) catalogSourceSupportsSecurityContextConfig(ctx context.Contex
 		return false, err
 	}
 
-	return strings.Contains(result.Stdout, "securityContextConfig"), nil // this is overly optimistic and would incorrectly succeed if an api version that contains this had "serving: false"
+	// TODO(#91): this is overly optimistic and would incorrectly succeed if an api version
+	// that contains this had "serving: false"
+	return strings.Contains(result.Stdout, "securityContextConfig"), nil
 }
 
 // createOperatorGroup creates the OperatorGroup.
@@ -251,7 +255,9 @@ func (d *Deployer) waitForAndApproveInstallPlan(ctx context.Context) error {
 	}
 
 	if time.Since(start) >= timeout {
-		return errors.New("timeout waiting for InstallPlan to be created") // some more info on what was wrong would be useful: a dump of the subscription or at least its name so that the user can investigate
+		// TODO(#91): some more info on what was wrong would be useful: a dump of the
+		// subscription or at least its name so that the user can investigate
+		return errors.New("timeout waiting for InstallPlan to be created")
 	}
 
 	// Sanity check:Verify currentCSV matches expected version.
@@ -321,7 +327,8 @@ func (d *Deployer) waitForCSVSuccess(ctx context.Context) error {
 		time.Sleep(5 * time.Second)
 	}
 
-	return fmt.Errorf("timeout waiting for CSV to succeed") // same as above
+	// TODO(#91): same as above
+	return fmt.Errorf("timeout waiting for CSV to succeed")
 }
 
 // detectOperatorDeploymentMode detects how the operator is currently deployed.
@@ -344,7 +351,9 @@ func (d *Deployer) detectOperatorDeploymentMode(ctx context.Context) (bool, Oper
 		result, err := d.runKubectl(ctx, KubectlOptions{
 			Args: []string{"get", "deployment", operatorDeploymentName, "-n", operatorNamespace, "-o", "jsonpath={.metadata.labels}"},
 		})
-		if err == nil && strings.Contains(result.Stdout, "olm.owner") { // This is not very robust. Better retrieve a specific label in the `get` command?
+		// TODO(#91): This is not very robust. Better retrieve a specific label in the `get`
+		// command?
+		if err == nil && strings.Contains(result.Stdout, "olm.owner") {
 			return true, OperatorModeOLM
 		}
 		// Deployment exists without OLM labels = non-OLM deployment.
diff --git a/internal/deployer/override.go b/internal/deployer/override.go
index bfd4746b..324ac1b9 100644
--- a/internal/deployer/override.go
+++ b/internal/deployer/override.go
@@ -34,7 +34,9 @@ func setNestedValue(m map[string]interface{}, path string, value interface{}) er
 			// Check if existing value is a map
 			if existingMap, isMap := existing.(map[string]interface{}); isMap {
 				current = existingMap
-			} else { // shouldn't this be an error instead? I think it's more likely someone made a typo...
+			} else {
+				// TODO(#91): shouldn't this be an error instead? I think it's more likely someone
+				// made a typo...
 				// Existing value is not a map, we need to replace it
 				newMap := make(map[string]interface{})
 				current[key] = newMap
diff --git a/internal/env/env.go b/internal/env/env.go
index 098c5188..d3ae4fae 100644
--- a/internal/env/env.go
+++ b/internal/env/env.go
@@ -328,7 +328,7 @@ func fetchAPIResources() ([]string, error) {
 }
 
 func IsInStackroxRepository() bool {
-	// This assumes that:
+	// TODO(#91): This assumes that:
 	// - origin is the name of the upstream remote (not true if someone cloned their fork)
 	// - the git transport was used
 	// How about instead looking for "# StackRox Kubernetes Security Platform" in README?
diff --git a/internal/helpers/tag.go b/internal/helpers/tag.go
index 7fee83ff..39a3cbb3 100644
--- a/internal/helpers/tag.go
+++ b/internal/helpers/tag.go
@@ -9,7 +9,8 @@ import (
 )
 
 const (
-	defaultMainImageTag = "4.9.2" // Is the plan to keep bumping this on new ACS releases?
+	// TODO(#91): Is the plan to keep bumping this on new ACS releases?
+	defaultMainImageTag = "4.9.2"
 )
 
 func LookupMainImageTag(log *logger.Logger) (string, error) {
diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go
index 0fe86539..8d61187d 100644
--- a/internal/imagecache/imagecache.go
+++ b/internal/imagecache/imagecache.go
@@ -33,7 +33,8 @@ func New(log *logger.Logger, cacheFile string, maxEntries int) *ImageCache {
 		if err != nil {
 			home = "."
 		}
-		cacheFile = filepath.Join(home, ".roxie.image_cache") // how about using something XDG-compliant like ~/.cache/roxie/images?
+		// TODO(#91): how about using something XDG-compliant like ~/.cache/roxie/images?
+		cacheFile = filepath.Join(home, ".roxie.image_cache")
 	}
 
 	if maxEntries <= 0 {
diff --git a/internal/portforward/portforward.go b/internal/portforward/portforward.go
index cb502f1d..d1ea1159 100644
--- a/internal/portforward/portforward.go
+++ b/internal/portforward/portforward.go
@@ -106,7 +106,8 @@ func (m *Manager) Start(namespace, serviceName string, remotePort, preferredLoca
 			if err == nil {
 				syscall.Kill(-pgid, syscall.SIGTERM)
 			}
-			cmd.Wait() // AFAICT this can get stuck forever if the process blocks SIGTERM...
+			// TODO(#91): AFAICT this can get stuck forever if the process blocks SIGTERM...
+			cmd.Wait()
 		}
 		return "", fmt.Errorf("port-forward did not become ready")
 	}
diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go
index 23520acd..90219a11 100644
--- a/tests/e2e/e2e_test.go
+++ b/tests/e2e/e2e_test.go
@@ -17,7 +17,8 @@ import (
 )
 
 const (
-	defaultMainImageTag = "4.8.2" // this is a bit old...
+	// TODO(#91): this is a bit old...
+	defaultMainImageTag = "4.8.2"
 	deployTimeout       = 30 * time.Minute
 	teardownTimeout     = 10 * time.Minute
 )
@@ -67,7 +68,8 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-func teardownAllDeployments() error { // maybe put the helper functions in a separate file?
+// TODO(#91): maybe put the helper functions in a separate file?
+func teardownAllDeployments() error {
 	fmt.Println("=== Tearing down all deployments before running tests ===")
 
 	ctx, cancel := context.WithTimeout(context.Background(), teardownTimeout)

From ee203f2b6ce1a3cf33b28c31807afa9ba7627df4 Mon Sep 17 00:00:00 2001
From: Marcin Owsiany <porridge@redhat.com>
Date: Tue, 14 Apr 2026 10:37:23 +0200
Subject: [PATCH 3/3] elaborate

---
 internal/deployer/operator_olm.go | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/internal/deployer/operator_olm.go b/internal/deployer/operator_olm.go
index 7ca29cbb..94b33f04 100644
--- a/internal/deployer/operator_olm.go
+++ b/internal/deployer/operator_olm.go
@@ -82,8 +82,9 @@ func (d *Deployer) checkOLMInstalled(ctx context.Context) error {
 	}
 
 	for _, crd := range requiredCRDs {
-		// TODO(#91): actually this is not the right way to check, since a CRD could be in a
-		// broken state; we should use the api-resources subcommand instead
+		// TODO(#91): actually this is not the right way to check whether it's safe to create a resource of a given kind.
+		// A CRD can be present, but still being loaded or end up not accepted by the API server.
+		// Instead we should use the `kubectl api-resources` subcommand which exposes the status we're looking for.
 		_, err := d.runKubectl(ctx, KubectlOptions{
 			Args: []string{"get", "crd", crd},
 		})