feat(e2e): cluster-free plugin sanity check for the catalog index (RHIDP-13508) - #4967
Open
gustavolira wants to merge 18 commits into
Open
feat(e2e): cluster-free plugin sanity check for the catalog index (RHIDP-13508)#4967gustavolira wants to merge 18 commits into
gustavolira wants to merge 18 commits into
Conversation
PR Summary by Qodofeat(e2e): add Playwright plugin sanity-check test Description
Diagram
High-Level Assessment
Files changed (3)
|
Contributor
|
The container image build workflow finished with status: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4967 +/- ##
==========================================
- Coverage 63.69% 59.96% -3.74%
==========================================
Files 123 111 -12
Lines 2424 2198 -226
Branches 573 551 -22
==========================================
- Hits 1544 1318 -226
- Misses 878 879 +1
+ Partials 2 1 -1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
Context used 1.
|
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Jun 18, 2026
Implements review findings from PR redhat-developer#4967 code review: **Correctness fixes:** - Fix test.beforeAll signature to use test.info() (follows smoke-test.spec.ts pattern) - Remove unused manifestPath variable in plugin-dynamic-loading.spec.ts **Code organization:** - Extract DEFAULT_PACKAGES_PATH constant to avoid duplication - Move CONFIG_OVERRIDES to module scope for better reusability - Remove unused catalog-index-parser.ts (can be recreated when needed) **Type safety:** - Import and use BackendFeature type for require() calls instead of any - Improves type safety when loading plugin modules **Developer experience:** - Improve console.warn message to explain impact of missing _nodeModulePaths All changes are low-risk refactorings that improve code quality without changing behavior. Type checking passes with no errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
Contributor
Member
Author
|
/test ? |
Member
Author
|
/test e2e-ocp-helm-nightly |
1 similar comment
Member
Author
|
/test e2e-ocp-helm-nightly |
Member
Author
|
/test e2e-ocp-helm-nightly |
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Jun 19, 2026
Implements review findings from PR redhat-developer#4967 code review: **Correctness fixes:** - Fix test.beforeAll signature to use test.info() (follows smoke-test.spec.ts pattern) - Remove unused manifestPath variable in plugin-dynamic-loading.spec.ts **Code organization:** - Extract DEFAULT_PACKAGES_PATH constant to avoid duplication - Move CONFIG_OVERRIDES to module scope for better reusability - Remove unused catalog-index-parser.ts (can be recreated when needed) **Type safety:** - Import and use BackendFeature type for require() calls instead of any - Improves type safety when loading plugin modules **Developer experience:** - Improve console.warn message to explain impact of missing _nodeModulePaths All changes are low-risk refactorings that improve code quality without changing behavior. Type checking passes with no errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Contributor
Member
Author
|
/test e2e-ocp-helm-nightly |
The check is cluster-free, but it only ran inside handle_ocp_nightly, so it was gated on a cluster being provisioned for unrelated steps and could only report at nightly cadence. Move it to the E2E Cluster-free GitHub Actions workflow as a `plugin-sanity` job on a plain runner. Every package the index declares resolves anonymously (install-dynamic-plugins rewrites registry.access.redhat.com/rhdh/ to quay.io/rhdh/), so no pull secret and no cluster are involved. - drops testing::run_plugin_sanity_check and its ocp-nightly.sh call site; the cluster-based sanity-plugins deployment and its startup-failure report stay - adds a schedule trigger, since the catalog index is built outside this repo and changes without a commit here, plus a workflow_dispatch `catalog_index_image` input replacing the Gangway override for RC verification - extracts the shared setup into .github/actions/setup-cluster-free-harness so the two cluster-free jobs do not duplicate it - extracts the Backstage startup-failure log shapes into local-harness/filter-plugin-startup-failures.sh, so the cluster and cluster-free paths cannot drift apart Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes from a shell/CI, TypeScript and duplication review pass.
Blocking:
- install jq in the cluster-free setup action: catalog-index-refs.sh parses the
index manifest with it and the Playwright image does not ship it, so the
populate step failed on every run (verified by running the pinned image)
- the plugin sanity config no longer adopts a backend already on :7007. The spec
compares dynamic-plugins-root against what the RUNNING backend loaded, so a
leftover legacy-local backend made it pass while validating nothing
- report startup failures only when the sanity step itself failed; on an earlier
failure the log does not exist and the step printed a false all-clear
- classify plugins by role.startsWith("frontend") instead of "not backend", so a
node-library/web-library role is no longer misfiled as frontend and then failed
for shipping no bundle
- exclude the spec from the any-test project, which collects every spec
Workflow:
- keep the e2e job on its original triggers; paths filters do not gate the new
schedule/dispatch events
- put the event name in the concurrency group so the nightly cannot cancel a
push run, and add timeout-minutes to both jobs
- trigger on changes to the composite action, shape-check the dispatch input
before it reaches GITHUB_OUTPUT, and upload the backend log with the report
- say plainly that schedule only runs from the default branch
Shell:
- restore the "JUnit results file not found" diagnostic lost in the refactor,
on stderr since the function's stdout is captured
- assign the layer digests before the loop so a missing jq fails loudly instead
of being reported as a missing default.yaml
- bound the pod log reads with timeout, and drop the single-use filter wrapper
- move the JUnit helpers below run_tests, whose doc block they had orphaned
Naming, duplication and coverage:
- loadManifest -> scanInstalledPlugins (it scans; there is no manifest file),
PluginError -> FrontendBundleError, drop the unused role/version plumbing
- share dynamicPluginsRoot and the reporter block via local-harness-servers.ts
- reuse parseRefreshToken instead of re-narrowing the session payload inline
- rename the unit test to plugin-loader.test.ts and cover the four previously
untested exports (49 tests, up from 22)
- yarn plugin-sanity -> yarn e2e:plugin-sanity, matching e2e:legacy-local
- fix the wrong "both flavors share catalog-index-refs.sh" claim and the stale
alternative that said the index needs a source build
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first run of the plugin-sanity job failed on "Resolve the catalog index
image" with `Syntax error: "(" unexpected`. Inside a job `container:` the
default shell is `sh` — /usr/bin/dash in the Playwright image — not the `bash`
GitHub uses on the runner host.
Three steps depended on bash: the `[[ ]]`/`=~` in the resolve step, the
`set -o pipefail` guarding the tee pipeline (dash: "Illegal option -o
pipefail"), and the `[[ -s ]]` in the startup-failure report. Declaring the
shell once at the workflow level covers all of them and stops the trap
recurring; verified against the pinned image that dash rejects and bash accepts
these constructs.
Also write the run log inside the checkout: pairing a repo-relative path with
/tmp made the upload-artifact root `/`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # workspace dependencies; e2e-tests has its own lockfile. | ||
| - name: Install dependencies (root) | ||
| shell: bash | ||
| run: yarn install |
…that boot The first working run of the plugin-sanity job showed the excludes never took effect: the generated install config `includes` the index's own plugin list, so omitting a package from the `plugins:` section does not stop it installing. Both "excluded" orchestrator packages were installed and aborted the backend. catalog-index-refs.sh grows an --excluded mode, and populate-catalog-index.sh now writes those refs into the config as `disabled: true` rather than dropping them. The excludes file documented the old, wrong premise; it now says how the exclusion is applied. The same run named five more plugins that abort the backend purely on missing startup config, so add dummy entries for them next to the existing ldapOrg and microsoftGraphOrg ones: keycloakOrg, githubOrg, gitlab (used by two modules) and the github catalog provider. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
Second pass on the dummy config, driven by what the run reported rather than by
guesswork: the shapes now mirror the pluginConfig blocks the catalog index
actually declares.
- the index declares two gitlab provider ids, `default` and `orgProvider`, both
keyed on ${GITLAB_HOST}; each validates its own host, so both need an entry
- githubOrg resolves its githubUrl against the configured integrations, so add
an integrations.github entry for github.invalid; without it the provider
aborts with "There is no GitHub integration that matches"
- give githubOrg the id/orgs the index sets, so the shapes line up
Everything stays on .invalid, so no request can leave the runner.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
The previous commit's integrations.github entry caused a regression: a host
other than github.com must configure apiBaseUrl and rawBaseUrl explicitly, and
without them core.urlReader failed to instantiate, taking catalog, scaffolder,
tech-radar and techdocs down with it.
Add both base URLs, and give gitlab the integration entry it needs for the same
reason the github provider did ("No gitlab integration found that matches
host"). Still all .invalid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
…ay on Sonar findings on the plugin sanity check. Applied: the two length assertions in the classification test now use toHaveLength, which reports the actual length on failure instead of just the compared number. Not applied, deliberately: "Lifecycle scripts are enabled by default in Yarn v2+" on the root install. The harness backend runs on better-sqlite3 with an in-memory database (app-config.local-e2e.yaml), and its native binding is fetched by an install script — disabling scripts would stop the backend booting in both cluster-free jobs. The line is also the pre-existing one this PR moved into the composite action, and matches the plain `yarn install` in pr.yaml, bash-e2e-lint.yaml, e2e-tests-lint.yaml and rulesync-check.yaml. Recorded at the step so the constraint is not rediscovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
rostalan
reviewed
Jul 29, 2026
zdrapela
reviewed
Jul 29, 2026
Findings from rostalan and zdrapela, both e2e-tests approvers.
Stop shipping a test-only config in the product image. Containerfile:206 does
`COPY app-config*.yaml ./` and .dockerignore excludes *.local-e2e.yaml but had
nothing for app-config.plugin-sanity.yaml, so a file whose header reads
"test-only. Never layer this into a production config" was being built into the
image. Moved under e2e-tests/local-harness/, which .dockerignore already
excludes wholesale, rather than adding another ignore entry.
Give cluster-free-only specs their own home. testDir is set once in
playwright.config.ts and no project overrides it, so the spec had to be ignored
in all four catch-all projects, added over two commits because one was missed.
Moved to playwright/cluster-free/ (precedent: playwright/blocked/) and deleted
the four testIgnore entries; verified showcase, showcase-k8s, showcase-operator
and any-test no longer list it.
Split the single test into one per concern, per CONTRIBUTING.MD §6 II ("each
test has to test one and only one thing"). It was one test with six assertions
over four concerns, so any failure named the whole thing.
Give the breadcrumb invariant a single owner: requireCatalogIndexExpectation
throws with the populate command, and the global setup and spec both use it
instead of each re-deriving the check and the message.
Add a per-frontend-plugin HTTP assertion. The scalprum backend logs a warning
and skips a plugin whose dist-scalprum is unusable, yet that plugin still shows
up in loaded-plugins, so neither existing check saw it. The spec now asserts
every dist-scalprum plugin is served at /api/scalprum/plugins. The auth dance
and both endpoints moved behind a DynamicPluginsApi service object.
Workflow: plugin-sanity timeout 45 -> 20 minutes (runs take 5-7), and the
resolved index digest is printed beside the tag for traceability.
Unit tests 49 -> 61, covering the new helpers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The index added three packages the harness cannot pull: homepage, intelligent-assistant and intelligent-assistant-backend. `skopeo inspect --no-creds` reports "unauthorized" for the whole repository on quay.io and on registry.access.redhat.com alike, so the populate step failed outright on a plain runner. These are tag-style refs (:1.11.0--x.y.z); all 31 digest-pinned refs the index declares still resolve anonymously through the CLI's quay.io fallback, so this is about publication, not the digest-lookup trap the file already warns about. I checked every declared package rather than the one the log named, so this should not need another round trip. The excludes file documented a single category (cannot initialize); it now documents both, and each new entry says how to re-check and to delete it once the image is public. Excluding homepage costs real coverage, so that is called out rather than buried. Also corrects the README, which claimed every index package resolves anonymously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
Both frontend tests assert that a filtered list is empty, which also holds when the list was empty to begin with - so they would pass while validating nothing. The scalprum one is the real risk: it only considers plugins that ship a dist-scalprum bundle. Assert both sets are non-empty first, the same guard the installed-count assertion already provides for the index as a whole. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
The container image build workflow finished with status: |
Two review passes over the plugin sanity check.
Corrects an inverted name. dist/remoteEntry.js was called LEGACY_BUNDLE and
described as the legacy format, but dist/remoteEntry.js + mf-manifest.json is
the module-federation layout used by New Frontend System plugins, and
dist-scalprum is the older RHDH-specific one that scalprum-backend serves. The
constant is now MF_REMOTE_ENTRY and the three comments saying "legacy" are
reworded, since they taught the opposite of the truth.
Layering and naming:
- plugin-loader.ts scanned an install directory and never loaded a plugin, and
it also held two HTTP response parsers whose only caller is the API module.
Renamed to installed-plugins.ts (filesystem only) and the parsers moved to
support/api/dynamic-plugins-api.ts.
- DynamicPluginsApi -> RhdhDynamicPluginsApi, matching RhdhRbacApi and the other
classes in support/api/.
- scalprumPluginNames was a static method on a class whose other method needs
construction. It is now a plain fetchScalprumPluginNames function: the
endpoint belongs to a different backend plugin and needs no credentials, so
that test no longer pays for a guest auth round trip.
- requireDynamicPluginsPopulated's runCommand parameter receives a yarn script
name, not a command; renamed to yarnScript.
Duplication:
- isUnknownArray was a second copy of the narrowing guards.ts already owns;
guards.ts now exports the guard and toUnknownArray delegates to it.
- one readJsonFile and one prop/stringProp pair replace two hand-rolled JSON
reads and three copies of the "object has a string property" narrowing.
- the two report-then-assert blocks in the spec collapse into missingFrom().
- repeated path literals are named constants, which also fixes the same artifact
being addressed as join(p, "a", "b") in one place and has("a/b") in another.
- validateFrontendBundle's "missing package.json" branch was unreachable, since
scanInstalledPlugins skips directories without one.
Correctness:
- readCatalogIndexExpectation now rejects a zero, negative or fractional count.
A populate run that miscounts to 0 used to be accepted and surfaced later as a
confusing count mismatch instead of at its cause.
- the breadcrumb test can no longer skip in CI; only a local run without
CATALOG_INDEX_IMAGE skips it.
Coverage 61 -> 75 tests, driven by mutation testing that found five surviving
mutants: catalogIndexPopulateCommand had no test at all, the package.json name
fallback and the non-directory skip were unpinned, and the digest test never
exercised the key/value re-join it claimed to. RhdhDynamicPluginsApi had no unit
test; it now has four against a fake APIRequestContext, verified to fail when
the Authorization header is dropped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…elector Shell and CI review of the plugin sanity check. app-config.plugin-sanity.yaml pointed microsoftGraphOrg at the real https://graph.microsoft.com/v1.0, the only routable host in a file whose own header says the check never talks to these services. With a 15s initialDelay the provider fired on every run, and @azure/identity authenticates against `authority`, which defaults to login.microsoftonline.com, so the nightly was making outbound auth calls to Microsoft with a bogus tenant from a shared runner IP. Both are now .invalid. testing::report_plugin_startup_failures read the global RELEASE_NAME instead of taking it as an argument (SC2153), unlike every sibling in that file. In a namespace with a different release the selector matches nothing and the function printed "No dynamic-plugin startup failures found", which is the reassuring message for exactly the silent miss it exists to catch. It now takes the release name and warns when no pods matched. The recorded catalog index digest was not the digest that was validated: the skopeo call omitted the --override-os/--override-arch that catalog-index-refs.sh uses, so it returned the child manifest rather than the tag's manifest list, and off linux/amd64 it failed outright into "unresolved". Both the script header and the excludes file documented a `!plugin-name` ref suffix the published index does not emit. Following that doc when adding an exclusion produces a pattern that can never match, so the plugin installs anyway and nothing reports it. Also: name the excludes file when it is the reason no packages are left, and correct the defaults.run.shell comment, which described the symptom rather than what requesting bash actually changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Member
Author
|
/agentic-review |
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
4 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
Plugin sanity checking for the RHDH catalog index, implementing RHIDP-13508.
Validates that every plugin the catalog index declares actually loads in the real RHDH backend, with no cluster deployment and no product image. Building on the cluster-free harness (#5005), Playwright boots
packages/backendfrom source — the product's own Backstage line anddynamicPluginsFeatureLoader— and the spec compares the installed set against/api/dynamic-plugins-info/loaded-plugins, failing with the exact list of installed-but-not-loaded plugins.The index is selected by
CATALOG_INDEX_IMAGE(branch-matched nightly index by default; RC index via Gangway--catalog-index-image).How it works
local-harness/catalog-index-refs.sh— extractsdynamic-plugins.default.yamlfrom the index image and lists every declared package (uncommented entries only), applyingplugin-sanity-excludes.txt. Layers are walked top-down: OCI layers are ordered base-first, so an index rebuilt as an overlay would otherwise yield a stale copy of the file. A missing excludes file is fatal rather than silently disabling every exclusion.local-harness/populate-catalog-index.sh— wraps those refs into an install config (the index ships packagesdisabled: true, so a plainincludeswould install nothing) and installs via the sharedpopulate.sh, which gained an optional config argument.playwright.plugin-sanity.config.ts— dedicated request-only config; no browser, no frontend dev server.Guarding against a green-but-empty run
The spec would otherwise only assert installed ⊆ loaded, which stays green when the install silently underran. So:
populate-catalog-index.shrecords the index image and resolvedoci://package count indynamic-plugins-root/.catalog-index-refsCATALOG_INDEX_IMAGEplugin-sanitygets its ownglobalSetuprequiring that breadcrumb, so adynamic-plugins-rootleft over from the curatedpopulate.shno longer satisfies the guardFailure reporting
A broken plugin takes the whole backend down by design, so the culprit is made obvious:
testing::report_plugin_startup_failuresscans pod logs (current + previous containers, so CrashLoopBackOff is covered) and prints aPLUGIN STARTUP FAILURESblock saved as an artifact; the cluster-free wrapper greps its own run log for the same signatures.CI integration
Runs in
handle_ocp_nightly→run_sanity_plugins_checkviatesting::run_plugin_sanity_check, which follows thetesting::run_testsconventions (test_run_tracker,save_overall_result, gzipped JUnit toSHARED_DIR, report artifacts) so Slack reporting includes it like any other run. It does not alter the existing cluster sanity-plugins deployment.Local runs
Verification
:next— 9 declared packages resolved, backend booted, all loaded.e2e-tests/unit/catalog-index-expectation.test.ts).oxlint,oxfmt,tsc,vitest,shellcheckand.ciprettier clean.Scope note
An earlier revision of this PR also made the cluster sanity-plugins deployment derive its enabled plugin set from the same index, replacing the hand-maintained list. That change has a very different risk profile — it swaps a working curated set for a fully derived one, and a single bad plugin fails the whole
showcase-sanity-pluginsproject — so it has been split into a follow-up PR and is not included here. Branch:gustavolira:sanity-plugins-cluster-dynamic.Jira