chore(e2e): run bulk-import tests with NFS - #3318
Conversation
Adds a bulk-import-app-next project. The namespace suffix is what makes
e2e-test-utils merge the NFS (app-next) secrets and the default app-auth /
app-integrations layers, so the project entry is the whole trigger.
No locator branching was needed, which is worth recording because acr and
tekton both required it. Their tab titles changed between the two shells
("Image Registry" to "ACR IMAGES", "CI" to "Tekton") because under NFS the
suite's Scalprum config is inert and the title comes from the plugin's own
blueprint. Here the two agree: the plugin's PageBlueprint declares
path: '/bulk-import' and title: 'Bulk import', which are exactly the values
the legacy lane gets from the Scalprum dynamicRoutes path and menuItem text.
So BULK_IMPORT_ROUTE and BULK_IMPORT_HEADING hold unchanged for both lanes.
The three Scalprum sections this workspace configures (translationResources,
appIcons, dynamicRoutes, all pinned module: Legacy) are simply not read under
NFS. They stay for the legacy lane.
Verified locally: run-e2e.sh --list discovers the project with the same 9
tests as the legacy lane, and tsc, eslint and prettier are clean. The lane
itself needs a cluster run.
Part of RHIDP-16293.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 6:22 PM UTC · Completed 6:29 PM UTC Commit: |
Review — chore(e2e): run bulk-import tests with NFSVerdict: approve · 5 files changed · E2E test infrastructure only SummaryThis PR adds a The substantive change beyond the project entry is the What was reviewed1. The old literal key The fix — 2. Appending 3. The old path — 4. NFS guardrail assertion (correctness — verified) if (isAppNext) {
expect(rhdh.deploymentConfig.useNewFrontendSystem).toBe(true);
}Only asserts the forward direction (app-next → NFS enabled), not the reverse, which is correct since 5. Documentation counts (verified)
6. Interaction with #3306 The PR body flags that #3306 (same workspace, No findings above thresholdNo correctness, security, or convention issues found. The change is well-scoped, follows established patterns, and the Previous runReview — approveScope: E2E test infrastructure — adds NFS (app-next) lane for the bulk-import workspace, fixes a What this PR does
Correctness
Existing workspace patternThe orchestrator spec ( No concerns
The change is well-scoped, well-documented, and consistent with the five prior NFS migrations. The 🤖 Reviewed with Claude Code Previous run (2)ReviewVerdict: approve Clean, well-scoped PR that adds the NFS ( What was reviewed
FindingsLow: Stale counts in The NFS triage sheet has several counts that become stale after this PR adds
The document carries its own "Verified against Notes for the reviewer
Previous run (3)Review of PR #3318 — chore(e2e): run bulk-import tests with NFSSummaryThis PR adds a Findings🔴
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
workspace-update
Adding the app-next project made this spec run in two projects, and its
setup was guarded by test.runOnce with a literal key. runOnce keys its flag
file by the string alone, in a directory shared by every project in the run:
const flagDir = path.join(os.tmpdir(), `playwright-once-${process.ppid}`);
const flagFile = path.join(flagDir, `${key}.done`);
So the first project's setup satisfied the second, and bulk-import-app-next
would have skipped configure() + deploy() entirely -- never deploying RHDH
into its own namespace, then failing on the first assertion with a missing
heading and nothing pointing at the cause.
The key is now scoped by namespace, which is what deploy() already does
internally (`deploy-${namespace}`); the outer literal key was preventing that
correct inner guard from ever being reached for the second project.
Verified against the real runOnce implementation: with the literal key only
one of the two projects runs its setup, with the scoped key both do.
None of the five already-migrated workspaces use runOnce, which is why no
earlier NFS migration surfaced this.
Part of RHIDP-16293.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 6:33 PM UTC · Completed 6:45 PM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
Four findings from the review passes, each verified before applying.
Skip the lane in nightly. Both bulk-import packages are in
default.packages.yaml, so e2e-test-utils' DPDY resolution rewrites them to
oci://<registry>/...:{{inherit}} in nightly mode and the lane would load
whatever build ships in the RHDH image rather than the artifact this repo
pins -- which is not known to expose the NFS entry point. app-defaults
already skips nightly for exactly this mechanism (RHIDP-15482) and
tech-radar scopes its skip to the app-next project the same way.
Assert the lane really is running NFS. Without it, a lane that silently
failed to enable the new frontend system would just re-run the legacy suite
and stay green -- a false pass on the only thing this lane adds. Only the
forward direction is asserted: useNewFrontendSystem is
`namespace.endsWith("-app-next") || USE_NEW_FRONTEND_SYSTEM === "true"`, so
a global flag may legally turn NFS on everywhere and the legacy lane must
not be constrained. A two-way assertion was proposed and rejected for that
reason; both guards were mutation-checked against the real config builder.
Add process.pid to the two generated repository names. Playwright cannot
share a worker between projects -- the project id is part of the worker
hash -- so the two lanes evaluate the module separately, but Date.now() is
millisecond resolution and the workers start together, so an identical name
is possible. Both lanes create real repositories in janus-qe and delete
them in afterAll, so a collision would have one lane deleting the repo the
other is still using. process.pid is distinct across concurrently-alive
workers; the project name is not usable here because catalogRepoName is
also the Backstage metadata.name, capped at 63 chars, and the app-next
variant is 62.
Correct the JSDoc attribution. The claim that no locator branching is
needed holds, but the H1 does not come from the blueprint title: the
blueprint sets noHeader: true, so the shell header is suppressed and the
heading comes from the plugin's own <Header title={t('page.title')}>. The
blueprint title drives the nav item. Rationale moved next to
BULK_IMPORT_HEADING, where a reader looking at the locator will find it,
and the config comment now records why the orchestrator lane deliberately
has no app-next counterpart.
Also refreshes the triage sheet's counts, which this PR's new project makes
stale: 46 projects -> 47, 6 app-next lanes -> 7, 40 legacy-only -> 39.
Part of RHIDP-16293.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Review · Commit: |
My earlier commit updated four numbers and missed three the reviewer caught: the svc class tally (9 -> 10, the new lane is svc like the legacy one it mirrors), the "29 of the 46 projects do not need OpenShift" prose (30 of 47), and the count in the §6 reproduce command's own comment. That last one mattered most: §6 exists so a reader can re-derive the numbers, so a stale comment there undercuts the whole section. Both figures now reproduce -- 47 projects, 7 app-next lanes. Part of RHIDP-16293. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I added that skip on a premise I never verified -- that the build nightly
resolves via {{inherit}} would not expose the NFS entry point. Checking the
repo's actual convention shows the skip is wrong, and topology settles it:
its frontend package is in default.packages.yaml exactly like this one, it
has an app-next lane, and it does not skip nightly.
The two workspaces that do skip have unrelated, verified causes.
app-defaults' packages are not in default.packages.yaml and not in the RHDH
image (RHIDP-15482). tech-radar is shadowed by a baked-in wrapper. Neither
condition holds for bulk-import, which ships in the image and resolves to a
real build.
What nightly actually does here is resolve to
oci://registry.access.redhat.com/rhdh/...:{{inherit}}, i.e. test the
productized plugin rather than the ghcr artifact this repo pins. For an NFS
lane that is the more useful signal, not a weaker one -- "does the plugin
customers get mount under the new frontend system" is the question the epic
is asking. Skipping it starved the nightly triage pipeline of exactly that
answer.
The comment now records why the skip should not come back, since the
{{inherit}} rewrite looks alarming until you know topology already runs
with it.
The NFS assertion added alongside the skip stays, and matters more now: it
is what stops a lane that failed to enable the new frontend system from
quietly re-running the legacy suite and passing.
Part of RHIDP-16293.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/publish |
|
🤖 Finished Review · ✅ Success · Started 7:35 PM UTC · Completed 7:40 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
Publish workflow has completed with success. Publishing process✅ Finished successfully. ✅ Published container images:
Backstage Compatibility Check✅ All workspaces are compatible with the target Backstage version ( No action required. Metadata Validation✅ All metadata files validated successfully. Running e2e tests |
|
✅ Smoke tests workflow passed. All plugins loaded successfully. |
❌ Failed E2E Tests -
|
The app-next lane failed one test on `clickButton("Self-service")`. That
button lives in RHDH's global header, and packages/app-next ships no global
header, so the path does not exist in that shell.
Replaced the two shell-chrome clicks with a direct navigation to
/catalog-import -- the same page those clicks land on. The test's subject is
that a catalog-imported repo shows up in the Catalog and not in Bulk import;
how the import page was reached is incidental, and CatalogImportPO only
needs to be on the page (it fills input[name="url"] and clicks Analyze).
This removes a dependency on chrome that differs between the two shells
rather than branching on the lane, so it is one path for both.
What the run established, beyond this one failure:
The lane deployed and 6 of its 7 runnable tests passed, so the runOnce key
fix worked -- without it the lane would have had no deployment at all. The
NFS assertion passed, so the new frontend system really was active. The
"Bulk import" nav item and the /bulk-import route both resolved under NFS,
which is what the config comment claimed. And the failure screenshot shows
the generated repo name ending in the worker pid, so that fix is live too.
It also settles the nightly question the earlier revert was about: the
plugin does mount under NFS, so skipping the lane in nightly would have
suppressed a working signal.
The other failure in that run is not this PR's: bulk-import-orchestrator
never became ready because the orchestrator backend plugin crashes at
startup with "TypeError: Cannot read properties of undefined (reading
'id')". That is tracked in #3083 with PR #3086 open for it, and this diff
touches no orchestrator file.
Part of RHIDP-16293.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
🤖 Finished Review · ✅ Success · Started 8:07 PM UTC · Completed 8:12 PM UTC Commit: |
fullsend-ai-review
Bot
added
ready-for-merge
github-actions
Bot
added
the
mandatory-workspace
|
/test e2e-ocp-helm |
❌ Failed E2E Tests -
|
|
/publish |
|
Publish workflow has completed with success. Publishing process✅ Finished successfully. ✅ Published container images:
Backstage Compatibility Check✅ All workspaces are compatible with the target Backstage version ( No action required. Metadata Validation✅ All metadata files validated successfully. Running e2e tests |
|
✅ Smoke tests workflow passed. All plugins loaded successfully. |
❌ Failed E2E Tests -
|
|
@gustavolira: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
🤖 Finished Retro · ✅ Success · Started 10:09 PM UTC · Completed 10:24 PM UTC Commit: |
Retro: PR #3318 —
|
3 participants
Closes RHIDP-16293.
Adds a
bulk-import-app-nextproject. The-app-nextnamespace suffix is what makese2e-test-utilsmerge the NFS secrets and the defaultapp-auth/app-integrationslayers.Correction to an earlier revision of this description: it said the project entry was the whole change, as it was for
analytics(#2900) andacr(#2889). Not here — running the spec in a second project exposed atest.runOncekey collision that would have stopped the new lane from deploying at all. See below.Why no locator branching was needed
Worth recording, because the two most recent migrations both needed it.
acrhad to branch"Image Registry"→"ACR IMAGES"andtekton"CI"→"Tekton", because under NFS the suite's Scalprum config is inert and the title comes from the plugin's own blueprint instead.Here the two shells agree. From
rhdh-plugins/workspaces/bulk-import/plugins/bulk-import/src/index.tsx:and the legacy lane's Scalprum config sets
dynamicRoutes.path: /bulk-importwithmenuItem.text: Bulk import. So the suite'sBULK_IMPORT_ROUTE(/bulk-import) andBULK_IMPORT_HEADING(Bulk import) hold for both lanes unchanged.The three Scalprum sections this workspace configures —
translationResources,appIcons,dynamicRoutes, all pinnedmodule: Legacy— are simply not read under NFS. They stay for the legacy lane.The one real change beyond the config entry
test.runOncekeys its flag file by the string alone, in a directory shared by every project in the run:This spec guarded its setup with a literal key, so once the
bulk-importproject had run it,bulk-import-app-nextfound the flag and skippedconfigure()+deploy()entirely. The new lane would never have deployed RHDH into its own namespace, and the first assertion would have failed on a missing heading with nothing pointing at the cause.The key is now scoped by namespace — which is what
deploy()already does internally (deploy-${namespace}). The outer literal key was preventing that correct inner guard from ever being reached for the second project.Proven against the real
runOnceimplementation rather than by inspection: with the literal key, one of the two projects runs its setup; with the scoped key, both do.Worth noting for the remaining migrations: none of the five already-migrated workspaces use
runOnce, which is why no earlier NFS migration surfaced this. Any workspace that does — and runs one spec in both shells — has the same trap.Verified locally
./run-e2e.sh -w bulk-import --listdiscoversbulk-import-app-nextwith the same 9 tests as the legacy lane (22 total across 3 projects).tsc --noEmit,eslintandprettier --checkclean on the changed file.Needs a cluster run
/test e2e-ocp-helm— the lane itself cannot be verified locally. Two things I would watch in that run:app-next. The suite installs from a local dist path (./dynamic-plugins/dist/red-hat-developer-hub-backstage-plugin-bulk-import), i.e. the copy baked into the RHDH image rather than this repo's OCI artifact.packages/app-nextships only catalog, scaffolder, search and user-settings plus the dynamic loader, so bulk-import has to arrive through the loader. If it does not, the failure mode is a clean boot with an empty page — no error, exit 0 — so the first failing assertion will be the heading, not anything diagnostic.dynamic-plugins.default.yamlpre-configures bulk-import withmodule: AlphafortranslationResourcesand no module elsewhere, and the deep merge stripsmodule: Legacy, breaking scalprum's resolution ofBulkImportPage. It addsdisablePluginsto work around that on the legacy lane. Underapp-nextnone of those module names are read, so the two changes should be independent — but they touch the same workspace and fix(#3303): disable chart default bulk-import plugin to preserve module: Legacy #3306 should probably land first to avoid a confusing merge.Not done here
The plugin's NFS extension has no test anywhere —
bulkImportPageis built withPageBlueprint.makeand nothing asserts that it attaches or what it declares. That belongs upstream inrhdh-plugins, not in this PR, and is tracked in the analysis on the ticket. It is also what would turn the failure mode in point 1 above from "the heading is missing" into "this blueprint did not attach".🤖 Generated with Claude Code