diff --git a/docs/claude-progress.txt b/docs/claude-progress.txt index 0fb5e5b..cd0891b 100644 --- a/docs/claude-progress.txt +++ b/docs/claude-progress.txt @@ -1,6 +1,23 @@ # Claude Progress Log # Newest entries first. Agents: append your entry at the top after the header. +--- +## 2026-06-25 | Session: SRVOCF-841 fix resource coupling +Worked on: Decouple ksvc/deployment pairing from FunctionsListPage into useClusterService +Completed: +- Introduced ClusterFunction interface (name, optional knativeService, optional deployment) +- Moved ksvc-to-deployment pairing logic from FunctionsListPage into useClusterService with useMemo +- Pairing matches by revision label with fallback to function name label +- Changed ClusterService return type from separate arrays to functions: ClusterFunction[] +- Simplified FunctionsListPage enrichment to a name-based lookup +- Renamed FunctionTableItem.deployment to mainResource (it holds the ksvc, not a Deployment) +- Fixed FunctionTable.test.tsx fixture to use Knative Service shape instead of Deployment +- Added 4 new hook-level pairing tests (revision match, fallback, multiple revisions, empty) +- Removed duplicate "picks latest revision" test from FunctionsListPage (now tested in hook) +- All 147 tests passing, lint clean, build succeeds +Left off: Changes staged, ready for commit. +Blockers: None + --- ## 2026-06-24 | Session: Fix pre-commit hook error handling Worked on: Fix pre-commit hook to fail on lint/type-check/go vet errors diff --git a/docs/features.json b/docs/features.json index 6e3e25b..7991a22 100644 --- a/docs/features.json +++ b/docs/features.json @@ -202,5 +202,18 @@ "Document usage in README or script header" ], "passes": true + }, + { + "category": "technical", + "description": "useClusterService: fix resource coupling and improve tests. Knative Services and Deployments are co-present via OwnerReferences, clarify this invariant and simplify to a merged return type.", + "steps": [ + "Merge separate knativeServices/deployments arrays into a single paired return type (e.g. ClusterFunction[]) that pairs each ksvc with its deployment", + "Move ksvc-to-deployment matching logic from FunctionsListPage into useClusterService", + "Rename FunctionTableItem.deployment to knativeService (it stores the ksvc, not the deployment)", + "Update useClusterService tests to reflect the co-presence invariant (no test should have a ksvc without a deployment or vice versa)", + "Update FunctionsListPage to consume the simplified return type", + "All tests pass, no broken imports" + ], + "passes": false } ] diff --git a/src/common/services/cluster/useClusterService.test.tsx b/src/common/services/cluster/useClusterService.test.tsx index 3cd3ab7..6e8d823 100644 --- a/src/common/services/cluster/useClusterService.test.tsx +++ b/src/common/services/cluster/useClusterService.test.tsx @@ -1,5 +1,5 @@ import { render, screen } from '@testing-library/react'; -import { useClusterService } from './useClusterService'; +import { useClusterService, ClusterFunction } from './useClusterService'; const mockUseK8sWatchResource = vi.fn(); @@ -38,22 +38,18 @@ const mockDeployment = { }; function TestConsumer({ functionNames = [] }: { functionNames?: string[] }) { - const { knativeServices, deployments, loaded, error } = useClusterService(functionNames); + const { functions, loaded, error } = useClusterService(functionNames); return ( <> {String(loaded)} {String(error)} - {knativeServices.length} - {deployments.length} - {knativeServices.map((s) => ( - - {s.metadata?.name} - - ))} - {deployments.map((d) => ( - - {d.metadata?.name} - + {functions.length} + {functions.map((fn: ClusterFunction) => ( +
+ {fn.name} + {String(!!fn.knativeService)} + {String(!!fn.deployment)} +
))} ); @@ -71,14 +67,13 @@ 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', () => { mockUseK8sWatchResource .mockReturnValueOnce([[mockKsvc], true, null]) - .mockReturnValueOnce([[], true, null]); + .mockReturnValueOnce([[mockDeployment], true, null]); render(); @@ -91,13 +86,11 @@ describe('useClusterService', () => { ], }, }); - expect(screen.getByTestId('ksvc-count')).toHaveTextContent('1'); - expect(screen.getByTestId('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 +104,9 @@ describe('useClusterService', () => { ], }, }); - expect(screen.getByTestId('dep-count')).toHaveTextContent('1'); - expect(screen.getByTestId('deployment')).toHaveTextContent('my-func-00001-deployment'); }); - it('returns empty arrays when not loaded', () => { + it('returns empty functions array when not loaded', () => { mockUseK8sWatchResource .mockReturnValueOnce([[], false, null]) .mockReturnValueOnce([[], false, null]); @@ -123,7 +114,95 @@ 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('pairs ksvc with deployment by revision label', () => { + mockUseK8sWatchResource + .mockReturnValueOnce([[mockKsvc], true, null]) + .mockReturnValueOnce([[mockDeployment], true, null]); + + render(); + + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('fn-name')).toHaveTextContent('my-func'); + expect(screen.getByTestId('has-ksvc')).toHaveTextContent('true'); + expect(screen.getByTestId('has-dep')).toHaveTextContent('true'); + }); + + it('falls back to function name label when no latestReadyRevisionName', () => { + const ksvcNoRevision = { + ...mockKsvc, + status: { ...mockKsvc.status, latestReadyRevisionName: undefined }, + }; + const depByName = { + ...mockDeployment, + metadata: { + ...mockDeployment.metadata, + labels: { 'function.knative.dev/name': 'my-func' }, + }, + }; + + mockUseK8sWatchResource + .mockReturnValueOnce([[ksvcNoRevision], true, null]) + .mockReturnValueOnce([[depByName], true, null]); + + render(); + + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('has-dep')).toHaveTextContent('true'); + }); + + it('picks latest revision deployment when multiple revisions exist', () => { + const ksvcV2 = { + ...mockKsvc, + status: { ...mockKsvc.status, latestReadyRevisionName: 'my-func-00002' }, + }; + const depV1 = { + ...mockDeployment, + metadata: { + ...mockDeployment.metadata, + name: 'my-func-00001-deployment', + labels: { + 'function.knative.dev/name': 'my-func', + 'serving.knative.dev/revision': 'my-func-00001', + }, + }, + spec: { replicas: 0 }, + status: { readyReplicas: 0 }, + }; + const depV2 = { + ...mockDeployment, + metadata: { + ...mockDeployment.metadata, + name: 'my-func-00002-deployment', + labels: { + 'function.knative.dev/name': 'my-func', + 'serving.knative.dev/revision': 'my-func-00002', + }, + }, + spec: { replicas: 1 }, + status: { readyReplicas: 1 }, + }; + + mockUseK8sWatchResource + .mockReturnValueOnce([[ksvcV2], true, null]) + .mockReturnValueOnce([[depV1, depV2], true, null]); + + render(); + + expect(screen.getByTestId('fn-count')).toHaveTextContent('1'); + expect(screen.getByTestId('has-dep')).toHaveTextContent('true'); + }); + + it('returns empty functions array when no resources match', () => { + mockUseK8sWatchResource + .mockReturnValueOnce([[], true, null]) + .mockReturnValueOnce([[], true, null]); + + render(); + + expect(screen.getByTestId('loaded')).toHaveTextContent('true'); + expect(screen.getByTestId('fn-count')).toHaveTextContent('0'); }); }); diff --git a/src/common/services/cluster/useClusterService.ts b/src/common/services/cluster/useClusterService.ts index e1eeeb5..8df88e8 100644 --- a/src/common/services/cluster/useClusterService.ts +++ b/src/common/services/cluster/useClusterService.ts @@ -5,10 +5,16 @@ 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 ClusterFunction { + name: string; + knativeService?: K8sResourceKind; + deployment?: K8sResourceKind; +} interface ClusterService { - knativeServices: K8sResourceKind[]; - deployments: K8sResourceKind[]; + functions: ClusterFunction[]; loaded: boolean; error: unknown; generateKubeconfig: (namespace: string) => Promise; @@ -50,11 +56,32 @@ export function useClusterService(functionNames: string[] = []): ClusterService const [knSvcs, knLoaded, knError] = useK8sWatchResource(knSvcConfig); const [deps, depLoaded, depError] = useK8sWatchResource(depConfig); + const functions = useMemo(() => { + const safeKnSvcs = knLoaded ? (knSvcs ?? []) : []; + const safeDeps = depLoaded ? (deps ?? []) : []; + return pairResources(safeKnSvcs, safeDeps); + }, [knSvcs, knLoaded, deps, depLoaded]); + return { - knativeServices: knLoaded ? (knSvcs ?? []) : [], - deployments: depLoaded ? (deps ?? []) : [], + functions, loaded: knLoaded && depLoaded, error: knError || depError, generateKubeconfig: instance.generateKubeconfig.bind(instance), }; } + +function pairResources( + knSvcs: K8sResourceKind[], + deployments: K8sResourceKind[], +): ClusterFunction[] { + return knSvcs.map((ksvc) => { + const name = ksvc.metadata?.labels?.[FUNCTION_NAME_LABEL] ?? ksvc.metadata?.name ?? ''; + 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); + + return { name, knativeService: ksvc, deployment: deployment ?? undefined }; + }); +} diff --git a/src/pages/function-list/FunctionsListPage.test.tsx b/src/pages/function-list/FunctionsListPage.test.tsx index c0812b0..4103b8d 100644 --- a/src/pages/function-list/FunctionsListPage.test.tsx +++ b/src/pages/function-list/FunctionsListPage.test.tsx @@ -51,21 +51,27 @@ const GITHUB_API = 'https://api.github.com'; function clusterData( overrides: Partial<{ - knativeServices: unknown[]; - deployments: unknown[]; + functions: unknown[]; loaded: boolean; error: unknown; }> = {}, ) { return { - knativeServices: [], - deployments: [], + functions: [], loaded: true, error: null, ...overrides, }; } +function clusterFunction( + name: string, + ksvc: ReturnType, + dep: ReturnType, +) { + return { name, knativeService: ksvc, deployment: dep }; +} + function renderAuthenticated() { sessionStorage.setItem(PAT_KEY, 'ghp_test'); } @@ -199,8 +205,13 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-func', 'name: my-func\nruntime: go\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 1, 1)], + functions: [ + clusterFunction( + 'my-func', + ksvcFixture('my-func', 'True'), + deploymentFixture('my-func', 1, 1), + ), + ], }), ); @@ -311,8 +322,13 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-func', 'name: my-func\nruntime: go\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 1, 1)], + functions: [ + clusterFunction( + 'my-func', + ksvcFixture('my-func', 'True'), + deploymentFixture('my-func', 1, 1), + ), + ], }), ); @@ -333,8 +349,13 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-func', 'name: my-func\nruntime: go\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'True')], - deployments: [deploymentFixture('my-func', 0, 0)], + functions: [ + clusterFunction( + 'my-func', + ksvcFixture('my-func', 'True'), + deploymentFixture('my-func', 0, 0), + ), + ], }), ); @@ -354,8 +375,13 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-func', 'name: my-func\nruntime: go\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'Unknown')], - deployments: [deploymentFixture('my-func', 1, 0)], + functions: [ + clusterFunction( + 'my-func', + ksvcFixture('my-func', 'Unknown'), + deploymentFixture('my-func', 1, 0), + ), + ], }), ); @@ -374,30 +400,12 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-func', 'name: my-func\nruntime: go\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-func', 'False')], - deployments: [deploymentFixture('my-func', 0, 0)], - }), - ); - - render( - - - , - ); - - expect(await screen.findByTestId('fn-status')).toHaveTextContent('Error'); - }); - - it('picks latest revision deployment when multiple revisions exist', async () => { - renderAuthenticated(); - setupReposHandler([repoFixture('my-func')]); - setupFuncYamlHandler('my-func', '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'), + functions: [ + clusterFunction( + 'my-func', + ksvcFixture('my-func', 'False'), + deploymentFixture('my-func', 0, 0), + ), ], }), ); @@ -408,8 +416,7 @@ describe('FunctionsListPage', () => { , ); - expect(await screen.findByTestId('fn-status')).toHaveTextContent('Running'); - expect(screen.getByTestId('fn-replicas')).toHaveTextContent('1'); + expect(await screen.findByTestId('fn-status')).toHaveTextContent('Error'); }); it('passes function names to useClusterService', async () => { @@ -566,8 +573,13 @@ describe('FunctionsListPage', () => { setupFuncYamlHandler('my-repo', 'name: my-function\nruntime: node\nnamespace: demo\n'); mockUseClusterService.mockReturnValue( clusterData({ - knativeServices: [ksvcFixture('my-function', 'True')], - deployments: [deploymentFixture('my-function', 1, 1)], + functions: [ + clusterFunction( + 'my-function', + ksvcFixture('my-function', 'True'), + deploymentFixture('my-function', 1, 1), + ), + ], }), ); diff --git a/src/pages/function-list/FunctionsListPage.tsx b/src/pages/function-list/FunctionsListPage.tsx index 3c26166..eace4dd 100644 --- a/src/pages/function-list/FunctionsListPage.tsx +++ b/src/pages/function-list/FunctionsListPage.tsx @@ -25,7 +25,10 @@ import { ForgeConnectionContext, ForgeConnectionProvider, } from '../../common/context/ForgeConnectionProvider'; -import { useClusterService } from '../../common/services/cluster/useClusterService'; +import { + ClusterFunction, + useClusterService, +} from '../../common/services/cluster/useClusterService'; import { SourceControlService } from '../../common/services/source-control/SourceControlService'; import { useSourceControlService } from '../../common/services/source-control/useSourceControlService'; import { errorMessage, parseFuncYaml } from '../../common/utils/utils'; @@ -182,25 +185,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 cf = clusterFunctions.find((f) => f.name === item.name); + return cf ? enrichItem(item, cf) : item; }), - [functionItems, knativeServices, deployments], + [functionItems, clusterFunctions], ); const loaded = reposLoaded && clusterLoaded; @@ -252,17 +245,16 @@ function newItem( }; } -function enrichItem( - item: FunctionTableItem, - ksvc: K8sResourceKind, - deployment: K8sResourceKind, -): FunctionTableItem { +function enrichItem(item: FunctionTableItem, cf: ClusterFunction): FunctionTableItem { + const { knativeService: ksvc, deployment } = cf; + if (!ksvc || !deployment) return item; + return { ...item, status: deriveStatus(ksvc, deployment), url: ksvc.status?.url, replicas: deployment.status?.readyReplicas ?? 0, - deployment: ksvc, + mainResource: ksvc, }; } diff --git a/src/pages/function-list/components/FunctionTable.test.tsx b/src/pages/function-list/components/FunctionTable.test.tsx index c6cf855..2a5fab8 100644 --- a/src/pages/function-list/components/FunctionTable.test.tsx +++ b/src/pages/function-list/components/FunctionTable.test.tsx @@ -24,9 +24,9 @@ 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', @@ -43,7 +43,7 @@ const mockFunctions: FunctionTableItem[] = [ url: 'http://my-func.demo.svc', replicas: 1, namespace: 'demo', - deployment: mockDeployment, + mainResource: mockKnativeService, }, { name: 'idle-func', @@ -165,7 +165,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 702b7c6..c0c4b07 100644 --- a/src/pages/function-list/components/FunctionTable.tsx +++ b/src/pages/function-list/components/FunctionTable.tsx @@ -20,7 +20,7 @@ export interface FunctionTableItem { url?: string; replicas: number; namespace: string; - deployment?: K8sResourceKind; + mainResource?: K8sResourceKind; } export type FunctionStatus = @@ -88,7 +88,7 @@ export function FunctionTable({ /> - + @@ -133,10 +133,10 @@ function UrlCell({ url }: { url?: string }) { ); } -function DeleteActionButton({ deployment }: { deployment?: K8sResourceKind }) { +function DeleteActionButton({ mainResource }: { mainResource?: K8sResourceKind }) { const { t } = useTranslation('plugin__console-functions-plugin'); const launchDelete = useDeleteModal( - deployment as K8sResourceKind, + mainResource as K8sResourceKind, undefined, undefined, t('Undeploy'), @@ -147,7 +147,7 @@ function DeleteActionButton({ deployment }: { deployment?: K8sResourceKind }) { variant="plain" aria-label={t('Delete')} icon={} - isDisabled={!deployment} + isDisabled={!mainResource} onClick={() => launchDelete()} /> );