Conversation
…versions and upgrade - Add e2e-tests/plugin-version-management.test.ts verifying plugin check-versions and upgrade lifecycle - Test version mismatch detection, remediation output, and --json structured output - Test non-destructive --dry-run and --skip-install upgrade modes - Test full upgrade with package manager install and lockfile synchronization - Test multi-step version upgrade lifecycle across releases (1.8 -> 2.0 -> 2.1) - Test dynamic plugin export of an upgraded plugin fixture - Test offline and air-gapped auditing and upgrading via --manifest-file and RHDH_OFFLINE=true - Register versions:lint alias for check-versions in src/commands/index.ts - Enrich runCommand Error with stdout, stderr, code, and signal properties Assisted-By: opencode Signed-off-by: Stan Lewis <gashcrumb@gmail.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…xport-build - Move runExpectingFailure into e2e-tests/support/plugin-export-build.ts - Share runExpectingFailure across plugin-dev.test.ts and plugin-version-management.test.ts - Resolve SonarCloud code duplication quality gate failure Assisted-By: opencode Signed-off-by: Stan Lewis <gashcrumb@gmail.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Review · Commit: |
- Disable immutable installs (YARN_ENABLE_IMMUTABLE_INSTALLS=false) during upgrade's yarn install step - Allow Yarn Berry to update yarn.lock when running under CI=true or PR workflows - Pass YARN_ENABLE_IMMUTABLE_INSTALLS=false in e2e version management upgrade test Assisted-By: opencode Signed-off-by: Stan Lewis <gashcrumb@gmail.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:46 PM UTC · Completed 7:11 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $4.32 |
|
Risk Assessment: moderate (2/5) DetailsPrimarily a test-addition PR with two small production changes; large blast-radius classification is the dominant risk signal but is offset by high test file ratio, no protected or security-sensitive paths, and a well-scoped, easily reversible change set. |
ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
e2e-tests/plugin-version-management.test.ts(file-level): Line 469 · [low] test-correctness
The offlineDir fixture in beforeAll does not include a backstage.json file. syncBackstageJson silently skips when the file is absent, leaving the backstage.json sync path untested in offline mode. No assertion fails, but the gap is silent.
Suggested fix: Add a backstage.json to the offline fixture and assert parsedUpgrade.updatedFiles includes backstage.json after upgrade.
| runCommand, | ||
| runExpectingFailure, | ||
| RunCommandOptions, | ||
| } from './support/plugin-export-build'; |
There was a problem hiding this comment.
[medium] pattern-violation
RunCommandOptions is imported but no longer used anywhere in the file. The only consumer was the local runExpectingFailure function, which was removed and promoted to the shared support module in this PR. The stale import will produce an unused-import lint error and fail yarn lint:check in CI.
Suggested fix: Remove RunCommandOptions from the import block on line 22.
| // Ensure pluginDir is aligned with 2.1.0 | ||
| await runCommand(`"${rhdhCli}" plugin upgrade 2.1.0 --skip-install`, { | ||
| cwd: pluginDir, | ||
| }); |
There was a problem hiding this comment.
[low] test-correctness
The multi-step test asserts upgrade20Json.backstageVersion equals 1.52.0 and pkgAfter20.dependencies for @backstage/core-plugin-api equals ^1.12.7 without pinning to offline mode. Tier-1 remote fetch from the RHDH release-2.0 branch takes precedence when online. RHDH 2.0 is a frozen GA release so practical flakiness risk is low, but the unnecessary network dependency could produce false-negatives if the remote is transiently unavailable.
Suggested fix: Add RHDH_OFFLINE: true to the env for the multi-step upgrade and check calls, or use --manifest-file with a local fixture manifest to make the assertions fully deterministic.
| const currentPkg = await fs.readJson( | ||
| path.join(pluginDir, 'package.json'), | ||
| ); | ||
| expect(currentPkg.dependencies['@backstage/core-plugin-api']).not.toBe( |
There was a problem hiding this comment.
[low] test-correctness
The dependency upgrading describe block has a beforeEach that only re-skews two specific packages without a matching afterEach to restore the directory. After the --skip-install test fully upgrades all packages, subsequent beforeEach calls produce a mixed state. Test 3 partially compensates by manually re-applying skewedPackageJson, but the design is fragile against future test additions.
Suggested fix: Add an afterEach that restores pluginDir/package.json and backstage.json from a snapshot taken in beforeAll, mirroring the pattern used in the dependency auditing block.
| */ | ||
| async function runInstallDependencies(targetDir: string): Promise<boolean> { | ||
| const pm = await detectPackageManager(targetDir); | ||
| const prevImmutable = process.env.YARN_ENABLE_IMMUTABLE_INSTALLS; |
There was a problem hiding this comment.
[low] scope-creep
The PR is typed test(version-management) via Conventional Commits enforced by pr-semantic.yaml, but includes two production code changes: the versions:lint alias registration and the YARN_ENABLE_IMMUTABLE_INSTALLS env-var management. The PR body is transparent about both.
Suggested fix: Amend the PR title to fix(version-management) to reflect the production fixes included, or split them into a preceding fix commit.
| // Upgrading dependencies inherently mutates the lockfile. Ensure Yarn Berry | ||
| // does not block lockfile updates when running in CI or automated PR workflows. | ||
| if (pm === 'yarn') { | ||
| process.env.YARN_ENABLE_IMMUTABLE_INSTALLS = 'false'; |
There was a problem hiding this comment.
[low] code-organization
runInstallDependencies temporarily mutates process.env.YARN_ENABLE_IMMUTABLE_INSTALLS globally rather than passing the override via subprocess spawn options. The save/restore logic in finally is correct (handles unset, set, and non-yarn cases), but global env mutation is unnecessary for a subprocess invocation.
Suggested fix: Pass the override via Task.forCommand spawn env option using env: { ...process.env, YARN_ENABLE_IMMUTABLE_INSTALLS: false } to avoid mutating the global environment.
1 participant
Summary
Adds an automated end-to-end integration test suite (
e2e-tests/plugin-version-management.test.ts) coveringrhdh-cli plugin check-versionsandrhdh-cli plugin upgradeacross real plugin file structures, fulfilling the E2E testing requirements of RHIDP-16669.Key Coverage
plugin check-versions& aliasversions:lint):Run rhdh-cli plugin upgrade 2.1.0) and structured--jsonoutput (valid: false,status: "mismatch").versions:lintalias insrc/commands/index.ts.plugin upgrade& aliasversions:bump):--dry-run: Verifies table diff output while ensuringpackage.jsonandbackstage.jsonremain untouched on disk.--skip-install: Verifiespackage.jsonandbackstage.jsonupdate to target manifest versions without triggering package manager install.yarn install) updatingyarn.lock.yarn tsc,yarn build, andrhdh-cli plugin exportrun cleanly on the upgraded plugin, generatingdist-dynamic/package.jsonwith matchingbackstage.supported-versionsand zero version conflicts.check-versionsandupgradewith--manifest-file <path>andRHDH_OFFLINE=truewithout external network calls.e2e-tests/support/plugin-export-build.ts):ErrorfromrunCommandwithstdout,stderr,code, andsignalproperties for robust error assertions.Follow-up documentation refactoring across all on-ramp workflows (
plugin new,plugin dev,plugin check-versions,plugin upgrade) will be delivered in a subsequent PR.