ci: run dynamic-plugin e2e against RHDH on oinc - #348
Open
jasonmadigan wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe pull request adds dynamic-plugin RHDH E2E infrastructure, oinc/Dex cluster setup, structured Kubernetes error handling, Playwright reliability improvements, API-key annotation gating, RBAC updates, documentation, and multiline input sizing changes. ChangesDynamic RHDH E2E
Backend behaviour
E2E and UI validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Makefile
participant Oinc
participant RHDH
participant Playwright
GitHubActions->>Makefile: make dynamic-up
Makefile->>Oinc: provision cluster and Dex
Makefile->>RHDH: build and install dynamic plugins
GitHubActions->>Makefile: make e2e-specs
Makefile->>Playwright: run test suite
GitHubActions->>Makefile: make teardown
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)e2e-tests/rhdh/DockerfileTraceback (most recent call last): .github/workflows/ci.ymlTraceback (most recent call last): .github/workflows/e2e-dynamic.ymlTraceback (most recent call last):
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
|
This was referenced Jul 18, 2026
Closed
jasonmadigan
force-pushed
the
279-e2e-dynamic-plugin
branch
4 times, most recently
from
July 21, 2026 14:34
99ead7c to
5565d57
Compare
jasonmadigan
marked this pull request as ready for review
July 21, 2026 15:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
.github/workflows/e2e-dynamic.yml (1)
30-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence during checkout.
To improve the security posture, it is recommended to set
persist-credentials: falsewhen checking out the repository, unless the workflow explicitly needs to push changes back to the remote. This prevents theGITHUB_TOKENfrom being persisted in the local Git configuration.🛡️ Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-dynamic.yml around lines 30 - 31, Update the actions/checkout@v4 step to set persist-credentials to false, ensuring the workflow does not retain the GITHUB_TOKEN in the local Git configuration.Source: Linters/SAST tools
oinc/manifests/dex.yaml (1)
35-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider making the Dex root filesystem read-only.
The container already drops all capabilities and disables privilege escalation; adding
readOnlyRootFilesystem: truewould complete that hardening (flagged by Trivy KSV-0014). Given in-memory storage is documented, this is likely safe, but worth confirming Dex has no other write path (e.g./tmp) before adding it — mount anemptyDirat that path if it does.🔒️ Proposed hardening
securityContext: allowPrivilegeEscalation: false + readOnlyRootFilesystem: true capabilities: drop: ["ALL"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oinc/manifests/dex.yaml` around lines 35 - 38, Update the Dex container securityContext near allowPrivilegeEscalation and capabilities to set readOnlyRootFilesystem: true. Verify Dex’s runtime write paths, especially /tmp, and add an emptyDir mount for any required writable path while preserving the existing hardening settings.Source: Linters/SAST tools
oinc/setup-rhdh.sh (1)
99-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate npm integrity hashes before using them.
If
npm viewsucceeds but returns an empty string (e.g. version not yet published, ordist.integritymissing),FRONTEND_HASH/BACKEND_HASHend up empty and silently produce anintegrity: ""entry in the generated overlay — this only surfaces later as an opaque RHDH plugin-load failure rather than a clear script error.🛡️ Proposed fix
FRONTEND_HASH=$(npm view "${FRONTEND_PKG}" dist.integrity) BACKEND_HASH=$(npm view "${BACKEND_PKG}" dist.integrity) + if [ -z "${FRONTEND_HASH}" ] || [ -z "${BACKEND_HASH}" ]; then + log "error: failed to resolve npm package integrity hashes" + exit 1 + fi log "frontend: ${FRONTEND_HASH}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oinc/setup-rhdh.sh` around lines 99 - 105, Validate FRONTEND_HASH and BACKEND_HASH immediately after the npm view calls in the PLUGIN_SOURCE=npm branch, and fail with a clear error if either value is empty before logging or generating the overlay. Preserve the existing successful hash flow when both integrity values are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e-tests/playwright/e2e/kuadrant-happy-path.spec.ts`:
- Around line 85-92: Update the cleanup flow around context.close so the browser
context is always closed in a finally block, including when product cleanup
fails. Preserve the existing loud error logging, but rethrow the cleanup error
after logging so the test cannot pass while cleanup is incomplete.
- Around line 313-329: Track whether the Search textbox was found and filled in
the existing search flow. Only run the empty-state message assertion when
filtering occurred; when Search is absent, assert that the table rendered
instead, while preserving the scoped zero-row assertion for owner1’s API.
In `@e2e-tests/playwright/e2e/kuadrant-request-access.spec.ts`:
- Around line 471-480: Update the request route mock in the kuadrant access test
to return HTTP 422 and match the new Kubernetes mapper’s field-level error
response structure, replacing the legacy 500/raw-client payload while preserving
the invalid email scenario.
In `@e2e-tests/playwright/fixtures/test.ts`:
- Around line 19-23: Update the network failure watcher in the Playwright
fixture to register a requestfailed handler alongside the existing
response-status check. Record transport-level failures in failures, including
the failed request details, so DNS, connection, and reset errors are captured
even when no 5xx response exists; preserve the existing response handling.
In `@e2e-tests/playwright/utils/kuadrant-helpers.ts`:
- Around line 304-315: Update seedPendingApiKeyRequest to create its manual
browser context with the project use options, including baseURL and
ignoreHTTPSErrors, before calling dexQuickLogin. Preserve the existing page
setup, API key request, and context cleanup flow.
In `@kuadrant-dev-setup/rbac/rhdh-cluster-role.yaml`:
- Around line 27-32: Replace the cluster-wide secrets permissions in the
ClusterRole with namespace-scoped access, using a Role for each demo/target
namespace or resourceNames restrictions when API-key secret names are
predictable. Preserve only the get, create, and delete permissions required by
the API-key workflow, and leave the namespaces permissions unchanged.
In `@oinc/setup-cluster.sh`:
- Around line 47-54: Update the oinc create invocation in the cluster setup flow
to use only the supported --addons syntax for v0.3.1, removing
--kuadrant-devportal, --metallb-address-pool auto, and --gateway-api-gateway.
Preserve the requested addon installation through the supported addon install
flow after cluster creation, and keep the existing diagnostic and exit handling
on failure.
In `@plugins/kuadrant-backend/src/router.ts`:
- Around line 1816-1817: Update the inner catch around getSecret in the API key
secret retrieval flow so it handles only status 404 as “secret not found”;
rethrow 403, 429, 5xx, and all other failures to let sendKubernetesError map
them. Preserve the existing fallback response for genuinely missing secrets.
- Around line 153-165: Update the status handling in
plugins/kuadrant-backend/src/router.ts lines 153-165 to treat upstream status
503 like the existing 429 path: return HTTP 503 with the generic unavailable
message and set Retry-After from retryAfterSeconds when it is a positive number.
Update the corresponding test in plugins/kuadrant-backend/src/router.test.ts
lines 1634-1642 to expect 503 and cover the optional Retry-After header.
- Around line 470-475: Replace raw errorMessage responses in the fallback
branches with generic, operation-specific messages: API-product creation at
plugins/kuadrant-backend/src/router.ts lines 470-475, API-product update at
lines 754-756, Secret creation at lines 898-900, Secret deletion at lines
930-932, and API-key request creation at lines 1028-1033. Preserve
sendKubernetesError handling while ensuring these declined-status fallbacks
never expose K8sApiError.message.
---
Nitpick comments:
In @.github/workflows/e2e-dynamic.yml:
- Around line 30-31: Update the actions/checkout@v4 step to set
persist-credentials to false, ensuring the workflow does not retain the
GITHUB_TOKEN in the local Git configuration.
In `@oinc/manifests/dex.yaml`:
- Around line 35-38: Update the Dex container securityContext near
allowPrivilegeEscalation and capabilities to set readOnlyRootFilesystem: true.
Verify Dex’s runtime write paths, especially /tmp, and add an emptyDir mount for
any required writable path while preserving the existing hardening settings.
In `@oinc/setup-rhdh.sh`:
- Around line 99-105: Validate FRONTEND_HASH and BACKEND_HASH immediately after
the npm view calls in the PLUGIN_SOURCE=npm branch, and fail with a clear error
if either value is empty before logging or generating the overlay. Preserve the
existing successful hash flow when both integrity values are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bbc71ba-f324-4c22-a04d-6f4893c34e44
📒 Files selected for processing (45)
.dockerignore.github/workflows/ci.yml.github/workflows/e2e-dynamic.ymlCLAUDE.mdMakefiledocs/ci.mddocs/e2e-testing.mddocs/kuadrant-resources.mddocs/oinc.mddocs/rbac-permissions.mde2e-tests/playwright/e2e/kuadrant-auth-schemes.spec.tse2e-tests/playwright/e2e/kuadrant-happy-path.spec.tse2e-tests/playwright/e2e/kuadrant-live-backend.spec.tse2e-tests/playwright/e2e/kuadrant-permissions-matrix.spec.tse2e-tests/playwright/e2e/kuadrant-plugin.spec.tse2e-tests/playwright/e2e/kuadrant-request-access.spec.tse2e-tests/playwright/e2e/kuadrant-skeleton-loaders.spec.tse2e-tests/playwright/e2e/smoke-test.spec.tse2e-tests/playwright/fixtures/test.tse2e-tests/playwright/utils/kuadrant-helpers.tse2e-tests/rhdh/Dockerfilee2e-tests/rhdh/wait-for-catalog.shkuadrant-dev-setup/Makefilekuadrant-dev-setup/README.mdkuadrant-dev-setup/dex/config.yamlkuadrant-dev-setup/rbac/rhdh-cluster-role.yamlkuadrant-dev-setup/rbac/rhdh-rbac.yamloinc/lib.shoinc/manifests/dex.yamloinc/manifests/rhdh-sa.yamloinc/setup-cluster.shoinc/setup-dex.shoinc/setup-rhdh.shoinc/setup.shoinc/teardown.shplugins/kuadrant-backend/src/k8s-client.tsplugins/kuadrant-backend/src/providers/APIProductEntityProvider.test.tsplugins/kuadrant-backend/src/providers/APIProductEntityProvider.tsplugins/kuadrant-backend/src/router.test.tsplugins/kuadrant-backend/src/router.tsplugins/kuadrant/src/components/CreateAPIProductDialog/CreateAPIProductDialog.tsxplugins/kuadrant/src/components/EditAPIKeyDialog/EditAPIKeyDialog.tsxplugins/kuadrant/src/components/EditAPIProductDialog/EditAPIProductDialog.tsxplugins/kuadrant/src/components/RequestAccessDialog/RequestAccessDialog.tsxplugins/kuadrant/src/components/SimpleRequestAccessDialog/SimpleRequestAccessDialog.tsx
💤 Files with no reviewable changes (3)
- oinc/lib.sh
- kuadrant-dev-setup/rbac/rhdh-rbac.yaml
- oinc/manifests/rhdh-sa.yaml
jasonmadigan
force-pushed
the
279-e2e-dynamic-plugin
branch
from
July 21, 2026 17:02
5565d57 to
b4be492
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/e2e-dynamic.yml (1)
30-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDisable credential persistence in checkout.
As a security best practice, consider setting
persist-credentials: falsewhen checking out the code, as this workflow does not need to push changes back to the repository.🛠️ Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-dynamic.yml around lines 30 - 31, Update the actions/checkout@v4 step to set persist-credentials to false, leaving the rest of the checkout configuration unchanged.Makefile (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccommodate macOS environments with GNU
grep.Based on learnings, GNU
grepis required on macOS instead of the BSD implementation. You might consider adding a fallback to useggrepif it is available, preventing potential compatibility issues for developers using macOS.🛠️ Proposed fix
- `@version`=$$(oinc version 2>/dev/null | grep -o 'v[0-9][^[:space:]]*' | head -1); \ + `@GREP`=$$(command -v ggrep >/dev/null 2>&1 && echo ggrep || echo grep); \ + version=$$(oinc version 2>/dev/null | $$GREP -o 'v[0-9][^[:space:]]*' | head -1); \ case "$$version" in \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 38 - 39, Update the version-detection command in the Makefile to use GNU grep via ggrep when that executable is available, while retaining grep as the fallback for environments where ggrep is unavailable. Preserve the existing oinc version parsing and case handling.Source: Learnings
oinc/setup-dex.sh (1)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccommodate macOS environments with GNU
sed.Based on learnings, GNU
sedis required on macOS instead of the BSD implementation. You may want to detect the environment and usegsedif available to prevent potential regular expression compatibility issues for developers using macOS.🛠️ Proposed fix
-sed -E "s|^issuer:[[:space:]].*|issuer: ${DEX_URL}|" \ +SED=$(command -v gsed >/dev/null 2>&1 && echo gsed || echo sed) +$SED -E "s|^issuer:[[:space:]].*|issuer: ${DEX_URL}|" \ "${DEX_CONFIG}" >"${rendered_config}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oinc/setup-dex.sh` around lines 25 - 26, Update the sed invocation in the setup script to select GNU sed (`gsed`) when running on macOS, while retaining the existing sed behavior elsewhere. Detect whether `gsed` is available and use it for the issuer replacement performed by the rendered configuration generation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/e2e-dynamic.yml:
- Around line 30-31: Update the actions/checkout@v4 step to set
persist-credentials to false, leaving the rest of the checkout configuration
unchanged.
In `@Makefile`:
- Around line 38-39: Update the version-detection command in the Makefile to use
GNU grep via ggrep when that executable is available, while retaining grep as
the fallback for environments where ggrep is unavailable. Preserve the existing
oinc version parsing and case handling.
In `@oinc/setup-dex.sh`:
- Around line 25-26: Update the sed invocation in the setup script to select GNU
sed (`gsed`) when running on macOS, while retaining the existing sed behavior
elsewhere. Detect whether `gsed` is available and use it for the issuer
replacement performed by the rendered configuration generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11e9335a-9140-462a-a60c-f522e92e84e8
📒 Files selected for processing (42)
.dockerignore.github/workflows/ci.yml.github/workflows/e2e-dynamic.ymlCLAUDE.mdMakefiledocs/ci.mddocs/e2e-testing.mddocs/oinc.mddocs/rbac-permissions.mde2e-tests/playwright/e2e/kuadrant-auth-schemes.spec.tse2e-tests/playwright/e2e/kuadrant-happy-path.spec.tse2e-tests/playwright/e2e/kuadrant-live-backend.spec.tse2e-tests/playwright/e2e/kuadrant-permissions-matrix.spec.tse2e-tests/playwright/e2e/kuadrant-plugin.spec.tse2e-tests/playwright/e2e/kuadrant-request-access.spec.tse2e-tests/playwright/e2e/kuadrant-skeleton-loaders.spec.tse2e-tests/playwright/e2e/smoke-test.spec.tse2e-tests/playwright/fixtures/test.tse2e-tests/playwright/utils/kuadrant-helpers.tse2e-tests/rhdh/Dockerfilee2e-tests/rhdh/wait-for-catalog.shkuadrant-dev-setup/Makefilekuadrant-dev-setup/README.mdkuadrant-dev-setup/dex/config.yamlkuadrant-dev-setup/rbac/rhdh-cluster-role.yamlkuadrant-dev-setup/rbac/rhdh-rbac.yamloinc/lib.shoinc/manifests/dex.yamloinc/manifests/rhdh-sa.yamloinc/setup-cluster.shoinc/setup-dex.shoinc/setup-rhdh.shoinc/setup.shoinc/teardown.shplugins/kuadrant-backend/src/k8s-client.tsplugins/kuadrant-backend/src/router.test.tsplugins/kuadrant-backend/src/router.tsplugins/kuadrant/src/components/CreateAPIProductDialog/CreateAPIProductDialog.tsxplugins/kuadrant/src/components/EditAPIKeyDialog/EditAPIKeyDialog.tsxplugins/kuadrant/src/components/EditAPIProductDialog/EditAPIProductDialog.tsxplugins/kuadrant/src/components/RequestAccessDialog/RequestAccessDialog.tsxplugins/kuadrant/src/components/SimpleRequestAccessDialog/SimpleRequestAccessDialog.tsx
💤 Files with no reviewable changes (3)
- kuadrant-dev-setup/rbac/rhdh-rbac.yaml
- oinc/manifests/rhdh-sa.yaml
- oinc/lib.sh
🚧 Files skipped from review as they are similar to previous changes (23)
- plugins/kuadrant/src/components/CreateAPIProductDialog/CreateAPIProductDialog.tsx
- plugins/kuadrant/src/components/RequestAccessDialog/RequestAccessDialog.tsx
- plugins/kuadrant/src/components/EditAPIKeyDialog/EditAPIKeyDialog.tsx
- plugins/kuadrant/src/components/SimpleRequestAccessDialog/SimpleRequestAccessDialog.tsx
- .github/workflows/ci.yml
- kuadrant-dev-setup/Makefile
- oinc/teardown.sh
- plugins/kuadrant/src/components/EditAPIProductDialog/EditAPIProductDialog.tsx
- e2e-tests/playwright/e2e/kuadrant-auth-schemes.spec.ts
- kuadrant-dev-setup/README.md
- e2e-tests/rhdh/wait-for-catalog.sh
- e2e-tests/playwright/e2e/kuadrant-plugin.spec.ts
- kuadrant-dev-setup/dex/config.yaml
- e2e-tests/playwright/e2e/kuadrant-live-backend.spec.ts
- docs/rbac-permissions.md
- e2e-tests/playwright/utils/kuadrant-helpers.ts
- CLAUDE.md
- e2e-tests/playwright/fixtures/test.ts
- plugins/kuadrant-backend/src/k8s-client.ts
- e2e-tests/playwright/e2e/kuadrant-happy-path.spec.ts
- oinc/setup-rhdh.sh
- plugins/kuadrant-backend/src/router.ts
- e2e-tests/playwright/e2e/kuadrant-permissions-matrix.spec.ts
RHDH's dynamic hasAnnotation condition checks key presence, so an annotation value of false still exposes the API Keys tab. Emit the annotation only for discovered API-key schemes and cover OIDC-only, mixed, and unresolved products. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
Keep Kubernetes status details through the client boundary so request errors, conflicts, missing objects, validation failures, and throttling reach callers accurately. Treat service-account RBAC and missing-CRD failures as backend faults instead of blaming the signed-in user. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
Run every Kuadrant spec through the same Dex personas, seed state instead of conditionally skipping assertions, and search paginated tables so repeated manual runs remain deterministic. Add live-backend coverage and fail tests on silent Kuadrant 5xx responses, page exceptions, and unexpected console errors. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
Use minRows for multiline fields so MUI no longer emits the deprecated rows warning caught by the e2e runtime guard. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
Keep the ClusterRole in one manifest and leave environment-specific service accounts and bindings in their own files. This gives the static and dynamic paths one permission set and includes the status, approval, request, and namespace access found while exercising the live backend. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
Keep the existing static e2e job as the required PR path and add a workflow_dispatch-only RHDH path for the exported plugins. Share the build and cluster phases with local Make targets, run the full suite, pin Kuadrant 1.5.1 with an override, and keep manual environments up for inspection. Use the existing Dex personas for every RHDH entry point, including yarn oinc:rhdh, while keeping the local Dex 2.45.1 pin direct. The baked and npm plugin modes now share the same RHDH configuration and canonical Kubernetes role. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
jasonmadigan
force-pushed
the
279-e2e-dynamic-plugin
branch
from
July 22, 2026 13:53
b4be492 to
2ec783d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/workflows/e2e-dynamic.yml (1)
30-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable persisted checkout credentials.
The job does not need Git credentials after checkout; avoid leaving the read token in the local Git config for repository-controlled build steps.
Proposed change
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-dynamic.yml around lines 30 - 31, Update the actions/checkout@v4 step to disable persisted Git credentials by setting its persist-credentials option to false, while leaving the existing checkout behavior unchanged.Source: Linters/SAST tools
oinc/manifests/dex.yaml (1)
35-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake Dex’s root filesystem read-only.
Dex only consumes the mounted configuration and template here; prevent writes outside explicitly mounted volumes.
Proposed change
securityContext: allowPrivilegeEscalation: false capabilities: drop: ["ALL"] + readOnlyRootFilesystem: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oinc/manifests/dex.yaml` around lines 35 - 38, Update Dex’s securityContext in the dex manifest to set the root filesystem as read-only, while preserving the existing privilege-escalation and capability restrictions.Source: Linters/SAST tools
plugins/kuadrant-backend/src/router.ts (2)
1146-1204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent with the new structured status check.
The outer catch at Line 1200 now uses
sendKubernetesError, but the inner catch (Lines 1170-1176, unchanged) still detects a missing consumer namespace viaerror.message?.includes('404') || error.statusCode === 404— a fragile string-match that the newkubernetesStatus(error)helper was built to replace. A false-positive match (e.g. an unrelated error whose message happens to contain "404") would silently return an empty list instead of surfacing the real failure.Consider swapping to
kubernetesStatus(error) === 404for consistency with the rest of this file's new error-mapping approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/kuadrant-backend/src/router.ts` around lines 1146 - 1204, Replace the fragile 404 detection in the inner catch of the /requests/my handler with kubernetesStatus(error) === 404. Preserve the existing empty-items response for a genuinely missing consumer namespace, and rethrow all other errors for the outer error handling.
155-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate 429/503 handling.
The 429 and 503 branches are identical apart from the
console.errortext: both extractretryAfterSeconds, set the sameRetry-Afterheader, and return the same 503 JSON body. Worth extracting a small shared helper to avoid the two copies drifting apart later.♻️ Proposed refactor
+ function sendTransientUnavailable(logPrefix: string, error: unknown): express.Response { + console.error(logPrefix, kubernetesMessage(error)); + const retryAfter = kubernetesDetails(error)?.retryAfterSeconds; + if (typeof retryAfter === 'number' && retryAfter > 0) { + res.setHeader('Retry-After', String(retryAfter)); + } + return res.status(503).json({ error: 'service temporarily unavailable' }); + } + if (status === 429) { - console.error('kubernetes is throttling this backend:', kubernetesMessage(error)); - const retryAfter = kubernetesDetails(error)?.retryAfterSeconds; - if (typeof retryAfter === 'number' && retryAfter > 0) { - res.setHeader('Retry-After', String(retryAfter)); - } - return res.status(503).json({ error: 'service temporarily unavailable' }); + return sendTransientUnavailable('kubernetes is throttling this backend:', error); } if (status === 503) { - console.error('kubernetes is unavailable:', kubernetesMessage(error)); - const retryAfter = kubernetesDetails(error)?.retryAfterSeconds; - if (typeof retryAfter === 'number' && retryAfter > 0) { - res.setHeader('Retry-After', String(retryAfter)); - } - return res.status(503).json({ error: 'service temporarily unavailable' }); + return sendTransientUnavailable('kubernetes is unavailable:', error); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/kuadrant-backend/src/router.ts` around lines 155 - 171, The 429 and 503 branches duplicate the same response handling. Extract the shared retry-after header and 503 JSON response logic into a small helper, then have both status branches retain only their distinct logging and delegate to that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/e2e-dynamic.yml:
- Around line 30-31: Update the actions/checkout@v4 step to disable persisted
Git credentials by setting its persist-credentials option to false, while
leaving the existing checkout behavior unchanged.
In `@oinc/manifests/dex.yaml`:
- Around line 35-38: Update Dex’s securityContext in the dex manifest to set the
root filesystem as read-only, while preserving the existing privilege-escalation
and capability restrictions.
In `@plugins/kuadrant-backend/src/router.ts`:
- Around line 1146-1204: Replace the fragile 404 detection in the inner catch of
the /requests/my handler with kubernetesStatus(error) === 404. Preserve the
existing empty-items response for a genuinely missing consumer namespace, and
rethrow all other errors for the outer error handling.
- Around line 155-171: The 429 and 503 branches duplicate the same response
handling. Extract the shared retry-after header and 503 JSON response logic into
a small helper, then have both status branches retain only their distinct
logging and delegate to that helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abf81a0f-a6ea-45bb-9542-78a3580a6f3b
📒 Files selected for processing (45)
.dockerignore.github/workflows/ci.yml.github/workflows/e2e-dynamic.ymlCLAUDE.mdMakefiledocs/ci.mddocs/e2e-testing.mddocs/kuadrant-resources.mddocs/oinc.mddocs/rbac-permissions.mde2e-tests/playwright/e2e/kuadrant-auth-schemes.spec.tse2e-tests/playwright/e2e/kuadrant-happy-path.spec.tse2e-tests/playwright/e2e/kuadrant-live-backend.spec.tse2e-tests/playwright/e2e/kuadrant-permissions-matrix.spec.tse2e-tests/playwright/e2e/kuadrant-plugin.spec.tse2e-tests/playwright/e2e/kuadrant-request-access.spec.tse2e-tests/playwright/e2e/kuadrant-skeleton-loaders.spec.tse2e-tests/playwright/e2e/smoke-test.spec.tse2e-tests/playwright/fixtures/test.tse2e-tests/playwright/utils/kuadrant-helpers.tse2e-tests/rhdh/Dockerfilee2e-tests/rhdh/wait-for-catalog.shkuadrant-dev-setup/Makefilekuadrant-dev-setup/README.mdkuadrant-dev-setup/dex/config.yamlkuadrant-dev-setup/rbac/rhdh-cluster-role.yamlkuadrant-dev-setup/rbac/rhdh-rbac.yamloinc/lib.shoinc/manifests/dex.yamloinc/manifests/rhdh-sa.yamloinc/setup-cluster.shoinc/setup-dex.shoinc/setup-rhdh.shoinc/setup.shoinc/teardown.shplugins/kuadrant-backend/src/k8s-client.tsplugins/kuadrant-backend/src/providers/APIProductEntityProvider.test.tsplugins/kuadrant-backend/src/providers/APIProductEntityProvider.tsplugins/kuadrant-backend/src/router.test.tsplugins/kuadrant-backend/src/router.tsplugins/kuadrant/src/components/CreateAPIProductDialog/CreateAPIProductDialog.tsxplugins/kuadrant/src/components/EditAPIKeyDialog/EditAPIKeyDialog.tsxplugins/kuadrant/src/components/EditAPIProductDialog/EditAPIProductDialog.tsxplugins/kuadrant/src/components/RequestAccessDialog/RequestAccessDialog.tsxplugins/kuadrant/src/components/SimpleRequestAccessDialog/SimpleRequestAccessDialog.tsx
💤 Files with no reviewable changes (3)
- oinc/lib.sh
- kuadrant-dev-setup/rbac/rhdh-rbac.yaml
- oinc/manifests/rhdh-sa.yaml
🚧 Files skipped from review as they are similar to previous changes (32)
- oinc/teardown.sh
- plugins/kuadrant/src/components/EditAPIKeyDialog/EditAPIKeyDialog.tsx
- e2e-tests/playwright/e2e/smoke-test.spec.ts
- e2e-tests/playwright/e2e/kuadrant-auth-schemes.spec.ts
- kuadrant-dev-setup/dex/config.yaml
- plugins/kuadrant/src/components/SimpleRequestAccessDialog/SimpleRequestAccessDialog.tsx
- e2e-tests/playwright/e2e/kuadrant-live-backend.spec.ts
- oinc/setup.sh
- plugins/kuadrant/src/components/CreateAPIProductDialog/CreateAPIProductDialog.tsx
- .dockerignore
- e2e-tests/playwright/e2e/kuadrant-skeleton-loaders.spec.ts
- plugins/kuadrant-backend/src/providers/APIProductEntityProvider.test.ts
- docs/kuadrant-resources.md
- plugins/kuadrant-backend/src/providers/APIProductEntityProvider.ts
- kuadrant-dev-setup/README.md
- plugins/kuadrant/src/components/RequestAccessDialog/RequestAccessDialog.tsx
- e2e-tests/rhdh/wait-for-catalog.sh
- kuadrant-dev-setup/Makefile
- .github/workflows/ci.yml
- plugins/kuadrant/src/components/EditAPIProductDialog/EditAPIProductDialog.tsx
- docs/e2e-testing.md
- e2e-tests/playwright/e2e/kuadrant-plugin.spec.ts
- e2e-tests/playwright/fixtures/test.ts
- e2e-tests/playwright/e2e/kuadrant-request-access.spec.ts
- docs/rbac-permissions.md
- oinc/setup-rhdh.sh
- CLAUDE.md
- plugins/kuadrant-backend/src/router.test.ts
- plugins/kuadrant-backend/src/k8s-client.ts
- e2e-tests/playwright/utils/kuadrant-helpers.ts
- e2e-tests/playwright/e2e/kuadrant-happy-path.spec.ts
- e2e-tests/playwright/e2e/kuadrant-permissions-matrix.spec.ts
emmaaroche
reviewed
Jul 30, 2026
|
|
||
| // the queue pages at 20 rows, so narrow to the seeded api first - | ||
| // otherwise a fresh request can sit off the visible page. | ||
| await page.getByRole("textbox", { name: "Search" }).fill(owner2Api); |
Member
There was a problem hiding this comment.
The approval-queue search fills at lines 364, 399, and 587 are missing the count() guard that narrowProductsTable uses. When the table is small enough not to render a search box, fill() will block until timeout.
// lines 364, 399, 587 — no guard:
await page.getByRole("textbox", { name: "Search" }).fill(owner2Api);
// narrowProductsTable does this:
const search = page.getByRole("textbox", { name: "Search" });
if (await search.count()) {
await search.fill(text);
}
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
2 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
Closes #279.
Adds an opt-in path for testing the Kuadrant plugins as RHDH dynamic plugins:
E2E (dynamic plugins)isworkflow_dispatchonly.The PR also includes fixes exercised by the dynamic environment: API-key annotation conditions, Kubernetes error propagation, live-backend E2E coverage, repeatable test state, and the deprecated MUI multiline field usage.
Local use
PLAYWRIGHT_ARGSis forwarded bymake e2e-specsfor focused or headed runs.Verification
make e2e-dynamiccompleted the build, deployment, full suite, and teardown successfully: 84 passed and one passed on retry.The cold-run retry is an upstream developer-portal-controller v0.2.1 auto-approval ordering race; it is not a plugin or RHDH failure.
The manual workflow becomes dispatchable once its workflow file exists on the default branch.
Summary by CodeRabbit