From 5b75bbe45a523a84a01e8b2e24f5cd32c499c49a Mon Sep 17 00:00:00 2001 From: atbagan Date: Thu, 11 Jun 2026 08:21:45 -0500 Subject: [PATCH] fix(cli): provider get/list read correct scope and status paths The provider detail view and list wide column read spec.scope as a string, but the CRD stores scope as an object (spec.scope.type), so a team-scoped provider rendered as "platform" via the absent-scope fallback. They now read spec.scope.type. Two status paths were also wrong: the validation error was read from a non-existent status.validationError field, and the last-validation timestamp from status.lastValidatedAt. The error is recorded as the Ready condition's message, and the timestamp field is status.lastValidationTime; both are now read correctly. Found by exercising provider get against a real team-scoped provider, the same display-path class of bug fixed for idp get earlier. --- internal/adm/provider/get.go | 2 +- internal/adm/provider/provider.go | 24 ++++++++-- internal/adm/provider/provider_p15_test.go | 56 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/internal/adm/provider/get.go b/internal/adm/provider/get.go index 97a5774..fe4c32d 100644 --- a/internal/adm/provider/get.go +++ b/internal/adm/provider/get.go @@ -109,7 +109,7 @@ func printProviderDetail(out io.Writer, pc *unstructured.Unstructured) error { t.AddRow("Endpoint", info.Endpoint) } - scope := client.GetNestedString(pc.Object, "spec", "scope") + scope := client.GetNestedString(pc.Object, "spec", "scope", "type") if scope == "" { scope = "platform" } diff --git a/internal/adm/provider/provider.go b/internal/adm/provider/provider.go index 421b2c6..4928849 100644 --- a/internal/adm/provider/provider.go +++ b/internal/adm/provider/provider.go @@ -190,7 +190,7 @@ func runList(ctx context.Context, logger *log.Logger, opts *listOptions) error { row := []string{name, provider, validatedStr, endpoint, age} if wide { ns := pc.GetNamespace() - scope := client.GetNestedString(pc.Object, "spec", "scope") + scope := client.GetNestedString(pc.Object, "spec", "scope", "type") if scope == "" { scope = "platform" } @@ -673,6 +673,24 @@ type ProviderInfo struct { LastValidatedAt string `json:"lastValidatedAt,omitempty"` } +// providerValidationError returns the message of the Ready condition when it +// is not True. The ProviderConfig status has no validationError field; a failed +// validation is recorded as the Ready condition's message (see +// updateProviderConfigStatus), so that is where the error must be read from. +func providerValidationError(pc *unstructured.Unstructured) string { + conditions, _, _ := unstructured.NestedSlice(pc.Object, "status", "conditions") + for _, c := range conditions { + cond, ok := c.(map[string]interface{}) + if !ok { + continue + } + if client.GetNestedString(cond, "type") == "Ready" && client.GetNestedString(cond, "status") != "True" { + return client.GetNestedString(cond, "message") + } + } + return "" +} + func extractProviderInfo(pc *unstructured.Unstructured) ProviderInfo { provider := client.GetNestedString(pc.Object, "spec", "provider") @@ -699,8 +717,8 @@ func extractProviderInfo(pc *unstructured.Unstructured) ProviderInfo { Provider: provider, Validated: client.GetNestedBool(pc.Object, "status", "validated"), Endpoint: endpoint, - ValidationError: client.GetNestedString(pc.Object, "status", "validationError"), - LastValidatedAt: client.GetNestedString(pc.Object, "status", "lastValidatedAt"), + ValidationError: providerValidationError(pc), + LastValidatedAt: client.GetNestedString(pc.Object, "status", "lastValidationTime"), } } diff --git a/internal/adm/provider/provider_p15_test.go b/internal/adm/provider/provider_p15_test.go index 801f39c..b66137e 100644 --- a/internal/adm/provider/provider_p15_test.go +++ b/internal/adm/provider/provider_p15_test.go @@ -48,6 +48,62 @@ func TestPrintProviderDetail(t *testing.T) { } } +// TestPrintProviderDetailScope guards the scope display. The CRD stores scope as +// an object (spec.scope.type), so a team-scoped provider must render "team", not +// the "platform" fallback. An absent scope falls back to "platform". +func TestPrintProviderDetailScope(t *testing.T) { + teamScoped := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{"name": "p", "namespace": "butler-system"}, + "spec": map[string]interface{}{ + "provider": "harvester", + "scope": map[string]interface{}{"type": "team", "teamRef": map[string]interface{}{"name": "eng"}}, + }, + }} + var out bytes.Buffer + if err := printProviderDetail(&out, teamScoped); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), "team") { + t.Errorf("team-scoped provider should render scope 'team':\n%s", out.String()) + } + + unscoped := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{"name": "p", "namespace": "butler-system"}, + "spec": map[string]interface{}{"provider": "harvester"}, + }} + var out2 bytes.Buffer + _ = printProviderDetail(&out2, unscoped) + if !strings.Contains(out2.String(), "platform") { + t.Errorf("unscoped provider should fall back to scope 'platform':\n%s", out2.String()) + } +} + +// TestProviderValidationError guards reading the validation error from the Ready +// condition (the status has no validationError field). +func TestProviderValidationError(t *testing.T) { + failed := &unstructured.Unstructured{Object: map[string]interface{}{ + "status": map[string]interface{}{"conditions": []interface{}{ + map[string]interface{}{"type": "Ready", "status": "False", "message": "auth failed"}, + }}, + }} + if got := providerValidationError(failed); got != "auth failed" { + t.Errorf("validation error = %q, want %q", got, "auth failed") + } + + ok := &unstructured.Unstructured{Object: map[string]interface{}{ + "status": map[string]interface{}{"conditions": []interface{}{ + map[string]interface{}{"type": "Ready", "status": "True", "message": "ok"}, + }}, + }} + if got := providerValidationError(ok); got != "" { + t.Errorf("Ready=True must yield no error, got %q", got) + } + + if got := providerValidationError(&unstructured.Unstructured{Object: map[string]interface{}{}}); got != "" { + t.Errorf("no conditions must yield no error, got %q", got) + } +} + // TestMergeProviderSpec is the load-bearing update correctness test. A spec // update must replace spec but PRESERVE the existing status and // resourceVersion, so it never clobbers validation status and the Update