From e83f9b87a1e51a24051b540abb9cc3a65c142d49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Va=C5=A1ek?= Date: Mon, 8 Jun 2026 22:45:25 +0200 Subject: [PATCH] refactor: merge cluster service arrays into record useClusterService returned Knative Services and Deployments as separate arrays. Consumers had to manually correlate them by revision label, and a naming bug stored the ksvc in a field called "deployment". The hook now returns a Record with pre-correlated pairs. The revision-matching logic moves into the hook, simplifying every consumer to a single record lookup. Renamed the misleading field to "knativeService" on FunctionTableItem and fixed the test fixture to use an actual Knative Service object. Co-Authored-By: Claude --- docs/claude-progress.txt | 18 +++ docs/features.json | 11 ++ .../cluster/useClusterService.test.tsx | 112 ++++++++++++++---- .../services/cluster/useClusterService.ts | 38 +++++- .../FunctionCreatePage.test.tsx | 3 +- .../function-list/FunctionsListPage.test.tsx | 82 ++++++------- src/pages/function-list/FunctionsListPage.tsx | 20 +--- .../components/FunctionTable.test.tsx | 15 ++- .../components/FunctionTable.tsx | 10 +- 9 files changed, 215 insertions(+), 94 deletions(-) diff --git a/docs/claude-progress.txt b/docs/claude-progress.txt index 1591bd8..be37a48 100644 --- a/docs/claude-progress.txt +++ b/docs/claude-progress.txt @@ -1,6 +1,24 @@ # Claude Progress Log # Newest entries first. Agents: append your entry at the top after the header. +--- +## 2026-06-08 | Session: useClusterService resource coupling fix (SRVOCF-841 item 1) +Worked on: Merge separate knativeServices/deployments arrays into correlated Record, fix naming bug, improve tests +Completed: +- Changed useClusterService return type from { knativeServices[], deployments[] } to { functions: Record } +- Moved revision-based correlation logic (latestReadyRevisionName matching) from FunctionsListPage into the hook's useMemo +- Renamed misleading `deployment` field to `knativeService` on FunctionTableItem (the field held a Knative Service, not a Deployment) +- Fixed FunctionTable.test.tsx fixture from Deployment object to actual Knative Service object +- Simplified FunctionsListPage merge memo from 11-line find/match to 3-line record lookup +- Added 4 new hook tests: error propagation, co-presence pairing, ksvc without deployment, revision-based matching +- Added ownerReferences to deployment fixtures (documents Knative ownership chain) +- Removed "picks latest revision" test from FunctionsListPage (logic moved to hook, tested there) +- Updated FunctionCreatePage test mock to match new return shape +- Fixed react-hooks/exhaustive-deps lint warnings by moving conditionals inside useMemo +- 14 suites, 129 tests passing, zero lint errors +Left off: Changes ready for commit on branch cleanup-cluster-svc. +Blockers: None + --- ## 2026-05-21 | Session: Page co-location restructure (continued) Worked on: Follow-up fixes from review, test cleanup, docs diff --git a/docs/features.json b/docs/features.json index d888d48..2907c49 100644 --- a/docs/features.json +++ b/docs/features.json @@ -180,5 +180,16 @@ "All tests pass, no broken imports" ], "passes": true + }, + { + "category": "technical", + "description": "useClusterService: fix resource coupling and improve tests. Knative Services own their Deployments via OwnerReferences, so they are always co-present. Clarify this invariant in tests and consider simplifying to a single merged return type.", + "steps": [ + "Audit useClusterService return type for redundant Knative Service / Deployment separation", + "Simplify to a single merged return type if warranted", + "Update tests to reflect the co-presence invariant", + "All tests pass, no broken imports, build clean" + ], + "passes": true } ] diff --git a/src/common/services/cluster/useClusterService.test.tsx b/src/common/services/cluster/useClusterService.test.tsx index 3cd3ab7..d999dff 100644 --- a/src/common/services/cluster/useClusterService.test.tsx +++ b/src/common/services/cluster/useClusterService.test.tsx @@ -32,28 +32,35 @@ const mockDeployment = { 'function.knative.dev/name': 'my-func', 'serving.knative.dev/revision': 'my-func-00001', }, + ownerReferences: [ + { + apiVersion: 'serving.knative.dev/v1', + kind: 'Revision', + name: 'my-func-00001', + uid: 'rev-uid-001', + controller: true, + blockOwnerDeletion: true, + }, + ], }, spec: { replicas: 1 }, status: { readyReplicas: 1 }, }; function TestConsumer({ functionNames = [] }: { functionNames?: string[] }) { - const { knativeServices, deployments, loaded, error } = useClusterService(functionNames); + const { functions, loaded, error } = useClusterService(functionNames); + const entries = Object.entries(functions); return ( <> {String(loaded)} {String(error)} - {knativeServices.length} - {deployments.length} - {knativeServices.map((s) => ( - - {s.metadata?.name} - - ))} - {deployments.map((d) => ( - - {d.metadata?.name} - + {entries.length} + {entries.map(([name, info]) => ( +
+ {name} + {info.knativeService.metadata?.name} + {info.deployment?.metadata?.name ?? 'none'} +
))} ); @@ -71,8 +78,7 @@ describe('useClusterService', () => { expect(mockUseK8sWatchResource).toHaveBeenCalledWith(null); expect(screen.getByTestId('loaded')).toHaveTextContent('true'); - expect(screen.getByTestId('ksvc-count')).toHaveTextContent('0'); - expect(screen.getByTestId('dep-count')).toHaveTextContent('0'); + expect(screen.getByTestId('fn-count')).toHaveTextContent('0'); }); it('watches Knative Services with In selector for given function names', () => { @@ -91,13 +97,13 @@ describe('useClusterService', () => { ], }, }); - expect(screen.getByTestId('ksvc-count')).toHaveTextContent('1'); - expect(screen.getByTestId('ksvc')).toHaveTextContent('my-func'); + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('fn-ksvc')).toHaveTextContent('my-func'); }); it('watches Deployments with In selector for given function names', () => { mockUseK8sWatchResource - .mockReturnValueOnce([[], true, null]) + .mockReturnValueOnce([[mockKsvc], true, null]) .mockReturnValueOnce([[mockDeployment], true, null]); render(); @@ -111,11 +117,10 @@ describe('useClusterService', () => { ], }, }); - expect(screen.getByTestId('dep-count')).toHaveTextContent('1'); - expect(screen.getByTestId('deployment')).toHaveTextContent('my-func-00001-deployment'); + expect(screen.getByTestId('fn-dep')).toHaveTextContent('my-func-00001-deployment'); }); - it('returns empty arrays when not loaded', () => { + it('returns empty record when not loaded', () => { mockUseK8sWatchResource .mockReturnValueOnce([[], false, null]) .mockReturnValueOnce([[], false, null]); @@ -123,7 +128,70 @@ describe('useClusterService', () => { render(); expect(screen.getByTestId('loaded')).toHaveTextContent('false'); - expect(screen.getByTestId('ksvc-count')).toHaveTextContent('0'); - expect(screen.getByTestId('dep-count')).toHaveTextContent('0'); + expect(screen.getByTestId('fn-count')).toHaveTextContent('0'); + }); + + it('propagates deployment watch error', () => { + mockUseK8sWatchResource + .mockReturnValueOnce([[], true, null]) + .mockReturnValueOnce([[], true, new Error('forbidden')]); + + render(); + + expect(screen.getByTestId('error')).toHaveTextContent('Error: forbidden'); + }); + + it('pairs ksvc and deployment together by revision', () => { + mockUseK8sWatchResource + .mockReturnValueOnce([[mockKsvc], true, null]) + .mockReturnValueOnce([[mockDeployment], true, null]); + + render(); + + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('fn-ksvc')).toHaveTextContent('my-func'); + expect(screen.getByTestId('fn-dep')).toHaveTextContent('my-func-00001-deployment'); + }); + + it('returns undefined deployment when ksvc has no matching deployment', () => { + mockUseK8sWatchResource + .mockReturnValueOnce([[mockKsvc], true, null]) + .mockReturnValueOnce([[], true, null]); + + render(); + + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('fn-dep')).toHaveTextContent('none'); + }); + + it('picks deployment matching the latest ready revision', () => { + const ksvcRev2 = { + ...mockKsvc, + status: { + ...mockKsvc.status, + latestReadyRevisionName: 'my-func-00002', + }, + }; + const oldDep = mockDeployment; + const newDep = { + ...mockDeployment, + metadata: { + ...mockDeployment.metadata, + name: 'my-func-00002-deployment', + labels: { + 'function.knative.dev/name': 'my-func', + 'serving.knative.dev/revision': 'my-func-00002', + }, + }, + status: { readyReplicas: 2 }, + }; + + mockUseK8sWatchResource + .mockReturnValueOnce([[ksvcRev2], true, null]) + .mockReturnValueOnce([[oldDep, newDep], true, null]); + + render(); + + expect(screen.getByTestId('fn-dep')).toHaveTextContent('my-func-00002-deployment'); }); }); diff --git a/src/common/services/cluster/useClusterService.ts b/src/common/services/cluster/useClusterService.ts index e1eeeb5..1949772 100644 --- a/src/common/services/cluster/useClusterService.ts +++ b/src/common/services/cluster/useClusterService.ts @@ -5,10 +5,15 @@ import { OcpClusterService } from './OcpClusterService'; const instance = new OcpClusterService(); const FUNCTION_NAME_LABEL = 'function.knative.dev/name'; +const REVISION_LABEL = 'serving.knative.dev/revision'; + +export interface ClusterFunctionInfo { + knativeService: K8sResourceKind; + deployment: K8sResourceKind | undefined; +} interface ClusterService { - knativeServices: K8sResourceKind[]; - deployments: K8sResourceKind[]; + functions: Record; loaded: boolean; error: unknown; generateKubeconfig: (namespace: string) => Promise; @@ -50,10 +55,33 @@ export function useClusterService(functionNames: string[] = []): ClusterService const [knSvcs, knLoaded, knError] = useK8sWatchResource(knSvcConfig); const [deps, depLoaded, depError] = useK8sWatchResource(depConfig); + const loaded = knLoaded && depLoaded; + + const functions = useMemo(() => { + if (!loaded) return {}; + + const knativeServices = knSvcs ?? []; + const deployments = deps ?? []; + const record: Record = {}; + + for (const ksvc of knativeServices) { + const name = ksvc.metadata?.labels?.[FUNCTION_NAME_LABEL]; + if (!name) continue; + + const latestRevision = ksvc.status?.latestReadyRevisionName; + const deployment = latestRevision + ? deployments.find((d) => d.metadata?.labels?.[REVISION_LABEL] === latestRevision) + : deployments.find((d) => d.metadata?.labels?.[FUNCTION_NAME_LABEL] === name); + + record[name] = { knativeService: ksvc, deployment }; + } + + return record; + }, [loaded, knSvcs, deps]); + return { - knativeServices: knLoaded ? (knSvcs ?? []) : [], - deployments: depLoaded ? (deps ?? []) : [], - loaded: knLoaded && depLoaded, + functions, + loaded, error: knError || depError, generateKubeconfig: instance.generateKubeconfig.bind(instance), }; diff --git a/src/pages/function-create/FunctionCreatePage.test.tsx b/src/pages/function-create/FunctionCreatePage.test.tsx index 227da35..a7bc6df 100644 --- a/src/pages/function-create/FunctionCreatePage.test.tsx +++ b/src/pages/function-create/FunctionCreatePage.test.tsx @@ -37,8 +37,7 @@ vi.mock('../../common/services/source-control/useSourceControlService', () => ({ vi.mock('../../common/services/cluster/useClusterService', () => ({ useClusterService: () => ({ - knativeServices: [], - deployments: [], + functions: {}, loaded: true, error: undefined, generateKubeconfig: mockGenerateKubeconfig, diff --git a/src/pages/function-list/FunctionsListPage.test.tsx b/src/pages/function-list/FunctionsListPage.test.tsx index c6a1f83..77549ea 100644 --- a/src/pages/function-list/FunctionsListPage.test.tsx +++ b/src/pages/function-list/FunctionsListPage.test.tsx @@ -52,15 +52,13 @@ vi.mock('../../common/components/UserAvatar', () => ({ function clusterData( overrides: Partial<{ - knativeServices: unknown[]; - deployments: unknown[]; + functions: Record; loaded: boolean; error: unknown; }> = {}, ) { return { - knativeServices: [], - deployments: [], + functions: {}, loaded: true, error: null, ...overrides, @@ -118,6 +116,16 @@ function deploymentFixture( 'function.knative.dev/name': name, 'serving.knative.dev/revision': revision, }, + ownerReferences: [ + { + apiVersion: 'serving.knative.dev/v1', + kind: 'Revision', + name: revision, + uid: `rev-uid-${revision}`, + controller: true, + blockOwnerDeletion: true, + }, + ], }, spec: { replicas: specReplicas }, status: { readyReplicas }, @@ -179,8 +187,12 @@ describe('FunctionsListPage', () => { }); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 1, 1)], + functions: { + 'my-func': { + knativeService: ksvcFixture('my-func', 'True'), + deployment: deploymentFixture('my-func', 1, 1), + }, + }, }), ); @@ -309,8 +321,12 @@ describe('FunctionsListPage', () => { }); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 1, 1)], + functions: { + 'my-func': { + knativeService: ksvcFixture('my-func', 'True'), + deployment: deploymentFixture('my-func', 1, 1), + }, + }, }), ); @@ -333,8 +349,12 @@ describe('FunctionsListPage', () => { }); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 0, 0)], + functions: { + 'my-func': { + knativeService: ksvcFixture('my-func', 'True'), + deployment: deploymentFixture('my-func', 0, 0), + }, + }, }), ); @@ -356,8 +376,12 @@ describe('FunctionsListPage', () => { }); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'Unknown')], - deployments: [deploymentFixture('my-func', 1, 0)], + functions: { + 'my-func': { + knativeService: ksvcFixture('my-func', 'Unknown'), + deployment: deploymentFixture('my-func', 1, 0), + }, + }, }), ); @@ -378,8 +402,12 @@ describe('FunctionsListPage', () => { }); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'False')], - deployments: [deploymentFixture('my-func', 0, 0)], + functions: { + 'my-func': { + knativeService: ksvcFixture('my-func', 'False'), + deployment: deploymentFixture('my-func', 0, 0), + }, + }, }), ); @@ -392,32 +420,6 @@ describe('FunctionsListPage', () => { expect(await screen.findByTestId('fn-status')).toHaveTextContent('Error'); }); - it('picks latest revision deployment when multiple revisions exist', async () => { - renderAuthenticated(); - mockUseSourceControl.mockReturnValue({ - listFunctionRepos: vi.fn().mockResolvedValue([repoFixture('my-func')]), - fetchFileContent: vi.fn().mockResolvedValue('name: my-func\nruntime: go\nnamespace: demo\n'), - }); - mockUseClusterService.mockReturnValue( - clusterData({ - knativeServices: [ksvcFixture('my-func', 'True', undefined, 'my-func-00002')], - deployments: [ - deploymentFixture('my-func', 0, 0, 'my-func-00001'), - deploymentFixture('my-func', 1, 1, 'my-func-00002'), - ], - }), - ); - - render( - - - , - ); - - expect(await screen.findByTestId('fn-status')).toHaveTextContent('Running'); - expect(screen.getByTestId('fn-replicas')).toHaveTextContent('1'); - }); - it('passes function names to useClusterService', async () => { renderAuthenticated(); mockUseSourceControl.mockReturnValue({ diff --git a/src/pages/function-list/FunctionsListPage.tsx b/src/pages/function-list/FunctionsListPage.tsx index 358a182..65d1587 100644 --- a/src/pages/function-list/FunctionsListPage.tsx +++ b/src/pages/function-list/FunctionsListPage.tsx @@ -182,25 +182,15 @@ function useFunctionListPage(): { const functionNames = useMemo(() => functionItems.map((item) => item.name), [functionItems]); - const { knativeServices, deployments, loaded: clusterLoaded } = useClusterService(functionNames); + const { functions: clusterFunctions, loaded: clusterLoaded } = useClusterService(functionNames); const functions = useMemo( () => functionItems.map((item) => { - const ksvc = knativeServices.find( - (s) => s.metadata?.labels?.['function.knative.dev/name'] === item.name, - ); - const latestRevision = ksvc?.status?.latestReadyRevisionName; - const deployment = latestRevision - ? deployments.find( - (d) => d.metadata?.labels?.['serving.knative.dev/revision'] === latestRevision, - ) - : deployments.find( - (d) => d.metadata?.labels?.['function.knative.dev/name'] === item.name, - ); - return ksvc && deployment ? enrichItem(item, ksvc, deployment) : item; + const info = clusterFunctions[item.name]; + return info?.deployment ? enrichItem(item, info.knativeService, info.deployment) : item; }), - [functionItems, knativeServices, deployments], + [functionItems, clusterFunctions], ); const loaded = reposLoaded && clusterLoaded; @@ -249,7 +239,7 @@ function enrichItem( status: deriveStatus(ksvc, deployment), url: ksvc.status?.url, replicas: deployment.status?.readyReplicas ?? 0, - deployment: ksvc, + knativeService: ksvc, }; } diff --git a/src/pages/function-list/components/FunctionTable.test.tsx b/src/pages/function-list/components/FunctionTable.test.tsx index 94df575..3a11238 100644 --- a/src/pages/function-list/components/FunctionTable.test.tsx +++ b/src/pages/function-list/components/FunctionTable.test.tsx @@ -24,14 +24,19 @@ vi.mock('@patternfly/react-icons', () => ({ TrashIcon: () => 'DeleteIcon', })); -const mockDeployment = { - apiVersion: 'apps/v1', - kind: 'Deployment', +const mockKnativeService = { + apiVersion: 'serving.knative.dev/v1', + kind: 'Service', metadata: { name: 'my-func', namespace: 'demo', labels: { 'function.knative.dev/name': 'my-func' }, }, + status: { + url: 'http://my-func.demo.svc', + latestReadyRevisionName: 'my-func-00001', + conditions: [{ type: 'Ready', status: 'True' }], + }, }; const mockFunctions: FunctionTableItem[] = [ @@ -42,7 +47,7 @@ const mockFunctions: FunctionTableItem[] = [ url: 'http://my-func.demo.svc', replicas: 1, namespace: 'demo', - deployment: mockDeployment, + knativeService: mockKnativeService, }, { name: 'idle-func', @@ -141,7 +146,7 @@ describe('FunctionTable', () => { await user.click(screen.getByRole('button', { name: 'Delete' })); expect(mockLauncher).toHaveBeenCalled(); expect(mockUseDeleteModal).toHaveBeenCalledWith( - mockDeployment, + mockKnativeService, undefined, undefined, 'Undeploy', diff --git a/src/pages/function-list/components/FunctionTable.tsx b/src/pages/function-list/components/FunctionTable.tsx index 993ee9a..7c1a8a5 100644 --- a/src/pages/function-list/components/FunctionTable.tsx +++ b/src/pages/function-list/components/FunctionTable.tsx @@ -19,7 +19,7 @@ export interface FunctionTableItem { url?: string; replicas: number; namespace: string; - deployment?: K8sResourceKind; + knativeService?: K8sResourceKind; } export type FunctionStatus = @@ -87,7 +87,7 @@ export function FunctionTable({ /> - + @@ -132,10 +132,10 @@ function UrlCell({ url }: { url?: string }) { ); } -function DeleteActionButton({ deployment }: { deployment?: K8sResourceKind }) { +function DeleteActionButton({ knativeService }: { knativeService?: K8sResourceKind }) { const { t } = useTranslation('plugin__console-functions-plugin'); const launchDelete = useDeleteModal( - deployment as K8sResourceKind, + knativeService as K8sResourceKind, undefined, undefined, t('Undeploy'), @@ -146,7 +146,7 @@ function DeleteActionButton({ deployment }: { deployment?: K8sResourceKind }) { variant="plain" aria-label={t('Delete')} icon={} - isDisabled={!deployment} + isDisabled={!knativeService} onClick={() => launchDelete()} /> );