Skip to content

refactor: decouple resource pairing into hook#11

Open
matejvasek wants to merge 2 commits into
openshift:masterfrom
matejvasek:SRVOCF-841-fix-res-coupling
Open

refactor: decouple resource pairing into hook#11
matejvasek wants to merge 2 commits into
openshift:masterfrom
matejvasek:SRVOCF-841-fix-res-coupling

Conversation

@matejvasek

Copy link
Copy Markdown

Summary

  • Introduce ClusterFunction paired type in useClusterService, moving ksvc-to-deployment matching logic out of FunctionsListPage into the hook
  • Simplify page enrichment to a name-based lookup on pre-paired ClusterFunction objects
  • Rename FunctionTableItem.deployment to mainResource (it holds the ksvc, not a Deployment)

Test plan

  • yarn ci passes (147 tests, lint clean, build succeeds)
  • Verify FunctionCreatePage still works (only uses generateKubeconfig, unchanged)
  • Manual smoke test on cluster: list page shows correct status, replicas, URL for deployed functions

🤖 Generated with Claude Code

matejvasek and others added 2 commits June 25, 2026 13:20
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
useClusterService returned Knative Services and Deployments as
separate arrays, forcing FunctionsListPage to do ksvc-to-deployment
matching (revision label lookup with function name fallback). This
is a cluster-domain concern that belongs in the hook.

Introduce ClusterFunction type pairing both resources by name, move
the matching logic into the hook behind useMemo, and simplify the
page to a name-based lookup. Rename FunctionTableItem.deployment to
mainResource since it holds the ksvc (for the delete modal), not a
Deployment.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matej Vašek <matejvasek@gmail.com>
@openshift-ci openshift-ci Bot requested review from TheRealJon and logonoff June 25, 2026 12:19
@openshift-ci

openshift-ci Bot commented Jun 25, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2026
@openshift-ci

openshift-ci Bot commented Jun 25, 2026

Copy link
Copy Markdown

@matejvasek: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@matejvasek matejvasek requested review from Cragsmann, dsimansk and twoGiants and removed request for TheRealJon and logonoff June 25, 2026 12:53
const { t } = useTranslation('plugin__console-functions-plugin');
const launchDelete = useDeleteModal(
deployment as K8sResourceKind,
mainResource as K8sResourceKind,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typecasting should be avoided here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but this is pre-existing issue. I would prefer to fix it in some other PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I am not sure about clean fix. Following is not much better IMO.

diff --git a/src/pages/function-list/components/FunctionTable.tsx b/src/pages/function-list/components/FunctionTable.tsx
index c0c4b07..3a92d39 100644
--- a/src/pages/function-list/components/FunctionTable.tsx
+++ b/src/pages/function-list/components/FunctionTable.tsx
@@ -88,7 +88,11 @@ export function FunctionTable({
                   />
                 </ActionListItem>
                 <ActionListItem>
-                  <DeleteActionButton mainResource={fn.mainResource} />
+                  {fn.mainResource ? (
+                    <DeleteActionButton mainResource={fn.mainResource} />
+                  ) : (
+                    <DisabledDeleteButton />
+                  )}
                 </ActionListItem>
               </ActionList>
             </Td>
@@ -133,21 +137,20 @@ function UrlCell({ url }: { url?: string }) {
   );
 }
 
-function DeleteActionButton({ mainResource }: { mainResource?: K8sResourceKind }) {
+function DisabledDeleteButton() {
   const { t } = useTranslation('plugin__console-functions-plugin');
-  const launchDelete = useDeleteModal(
-    mainResource as K8sResourceKind,
-    undefined,
-    undefined,
-    t('Undeploy'),
-  );
+  return <Button variant="plain" aria-label={t('Delete')} icon={<TrashIcon />} isDisabled />;
+}
+
+function DeleteActionButton({ mainResource }: { mainResource: K8sResourceKind }) {
+  const { t } = useTranslation('plugin__console-functions-plugin');
+  const launchDelete = useDeleteModal(mainResource, undefined, undefined, t('Undeploy'));
 
   return (
     <Button
       variant="plain"
       aria-label={t('Delete')}
       icon={<TrashIcon />}
-      isDisabled={!mainResource}
       onClick={() => launchDelete()}
     />
   );

@matejvasek matejvasek Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and mainResource ?? {} changes thruthines

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and mainResource ?? {} changes thruthines

But maybe it's valid approach, I don't know.

Comment on lines +59 to +63
const functions = useMemo(() => {
const safeKnSvcs = knLoaded ? (knSvcs ?? []) : [];
const safeDeps = depLoaded ? (deps ?? []) : [];
return pairResources(safeKnSvcs, safeDeps);
}, [knSvcs, knLoaded, deps, depLoaded]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const functions = useMemo(() => {
const safeKnSvcs = knLoaded ? (knSvcs ?? []) : [];
const safeDeps = depLoaded ? (deps ?? []) : [];
return pairResources(safeKnSvcs, safeDeps);
}, [knSvcs, knLoaded, deps, depLoaded]);
const functions = useMemo(
() => pairResources(
knLoaded ? (knSvcs ?? []) : [],
depLoaded ? (deps ?? []) : [],
),
[knSvcs, knLoaded, deps, depLoaded],
);

@Cragsmann Cragsmann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit-picks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants