Health/pre-flight wiring polish from v0.8.0 testing (REF-148)

internal/commands/cluster/actions.go

@@ -13,6 +13,7 @@ import (
 	"github.com/dantech2000/refresh/internal/commands/clusterview"
 	"github.com/dantech2000/refresh/internal/commands/factory"
 	"github.com/dantech2000/refresh/internal/commands/runner"
+	"github.com/dantech2000/refresh/internal/health"
 	clustersvc "github.com/dantech2000/refresh/internal/services/cluster"
 	"github.com/dantech2000/refresh/internal/services/status"
 	"github.com/dantech2000/refresh/internal/ui"
@@ -126,7 +127,15 @@ func runDescribe(ctx context.Context, cmd *cli.Command) error {
 	if cmd.Bool("check-readiness") {
 		humanOutput := strings.EqualFold(cmd.String("format"), "table")
 		k8sClient := resolveReadinessKubeClient(ctx, cmd.String("kubeconfig"), humanOutput)
-		clusterService = factory.NewClusterServiceWithHealth(awsCfg, k8sClient, nil)
+		// With cluster access, also wire metrics-server (best-effort) so the
+		// health card's live-utilization check measures instead of skipping. (REF-146)
+		var metricsClient health.NodeMetricsLister
+		if k8sClient != nil {
+			if m, err := health.BuildMetricsClient(cmd.String("kubeconfig")); err == nil {
+				metricsClient = m
+			}
+		}
+		clusterService = factory.NewClusterServiceWithHealth(awsCfg, k8sClient, metricsClient, nil)
 	} else {
 		clusterService = factory.NewClusterService(awsCfg, cmd.Bool("show-health"), nil)
 	}

internal/commands/factory/factory.go

@@ -10,6 +10,7 @@ import (
 	"github.com/aws/aws-sdk-go-v2/service/autoscaling"
 	"github.com/aws/aws-sdk-go-v2/service/cloudwatch"
 	"github.com/aws/aws-sdk-go-v2/service/eks"
+	"github.com/aws/aws-sdk-go-v2/service/servicequotas"
 	"k8s.io/client-go/kubernetes"
 
 	"github.com/dantech2000/refresh/internal/health"
@@ -54,15 +55,31 @@ func NewDefaultLogger(logger *slog.Logger) *slog.Logger {
 	return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: defaultLogLevel}))
 }
 
+// newHealthChecker builds a health checker with the AWS-backed clients always
+// wired — including Service Quotas, which needs no cluster access — plus the
+// optional Kubernetes and metrics-server clients. Centralizing construction
+// here keeps every entry point (describe, scale, upgrade) consistent so a check
+// isn't silently skipped just because one command forgot to wire its client.
+func newHealthChecker(awsCfg aws.Config, k8sClient kubernetes.Interface, metricsClient health.NodeMetricsLister) *health.HealthChecker {
+	hc := health.NewChecker(
+		eks.NewFromConfig(awsCfg),
+		k8sClient,
+		cloudwatch.NewFromConfig(awsCfg),
+		autoscaling.NewFromConfig(awsCfg),
+	)
+	hc.SetServiceQuotas(servicequotas.NewFromConfig(awsCfg))
+	if metricsClient != nil {
+		hc.SetNodeMetrics(metricsClient)
+	}
+	return hc
+}
+
 // NewClusterService initializes a cluster service with optional health checking.
 func NewClusterService(awsCfg aws.Config, withHealth bool, logger *slog.Logger) *cluster.ServiceImpl {
 	logger = NewDefaultLogger(logger)
 	var hc *health.HealthChecker
 	if withHealth {
-		eksClient := eks.NewFromConfig(awsCfg)
-		cwClient := cloudwatch.NewFromConfig(awsCfg)
-		asgClient := autoscaling.NewFromConfig(awsCfg)
-		hc = health.NewChecker(eksClient, nil, cwClient, asgClient)
+		hc = newHealthChecker(awsCfg, nil, nil)
 	}
 	return cluster.NewService(awsCfg, hc, logger)
 }
