chore(ci): dynamically decrease podman-desktop version before update e2e test #13448
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGitHub Actions workflows (.github/workflows/e2e-main.yaml and .github/workflows/pr-check.yaml) were updated to replace hard-coded version-downgrade steps with a Node.js utility invocation (tests/playwright/scripts/version-util.cjs ./package.json --update). Workflows now derive PD_VERSION from package.json and export it; macOS also exports PD_APP_PATH and locates the DMG volume dynamically using PD_VERSION. A new CommonJS script tests/playwright/scripts/version-util.cjs was added to decrement the package.json minor version (supports a --update flag and a fallback version). ESLint ignore patterns were extended to exclude tests/playwright/scripts/** and tests/playwright/tsconfig.json was updated to include scripts/*. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit can use Shopify Theme Check to improve the quality of Shopify theme reviews.Add a configuration file to your project to customize how CodeRabbit runs Shopify Theme Check. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)< 10000 p dir="auto">Type@coderabbitai help to get the list of available commands.
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/e2e-main.yaml (2)
218-229: Same UTF-16/jqpitfalls as inpr-check.yamlWindow stepRepeat the fixes: install
jqand add-Encoding utf8when writingpackage.json_tmp.
300-311: Replicate semver downgrade refinement for macOS pathLogic and edge-cases identical to the pr-check workflow – please apply the same improvement suggestion here for consistency.
🧹 Nitpick comments (1)
.github/workflows/pr-check.yaml (1)
548-557: Prefer a semver-aware downgrade instead of hard-coding to1.20.0When the current version is
x.0.z, the fallback jumps to1.20.0, which may be newer than the version under test (e.g.1.0.0 → 1.20.0).
Consider computing:if minor == 0: major -= 1; minor = maxMinorForMajoror use the
npm semverCLI (npx semver "$(jq -r '.version' package.json)" -i --preid 0 -d minor) to reliably pick “previous minor release”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
📚 Learning: in test files for podman desktop, benoitf prefers partial mocking over complete mocking. partial moc...
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
Applied to files:
.github/workflows/pr-check.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: mac update e2e tests
- GitHub Check: win update e2e tests (windows-2025)
- GitHub Check: build website
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: typecheck
- GitHub Check: unit tests / windows-2025
- GitHub Check: macOS
- GitHub Check: Linux
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/playwright/scripts/version-util.cjs (2)
21-31: LGTM with minor modernization suggestion.The version parsing logic is correct and handles invalid versions appropriately. However, consider using the modern Number.parseInt for consistency with ES2015+ standards.
Apply this diff to modernize the parseInt usage:
- const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10);
33-60: LGTM with template literal cleanup.The function correctly handles version decrementing with appropriate error handling, file validation, and dry-run functionality. The fallback to "1.20.0" for negative minor versions aligns with the workflow requirements.
Apply this diff to remove unnecessary template literal:
- console.log(`Minor cannot be less than 0, defaults to 1.20.0`); + console.log('Minor cannot be less than 0, defaults to 1.20.0');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)tests/playwright/scripts/version-util.cjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/e2e-main.yaml
- .github/workflows/pr-check.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default (version 1.8.1 on windows-2025), so no additional installation step is required when using jq in PowerShell scripts.
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default, so no additional installation step is required when using jq in PowerShell scripts.
📚 Learning: cli tool extensions have two distinct `selectversion` method definitions: one in the installer (whic...
Learnt from: dgolovin
PR: podman-desktop/podman-desktop#13030
File: packages/main/src/plugin/cli-tool-registry.ts:108-113
Timestamp: 2025-07-07T17:23:41.302Z
Learning: CLI tool extensions have two distinct `selectVersion` method definitions: one in the installer (which accepts an optional `latest` parameter) and one in the updater (which has different selection logic). The installer's `selectVersion` method signature is `async (latest?: boolean) => Promise<string>` while the updater's is `async () => Promise<string>`.
Applied to files:
tests/playwright/scripts/version-util.cjs
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 46-46: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 24-24: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 25-25: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (1)
tests/playwright/scripts/version-util.cjs (1)
62-73: LGTM with a note on argument handling.The command-line argument parsing correctly handles the --update flag and path extraction with appropriate error handling for missing arguments.
Consider what should happen if multiple non-flag arguments are provided. Currently, only the first one is used, which may be the intended behavior for this specific use case in CI workflows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/playwright/scripts/version-util.cjs (2)
21-31: Apply static analysis suggestions for modern JavaScript.The function logic is sound, but please address the static analysis recommendations:
function parseVersion(version) { const parts = version.split('.'); if (parts.length >= 2) { - const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10); if (!isNaN(major) && !isNaN(minor)) { return [major, minor]; } } throw new Error(`Invalid version: ${version}`); }
33-60: Fix unnecessary template literal and verify fallback version.The function implementation is solid with good error handling and formatting. Please address the static analysis suggestion:
if (newMinor < 0) { - console.log(`Minor cannot be less than 0, defaults to 1.20.0`); + console.log('Minor cannot be less than 0, defaults to 1.20.0'); newVersion = '1.20.0'; }The hardcoded fallback to "1.20.0" appears intentional based on the PR context. The function correctly handles file operations, JSON parsing, and provides useful dry-run functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)eslint.config.mjs(1 hunks)tests/playwright/scripts/version-util.cjs(1 hunks)tests/playwright/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eslint.config.mjs
- tests/playwright/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pr-check.yaml
- .github/workflows/e2e-main.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13232
File: packages/api/src/repository-infos.ts:21-21
Timestamp: 2025-07-16T15:41:18.089Z
Learning: In the Podman Desktop project, the `with { type: 'json' }` syntax is the correct modern approach for JSON imports with TypeScript 5.8.3 and Vite, not the older `assert { type: 'json' }` syntax.
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default (version 1.8.1 on windows-2025), so no additional installation step is required when using jq in PowerShell scripts.
📚 Learning: cli tool extensions have two distinct `selectversion` method definitions: one in the installer (whic...
Learnt from: dgolovin
PR: podman-desktop/podman-desktop#13030
File: packages/main/src/plugin/cli-tool-registry.ts:108-113
Timestamp: 2025-07-07T17:23:41.302Z
Learning: CLI tool extensions have two distinct `selectVersion` method definitions: one in the installer (which accepts an optional `latest` parameter) and one in the updater (which has different selection logic). The installer's `selectVersion` method signature is `async (latest?: boolean) => Promise<string>` while the updater's is `async () => Promise<string>`.
Applied to files:
tests/playwright/scripts/version-util.cjs
🧬 Code Graph Analysis (1)
tests/playwright/scripts/version-util.cjs (1)
extensions/compose/src/detect.ts (1)
parseVersion(117-121)
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 46-46: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 24-24: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 25-25: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: mac update e2e tests
- GitHub Check: win update e2e tests (windows-2025)
- GitHub Check: macOS
- GitHub Check: linter, formatters
- GitHub Check: build website
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: typecheck
🔇 Additional comments (2)
tests/playwright/scripts/version-util.cjs (2)
1-20: LGTM! Clean license header and modern Node.js imports.The Apache 2.0 license header is properly formatted, and the use of
node:prefixes for built-in modules follows modern Node.js best practices.
62-74: LGTM! Simple and effective CLI argument parsing.The command-line interface is well-suited for CI/CD usage with clear error messaging and proper exit codes. The example usage comment is helpful for documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/playwright/scripts/version-util.cjs (2)
21-31: Use Number.parseInt for ES2015 compliance.The function logic is correct, but static analysis suggests using the namespaced version for better ES2015 compliance.
Apply this diff:
- const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10);
33-60: Fix template literal usage and verify fallback version.The function logic is sound with good error handling. Two minor improvements:
- Line 46 uses an unnecessary template literal (flagged by static analysis)
- The hardcoded fallback version "1.20.0" should be documented or made configurable
Apply this diff to fix the template literal:
- console.log(`Minor cannot be less than 0, defaults to 1.20.0`); + console.log('Minor cannot be less than 0, defaults to 1.20.0');Is the hardcoded fallback version "1.20.0" intentional and appropriate for the project's versioning strategy?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)eslint.config.mjs(1 hunks)tests/playwright/scripts/version-util.cjs(1 hunks)tests/playwright/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eslint.config.mjs
- tests/playwright/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/e2e-main.yaml
- .github/workflows/pr-check.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13232
File: packages/api/src/repository-infos.ts:21-21
Timestamp: 2025-07-16T15:41:18.089Z
Learning: In the Podman Desktop project, the `with { type: 'json' }` syntax is the correct modern approach for JSON imports with TypeScript 5.8.3 and Vite, not the older `assert { type: 'json' }` syntax.
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default (version 1.8.1 on windows-2025), so no additional installation step is required when using jq in PowerShell scripts.
📚 Learning: cli tool extensions have two distinct `selectversion` method definitions: one in the installer (whic...
Learnt from: dgolovin
PR: podman-desktop/podman-desktop#13030
File: packages/main/src/plugin/cli-tool-registry.ts:108-113
Timestamp: 2025-07-07T17:23:41.302Z
Learning: CLI tool extensions have two distinct `selectVersion` method definitions: one in the installer (which accepts an optional `latest` parameter) and one in the updater (which has different selection logic). The installer's `selectVersion` method signature is `async (latest?: boolean) => Promise<string>` while the updater's is `async () => Promise<string>`.
Applied to files:
tests/playwright/scripts/version-util.cjs
🧬 Code Graph Analysis (1)
tests/playwright/scripts/version-util.cjs (1)
extensions/compose/src/detect.ts (1)
parseVersion(117-121)
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 46-46: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 24-24: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 25-25: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (2)
tests/playwright/scripts/version-util.cjs (2)
1-19: LGTM!Standard Apache 2.0 license header is correctly formatted and consistent with project standards.
62-73: LGTM!The command-line argument parsing is well-implemented with proper error handling and clear user feedback. The logic correctly handles the --update flag and validates required arguments.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/playwright/scripts/version-util.cjs (1)
62-74: Guard CLI execution and export functions for reuse and unit testsWrap the CLI entry point with require.main and export helpers for testing/reuse. Current script runs unconditionally when required.
-// Example usage: node tests/playwright/scripts/version-util.cjs ./package.json --update -const args = process.argv.slice(2); -const updateFlagIndex = args.indexOf('--update'); -const shouldUpdate = updateFlagIndex !== -1; -const pathArg = args.filter(arg => arg !== '--update')[0]; - -if (!pathArg) { - console.error('Error: A path to a package.json file must be provided as an argument.'); - process.exit(1); -} - -decreaseMinorVersion(pathArg, shouldUpdate); +// Example usage: node tests/playwright/scripts/version-util.cjs ./package.json --update +if (require.main === module) { + const args = process.argv.slice(2); + const updateFlagIndex = args.indexOf('--update'); + const shouldUpdate = updateFlagIndex !== -1; + const pathArg = args.find(arg => arg !== '--update'); + + if (!pathArg) { + console.error('Error: A path to a package.json file must be provided as an argument.'); + process.exit(1); + } + + decreaseMinorVersion(pathArg, shouldUpdate); +} + +module.exports = { + parseVersion, + decreaseMinorVersion, +};I can add a quick unit test harness if you’d like.
♻️ Duplicate comments (1)
.github/workflows/e2e-main.yaml (1)
217-217: Good move: centralize version handling via Node scriptThis aligns with prior feedback about using a single JS/Node utility for all platforms. Simpler and more maintainable.
🧹 Nitpick comments (3)
tests/playwright/scripts/version-util.cjs (2)
24-26: Use Number.parseInt and Number.isNaN for consistency with ES2015Biome flagged this. Prefer the Number namespace and Number.isNaN for clarity.
- const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); - if (!isNaN(major) && !isNaN(minor)) { + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10); + if (!Number.isNaN(major) && !Number.isNaN(minor)) {
49-57: Emit the computed version to GITHUB_OUTPUT for easy consumption in workflowsThis avoids extra tooling (e.g., jq) and makes PD_VERSION available via step outputs.
console.log(`Original version: ${originalVersion}`); console.log(`Decremented version (x.y.z): ${newVersion}`); if (shouldUpdate) { packageJson.version = newVersion; fs.writeFileSync(resolvedPath, JSON.stringify(packageJson, null, 2) + '\n', 'utf8'); console.log(`Success: package.json updated with version ${newVersion}`); } else { console.log('Dry run: File not modified. Use --update flag to replace the version.'); } + // If running in GitHub Actions, expose the result as a step output + if (process.env.GITHUB_OUTPUT) { + try { + fs.appendFileSync(process.env.GITHUB_OUTPUT, `pd_version=${newVersion}\n`); + } catch (e) { + console.warn('Failed to write GITHUB_OUTPUT:', e); + } + }.github/workflows/e2e-main.yaml (1)
288-290: Avoid jq dependency; read version with Node or capture from GITHUB_OUTPUTjq may not be available on all macOS runners. Use Node (already installed) or consume the script’s GITHUB_OUTPUT.
Option A (Node):
- actualVersion=$(jq -r '.version' package.json) + actualVersion=$(node -p 'require("./package.json").version') echo "PD_VERSION=$actualVersion" >> $GITHUB_ENVOption B (consume script output):
- Update the script to write pd_version to GITHUB_OUTPUT (see script review).
- Then here:
echo "PD_VERSION=${{ steps.decrease_version.outputs.pd_version }}" >> $GITHUB_ENV
📜 Review d 23D2 etails
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)eslint.config.mjs(1 hunks)tests/playwright/scripts/version-util.cjs(1 hunks)tests/playwright/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- eslint.config.mjs
- tests/playwright/tsconfig.json
- .github/workflows/pr-check.yaml
🧰 Additional context used
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 46-46: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 24-24: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 25-25: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: win update e2e tests (windows-2025)
- GitHub Check: mac update e2e tests
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (development)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: build website
- GitHub Check: Windows
🔇 Additional comments (1)
.github/workflows/e2e-main.yaml (1)
309-309: Nice: expose PD_APP_PATH for downstream stepsExporting the app path helps keep subsequent steps decoupled.
…e2e test Signed-off-by: Ondrej Dockal <odockal@redhat.com>
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/playwright/scripts/version-util.cjs (1)
46-50: Make minor-underflow fallback robust; avoid unused template literal; use warnCompute a previous-major fallback (or respect env override) and log via console.warn. Also removes the linter’s “unused template literal” warning.
- let newVersion = `${major}.${newMinor}.0`; - if (newMinor < 0) { - console.log(`Minor cannot be less than 0, defaults to 1.20.0`); - newVersion = FALLBACK_VERSION; - } + let newVersion; + if (newMinor < 0) { + const fallback = process.env.FALLBACK_VERSION || (major > 0 ? `${major - 1}.0.0` : '0.0.0'); + console.warn(`Minor cannot be less than 0; defaulting to ${fallback}`); + newVersion = fallback; + } else { + newVersion = `${major}.${newMinor}.0`; + }
🧹 Nitpick comments (5)
tests/playwright/scripts/version-util.cjs (3)
26-27: Use Number.parseInt per style guideAligns with Biome’s recommendation.
- const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10);
23-25: Be tolerant of “v” prefix and whitespace in version stringsSmall hardening: trim and strip an optional leading “v”.
-function parseVersion(version) { - const parts = version.split('.'); +function parseVersion(version) { + const cleaned = String(version).trim().replace(/^v/i, ''); + const parts = cleaned.split('.');
66-73: Harden CLI arg parsing and validate exactly one path argumentPrevents accidental multiple paths or stray flags being treated as the target path.
-const args = process.argv.slice(2); -const updateFlagIndex = args.indexOf('--update'); -const shouldUpdate = updateFlagIndex !== -1; -const pathArg = args.filter(arg => arg !== '--update')[0]; +const args = process.argv.slice(2); +const shouldUpdate = args.includes('--update'); +const nonFlagArgs = args.filter(arg => !arg.startsWith('-')); +const pathArg = nonFlagArgs[0]; +if (nonFlagArgs.length !== 1) { + console.error('Error: Exactly one path to package.json must be provided.'); + process.exit(1); +} -if (!pathArg) { - console.error('Error: A path to a package.json file must be provided as an argument.'); - process.exit(1); -} +// pathArg validated above.github/workflows/pr-check.yaml (2)
457-459: Export PD_VERSION on Windows too (for parity and future reuse)Not E7EE strictly required today, but it simplifies future steps and debugging to have PD_VERSION available like on macOS.
- - name: Adjust/Downgrade local podman desktop version Windows - run: node tests/playwright/scripts/version-util.cjs ./package.json --update + - name: Adjust/Downgrade local podman desktop version Windows + run: | + node tests/playwright/scripts/version-util.cjs ./package.json --update + # jq is available on Windows runners (per GH images); alternatively use node -p below + $actualVersion = (jq -r '.version' package.json).Trim() + # Fallback without jq: + # $actualVersion = node -p "require('./package.json').version" + echo ("PD_VERSION=" + $actualVersion) >> $env:GITHUB_ENV
531-537: Avoid jq dependency on macOS; use Node to read package.json versionNode is already set up; this reduces tooling variance and potential jq drift on images.
- name: Adjust/Downgrade local podman desktop version run: | - node tests/playwright/scripts/version-util.cjs ./package.json --update - actualVersion=$(jq -r '.version' package.json) - echo "PD_VERSION=$actualVersion" >> $GITHUB_ENV + node tests/playwright/scripts/version-util.cjs ./package.json --update + actualVersion=$(node -p "require('./package.json').version") + echo "PD_VERSION=$actualVersion" >> $GITHUB_ENV
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e-main.yaml(3 hunks).github/workflows/pr-check.yaml(2 hunks)eslint.config.mjs(1 hunks)tests/playwright/scripts/version-util.cjs(1 hunks)tests/playwright/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/playwright/tsconfig.json
- eslint.config.mjs
- .github/workflows/e2e-main.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-31T13:27:12.419Z
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default (version 1.8.1 on windows-2025), so no additional installation step is required when using jq in PowerShell scripts.
Applied to files:
.github/workflows/pr-check.yaml
📚 Learning: 2025-07-31T13:27:12.419Z
Learnt from: odockal
PR: podman-desktop/podman-desktop#13448
File: .github/workflows/pr-check.yaml:0-0
Timestamp: 2025-07-31T13:27:12.419Z
Learning: GitHub Actions Windows runners (windows-2025, windows-2022, etc.) have jq pre-installed by default, so no additional installation step is required when using jq in PowerShell scripts.
Applied to files:
.github/workflows/pr-check.yaml
🧬 Code Graph Analysis (1)
tests/playwright/scripts/version-util.cjs (1)
extensions/compose/src/detect.ts (1)
parseVersion(117-121)
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 48-48: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 26-26: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 27-27: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: win update e2e tests (windows-2025)
- GitHub Check: mac update e2e tests
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (production)
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: build website
- GitHub Check: smoke e2e tests (development)
- GitHub Check: typecheck
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/playwright/scripts/version-util.cjs (3)
21-22: Make fallback configurable via env and use ALL_CAPS constantHard-coding '1.20.0' is brittle. Surface a configurable fallback for CI and standardize naming.
-const fallBackVersion = '1.20.0'; +// Default fallback if minor underflows; can be overridden from CI with FALLBACK_VERSION +const FALLBACK_VERSION = process.env.FALLBACK_VERSION || '1.20.0';
40-44: Harden JSON parsing and validate the version field before parsingPrevents cryptic crashes on malformed package.json or missing version.
- const fileContent = fs.readFileSync(resolvedPath, 'utf8'); - const packageJson = JSON.parse(fileContent); - const originalVersion = packageJson.version; - const versionParts = parseVersion(originalVersion); + const fileContent = fs.readFileSync(resolvedPath, 'utf8'); + let packageJson; + try { + packageJson = JSON.parse(fileContent); + } catch (err) { + throw new Error(`Failed to parse JSON from ${resolvedPath}: ${err && err.message ? err.message : err}`); + } + const originalVersion = packageJson.version; + if (typeof originalVersion !== 'string' || originalVersion.length === 0) { + throw new Error(`Invalid or missing "version" field in ${resolvedPath}`); + } + const versionParts = parseVersion(originalVersion);
46-50: Fix minor-underflow branch: warn, remove unused template literal, and use the configurable fallbackAlso resolves the Biome noUnusedTemplateLiteral warning.
- let newVersion = `${major}.${newMinor}.0`; - if (newMinor < 0) { - console.log(`Minor cannot be less than 0, defaults to 1.20.0`); - newVersion = fallBackVersion; - } + let newVersion; + if (newMinor < 0) { + console.warn(`Minor cannot be less than 0; defaulting to ${FALLBACK_VERSION}`); + newVersion = FALLBACK_VERSION; + } else { + newVersion = `${major}.${newMinor}.0`; + }
🧹 Nitpick comments (2)
tests/playwright/scripts/version-util.cjs (2)
26-29: Use Number.parseInt/Number.isNaN to satisfy linter and modern JS styleAddresses Biome's useNumberNamespace rule and improves consistency.
- const major = parseInt(parts[0], 10); - const minor = parseInt(parts[1], 10); - if (!isNaN(major) && !isNaN(minor)) { + const major = Number.parseInt(parts[0], 10); + const minor = Number.parseInt(parts[1], 10); + if (!Number.isNaN(major) && !Number.isNaN(minor)) {
64-76: Guard CLI execution and export functions for reuse/testingAllows requiring this module in tests or elsewhere without executing the CLI path.
-// Example usage: node tests/playwright/scripts/version-util.cjs ./package.json --update -const args = process.argv.slice(2); -const updateFlagIndex = args.indexOf('--update'); -const shouldUpdate = updateFlagIndex !== -1; -const pathArg = args.filter(arg => arg !== '--update')[0]; - -if (!pathArg) { - console.error('Error: A path to a package.json file must be provided as an argument.'); - process.exit(1); -} - -decreaseMinorVersion(pathArg, shouldUpdate); +// Example usage: node tests/playwright/scripts/version-util.cjs ./package.json --update +module.exports = { parseVersion, decreaseMinorVersion }; + +if (require.main === module) { + const args = process.argv.slice(2); + const updateFlagIndex = args.indexOf('--update'); + const shouldUpdate = updateFlagIndex !== -1; + const pathArg = args.filter(arg => arg !== '--update')[0]; + + if (!pathArg) { + console.error('Error: A path to a package.json file must be provided as an argument.'); + process.exit(1); + } + + decreaseMinorVersion(pathArg, shouldUpdate); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/scripts/version-util.cjs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/playwright/scripts/version-util.cjs (2)
.electron-builder.config.cjs (2)
require(22-22)process(265-271)extensions/compose/src/detect.ts (1)
parseVersion(117-121)
🪛 Biome (2.1.2)
tests/playwright/scripts/version-util.cjs
[error] 48-48: Do not use template literals if interpolation and special-character handling are not needed.
Safe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 26-26: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 27-27: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: win update e2e tests (windows-2025)
- GitHub Check: mac update e2e tests
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (development)
- GitHub Check: typecheck
- GitHub Check: build website
- GitHub Check: smoke e2e tests (production)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (1)
tests/playwright/scripts/version-util.cjs (1)
51-59: LGTM: clear reporting and idempo 23D2 tent write behaviorThe informational logs and pretty-printed write (with trailing newline) are good for CI readability.
Signed-off-by: Ondrej Dockal <odockal@redhat.com>
…e2e test (podman-desktop#13448) * chore(ci): dynamically decrease podman-desktop version before update e2e test
What does this PR do?
dynamically update version of podman-desktp to be updated to a major.minor - 1
Screenshot / video of UI
What issues does this PR fix or reference?
#13447
How to test this PR?