🚧 SRVOCF-822: Function list namespace scoping - #155
Draft
twoGiants wants to merge 14 commits into
Draft
Conversation
The function list was sourced only from GitHub repositories, so functions deployed to the cluster without a discoverable repo never appeared in the UI. /func/list now returns the union of repo-discovered and cluster-deployed functions, tagging each with a source (repo or cluster) and keeping the repo source when a function exists in both. Cluster-only functions have no repository to edit, so the frontend disables their Edit action and explains why on hover. Empty runtime now renders as a dash, matching url and namespace. Function querying is split out of the cluster client, which is now provisioning-only (RBAC, service accounts, tokens). A new functions package exposes a growable Client facade over knative/func, and a new kube package owns the shared REST config (host resolution, TLS, JSON content type, request timeout) that both cluster and functions build on. This keeps each cluster concern focused and avoids a god-client. knative/func is bumped to pick up knative/func#4010 so cluster-deployed functions report their runtime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
func.yaml parsing pulled in go.yaml.in/yaml/v3 as a second direct YAML library, while sigs.k8s.io/yaml was already a dependency. Switch the func.yaml unmarshal to sigs.k8s.io/yaml and retag the struct with json tags (which sigs.k8s.io/yaml honors), removing the redundant direct dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The scaffold and functions packages were both thin facades over knative/func: scaffold generated a new function's source and CI files, while functions queried deployed functions. Keeping them apart split knative/func usage across two boundaries for no real gain. Fold scaffold into functions so a single package owns all knative/func usage, while keeping its two responsibilities distinct: - client.go holds the cluster-connected facade (Client, NewClient, List); deploy/undeploy/describe are expected next, hence an interface. - scaffold.go holds offline generation (Generate, ScaffoldConfig, EnvVar), which writes to a temp dir and returns scm.FileEntry blobs. It never touches the cluster, so it stays a package function rather than a Client method. Renames for clarity now that both concerns share the namespace: functions.New -> functions.NewClient, functions.Config -> functions.ScaffoldConfig. The knative/func import is aliased fn since the package is itself named functions. docs/ARCHITECTURE.md updated to drop the scaffold package and record why offline scaffolding stays separate from the cluster CRUD facade. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FunctionsListPage.test.tsx mocked FunctionTable and UserAvatar, hiding integration issues between the page and its children. Replace both with the real components. FunctionTable required adding SDK status stubs and useDeleteModal to the mock, and updating assertions from data-testid to content-based queries (getByText, getByRole). UserAvatar's PAT modal auto-opens for unauthenticated users, setting aria-hidden on page content and breaking role queries. Fix by extracting enableReconnect as an optional prop so tests can disable the modal. Also: rename isConnectedToForge to isAuthenticated, extract BACKEND_API to testing/setup.ts, remove unused pages/index.html. Issue SRVOCF-822 Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
The list page tests mocked useCluster entirely, hiding the hook's internals (status derivation, resource pairing, label selectors) behind pre-computed return values. The useCluster tests used a TestConsumer component to render hook output into DOM elements, then asserted via data-testid queries. Replace the useCluster mock with a shared useK8sWatchResourceStub that both useCluster.test.tsx and FunctionsListPage.test.tsx use. Tests now set up raw K8s resource fixtures (Knative Services, Deployments) and let the real useCluster derive status, replicas, and URL. Shape mismatches between the hook and its consumers are caught at test time instead of hidden by a mock. Replace TestConsumer with renderHook from @testing-library/react. Hook return values are asserted directly on result.current instead of through DOM nodes. Extract shared test infrastructure into src/common/testing: authFake (session auth helpers), constants (BACKEND_API), mswServer (default MSW handlers + server), and the K8s watch resource stub with fixture builders. Move FUNCTION_NAME_LABEL and REVISION_LABEL from useCluster.ts to types.ts so both production code and test stubs can reference them without circular imports. Issue SRVOCF-822 Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
The list and edit pages showed all functions regardless of the active namespace. Now both read the active namespace from the OCP project selector via useActiveNamespace and scope the backend API call and cluster watches accordingly. listFunctions sends ?namespace=X for a specific namespace or ?all=true when the user selects "All Namespaces". useCluster maps #ALL_NS# to undefined (cluster-wide watch) and specific namespaces to scoped watches. Watch config construction is extracted into helper functions for readability. The useActiveNamespace stub is now reactive via useSyncExternalStore so tests can switch namespaces mid-test with act() and verify re-fetches. The functionsClient stub gained namespace filtering and a wait option for testing the refresh spinner lifecycle. Renamed consoleSdkStubs to sdkTestDoubles. Issue SRVOCF-822 Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@twoGiants: This pull request references SRVOCF-822 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
The list endpoint previously aborted with 502 whenever the repo listing failed, so an SCM outage hid healthy cluster functions entirely. Cluster failures, by contrast, were silently swallowed, leaving the two sources treated asymmetrically. Query both sources concurrently and treat a single-source failure as non-fatal: return whatever the surviving source found. Only when both sources fail do we report 502, since there is genuinely nothing to return. An invalid SCM token still short-circuits to 401. Also serialize empty results as [] instead of null, which a nil slice produced on the both-empty and failure paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add namespace scoping to /func/list. The handler now requires a non-empty namespace query parameter and returns 400 otherwise, so a developer-perspective request can never trigger an unscoped list that would leak functions from other namespaces via the repo source (which has no RBAC awareness). When a specific namespace is requested, both repo-discovered functions (by their func.yaml namespace) and cluster-discovered functions (via the lister) are scoped to it. all=true returns every function unfiltered and cluster-wide, taking precedence over namespace. Issue SRVOCF-822 Signed-off-by: Pedro Almeida <pealmeid@redhat.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The function table always rendered a Namespace column, which was redundant when the list was already scoped to a single active namespace. It only adds information in the all-namespaces view. Drive the column from a showNamespace prop, derived from isAllNamespacesKey on the active namespace, so the column and its cells appear only when functions span multiple namespaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
|
The list scopes off Was that intentional? SRVOCF-822 doesn't mention how the user is meant to change scope from this page. Adding the SDK's |
Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
When switching from all-namespaces to a specific namespace, the namespace column disappeared from the old list before the new data arrived. This happened because reposLoaded stayed true across namespace changes, so the table kept rendering stale items with the updated showNamespace value. Replace the reposLoaded boolean state with loadedForNamespace, which tracks the namespace the current items were fetched for. reposLoaded is now derived: it is false whenever the current namespace differs from the loaded one, so the spinner shows instead of the stale table during transitions. Co-Authored-By: Pedro Almeida <pealmeid@redhat.com>
The function list is fetched cluster-wide, so the same function name can appear in more than one namespace. The list page keyed cluster status and React rows by name alone, so a deployment in one namespace bled its status onto a same-named function in another, and duplicate names produced colliding React keys. Key the cluster function map, status enrichment, and table rows by namespace and name, and match a ksvc to a deployment only within its own namespace (revision and function-name labels can repeat across namespaces). The delete e2e test deployed the preseeded function into a namespace that did not match its func.yaml, which under the new per-namespace dedup surfaced two rows. Point it (and the shared seed helpers) at the namespace declared in func.yaml via a PRESEEDED_FUNC_NAMESPACE constant. Signed-off-by: Pedro Almeida <pealmeid@redhat.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Remove the exported clusterFunctionKey helper from useCluster.ts; it belonged to the map data structure, not the hook. Inlined the template literal at both call sites with a short comment. - Reorder filterByNamespace in list.go to sit closer to its callers. - Update PRESEEDED_FUNC_NAMESPACE in e2e constants to test-namespace. Co-Authored-By: Pedro Almeida <pealmeid@redhat.com>
Add an e2e test suite for namespace scoping (namespace-scoping.test.ts) covering all-namespaces view, per-namespace filtering, and switching back to all namespaces. Add namespace selector helpers to navigation.ts using the OCP console's data-test attribute pattern. Add a unit regression test that verifies the spinner replaces stale rows immediately when the namespace changes, before the new list loads. Fix useCluster.test.tsx map key lookups to use namespace/name format after the clusterFunctionKey change in cherry-picked commits. Add NamespaceBar to the SDK mock in FunctionsListPage.test.tsx. Fix global-setup.ts to seed the preseeded function into PRESEEDED_FUNC_NAMESPACE instead of the default namespace. Co-Authored-By: Pedro Almeida <pealmeid@redhat.com>
twoGiants
commented
Aug 26, 2026
twoGiants
commented
Aug 26, 2026
| const [reposLoaded, setReposLoaded] = useState(!isAuthenticated); | ||
| // Tracks which namespace the current functionItems were loaded for. | ||
| // When namespace changes, this diverges and reposLoaded derives to false automatically. | ||
| const [loadedForNamespace, setLoadedForNamespace] = useState<string | null>( |
Author
There was a problem hiding this comment.
I'd like to use the same pattern here as for the connectionId reset. It's easier to understand for me.
Use namespaceLoaded as a plain boolean and add a prevNamespace state to detect namespace changes, same pattern the hook already uses for connectionId:
const [namespaceLoaded, setNamespaceLoaded] = useState(false);
const [prevNamespace, setPrevNamespace] = useState(namespace);
if (namespace !== prevNamespace) {
setPrevNamespace(namespace);
setFunctionItems([]);
setNamespaceLoaded(false);
setError('');
}Then: const loaded = namespaceLoaded && clusterLoaded;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes SRVOCF-XXX
Checklist
docs/ARCHITECTURE.md(if there are relevant changes to our layered architecture)