@@ -72,10 +89,7 @@ func NewNodegroupService(awsCfg aws.Config, withHealth bool, logger *slog.Logger
 	logger = NewDefaultLogger(logger)
 	var hc *health.HealthChecker
 	if withHealth {
-		eksClient := eks.NewFromConfig(awsCfg)
-		cwClient := cloudwatch.NewFromConfig(awsCfg)
-		asgClient := autoscaling.NewFromConfig(awsCfg)
-		hc = health.NewChecker(eksClient, nil, cwClient, asgClient)
+		hc = newHealthChecker(awsCfg, nil, nil)
 	}
 	return nodegroup.NewService(awsCfg, hc, logger)
 }
@@ -91,13 +105,9 @@ func NewAddonService(awsCfg aws.Config, logger *slog.Logger) *addons.ServiceImpl
 // kube-dependent signals degrade gracefully). Use this when a command has
 // resolved a --kubeconfig so measured node readiness runs against the right
 // cluster. (REF-130)
-func NewClusterServiceWithHealth(awsCfg aws.Config, k8sClient kubernetes.Interface, logger *slog.Logger) *cluster.ServiceImpl {
+func NewClusterServiceWithHealth(awsCfg aws.Config, k8sClient kubernetes.Interface, metricsClient health.NodeMetricsLister, logger *slog.Logger) *cluster.ServiceImpl {
 	logger = NewDefaultLogger(logger)
-	eksClient := eks.NewFromConfig(awsCfg)
-	cwClient := cloudwatch.NewFromConfig(awsCfg)
-	asgClient := autoscaling.NewFromConfig(awsCfg)
-	hc := health.NewChecker(eksClient, k8sClient, cwClient, asgClient)
-	return cluster.NewService(awsCfg, hc, logger)
+	return cluster.NewService(awsCfg, newHealthChecker(awsCfg, k8sClient, metricsClient), logger)
 }
 
 // NewNodegroupServiceWithHealth initializes a nodegroup service whose health
@@ -106,9 +116,5 @@ func NewClusterServiceWithHealth(awsCfg aws.Config, k8sClient kubernetes.Interfa
 // resolved a --kubeconfig so workload/PDB checks run against the right cluster.
 func NewNodegroupServiceWithHealth(awsCfg aws.Config, k8sClient kubernetes.Interface, logger *slog.Logger) *nodegroup.ServiceImpl {
 	logger = NewDefaultLogger(logger)
-	eksClient := eks.NewFromConfig(awsCfg)
-	cwClient := cloudwatch.NewFromConfig(awsCfg)
-	asgClient := autoscaling.NewFromConfig(awsCfg)
-	hc := health.NewChecker(eksClient, k8sClient, cwClient, asgClient)
-	return nodegroup.NewService(awsCfg, hc, logger)
+	return nodegroup.NewService(awsCfg, newHealthChecker(awsCfg, k8sClient, nil), logger)
 }

internal/commands/nodegroup/actions_scale.go

@@ -63,6 +63,11 @@ func runScale(ctx context.Context, cmd *cli.Command) error {
 		return err
 	}
 
+	// Pre-flight: warn if the nodegroup's instance type isn't offered in one of
+	// its AZs — a scale-up would fail to place nodes there. Runs for both the
+	// dry-run preview and a real scale, so the preview surfaces it too. (REF-143)
+	warnInstanceTypeAvailability(ctx, svc, clusterName, cmd.String("nodegroup"))
+
 	if opts.DryRun {
 		// With --check-pdbs, surface the actual PDBs that would constrain a
 		// scale-down (name/namespace/disruptions-allowed), not just a generic
@@ -78,10 +83,6 @@ func runScale(ctx context.Context, cmd *cli.Command) error {
 		return printScaleDryRun(ctx, eks.NewFromConfig(awsCfg), clusterName, cmd.String("nodegroup"), desired, minSize, maxSize, pdbs)
 	}
 
-	// Pre-flight: warn if the nodegroup's instance type isn't offered in one of
-	// its AZs — a scale-up would fail to place nodes there. (REF-143)
-	warnInstanceTypeAvailability(ctx, svc, clusterName, cmd.String("nodegroup"))
-
 	return runner.WithSpinner("nodegroup", "Scaling request submitted", func() error {
 		return svc.Scale(ctx, clusterName, cmd.String("nodegroup"), desired, minSize, maxSize, opts)
 	})

internal/health/checker.go

@@ -113,10 +113,15 @@ func aggregateResults(results []HealthResult) HealthSummary {
 	hasWarnings := false
 
 	for _, result := range results {
-		if !result.Skipped {
-			totalScore += result.Score
-			measuredCount++
+		// A skipped check could not be evaluated (missing prerequisite, e.g. no
+		// Kubernetes/metrics client). It contributes neither to the score nor to
+		// the verdict — otherwise a missing prerequisite would wrongly force a
+		// WARN decision on an otherwise-healthy cluster. (REF-146)
+		if result.Skipped {
+			continue
 		}
+		totalScore += result.Score
+		measuredCount++
 
 		switch {
 		case result.Status == StatusFail && result.IsBlocking:

internal/health/checker_test.go

@@ -75,21 +75,39 @@ func TestDecision_BlockingBeatsWarn(t *testing.T) {
 // aggregateResults is the real aggregation used by RunAllChecks (defined in
 // checker.go); these tests exercise it directly so they don't touch AWS.
 
-func TestAggregate_SkippedCheckExcludedFromScore(t *testing.T) {
-	// A perfect measured cluster with one skipped check (no kube client →
-	// fixed 70) must not be dragged down: score reflects only measured checks.
+func TestAggregate_SkippedCheckExcludedFromScoreAndVerdict(t *testing.T) {
+	// A perfect measured cluster with one skipped check (missing prerequisite,
+	// e.g. no kube client) must not be dragged down OR flipped to WARN: a
+	// skipped check contributes to neither the score nor the verdict. (REF-146)
 	results := []HealthResult{
 		{Status: StatusPass, Score: 100, IsBlocking: false},
 		{Status: StatusPass, Score: 100, IsBlocking: false},
 		{Status: StatusWarn, Score: 70, IsBlocking: false, Skipped: true, Message: "k8s unavailable"},
 	}
 	summary := aggregateResults(results)
 	if summary.OverallScore != 100 {
-		t.Errorf("skipped check should be excluded: score = %d, want 100", summary.OverallScore)
+		t.Errorf("skipped check should be excluded from score: got %d, want 100", summary.OverallScore)
 	}
-	// A skipped WARN still surfaces as a warning in the decision.
+	if summary.Decision != DecisionProceed {
+		t.Errorf("a skipped check must not force WARN: decision = %s, want PROCEED", summary.Decision)
+	}
+	if len(summary.Warnings) != 0 {
+		t.Errorf("skipped check should not surface a warning: %v", summary.Warnings)
+	}
+}
+
+func TestAggregate_RealWarningStillWarns(t *testing.T) {
+	// A genuine (non-skipped) WARN must still drive the verdict — only skipped
+	// checks are excluded.
+	summary := aggregateResults([]HealthResult{
+		{Status: StatusPass, Score: 100},
+		{Status: StatusWarn, Score: 70, Message: "limited capacity"},
+	})
 	if summary.Decision != DecisionWarn {
-		t.Errorf("decision = %s, want WARN", summary.Decision)
+		t.Errorf("decision = %s, want WARN for a real warning", summary.Decision)
+	}
+	if len(summary.Warnings) != 1 {
+		t.Errorf("warnings = %v, want 1", summary.Warnings)
 	}
 }
 

internal/health/utilization.go

@@ -37,7 +37,7 @@ func (hc *HealthChecker) CheckNodeUtilization(ctx context.Context, _ string) Hea
 			Name:    "Node Utilization",
 			Status:  StatusPass,
 			Skipped: true,
-			Message: "live utilization unavailable (metrics-server not configured)",
+			Message: "live utilization unavailable (no metrics client wired for this command)",
 		}
 	}