chore: update when field for podman machine editable properties #12939
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Curious to know if this is really only working on Mac, or would work for windows HyperV (I know it is not working for WSL, but not sure abotu hyperV)
cc @jeffmaury
CPU and memory should be exposed for HyperV |
|
@axel7083 @jeffmaury right now the podman machine editing option is only registered for macOS |
So we should create a follow up issue as I would guess HyperV support was added after edit and that was missed |
There was a problem hiding this comment.
I would not add isMac as we know it's a limited factor
for example we can have a property dedicated set in the code
let say for example podman.machine.editCpu
and then in the code if macOS we set this property to true (or if Hyper-V)
it'll avoid to change the properties in package.json later
also it's easier to have the logic in the code rather in the package.json as it's easier to provide unit tests
📝 Walkthrough""" WalkthroughThe changes update the Podman extension to conditionally support editing machine resource settings—CPUs, memory, and disk size—based on the operating system and virtualization capabilities. New exported context keys ( Estimated code review effort2 (~20 minutes) Suggested labels
Suggested reviewers
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. 🔧 ESLint
extensions/podman/packages/extension/src/extension.tsOops! Something went wrong! :( ESLint: 9.30.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey @SoniaSandler - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
extensions/podman/packages/extension/package.json (2)
80-90: Same validation needed for Memory edit flag
podman.podmanMachineEditMemorySupportedmust follow the same life-cycle guarantees as the CPU flag (definition, initialisation, refresh).
Duplicating the verification logic will avoid drift between the two capabilities.
98-107: Confirm disk-size edit gating aligns with platform policyThe disk-size field is now hidden unless
podman.podmanMachineEditDiskSizeSupportedis true.
Because disk resizing is macOS-only, be certain that:
- The flag returns
trueonly on macOS.- Windows & Linux paths explicitly set it to
falseto prevent stale context state after an OS switch in dev environments.Minor: for consistency, consider shortening the flag names (
podmanMachineEditDiskSupportedvspodmanMachineEditDiskSizeSupported) or documenting the naming rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
extensions/podman/packages/extension/package.json(3 hunks)extensions/podman/packages/extension/src/extension.spec.ts(4 hunks)extensions/podman/packages/extension/src/extension.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/podman/packages/extension/src/extension.spec.ts (1)
packages/extension-api/src/extension-api.d.ts (3)
RunResult(4382-4397)ContainerProviderConnection(391-404)Disposable(90-118)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: unit tests / windows-2022
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (development)
- GitHub Check: typecheck
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: macOS
- GitHub Check: linter, formatters
- GitHub Check: build website
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (7)
extensions/podman/packages/extension/package.json (1)
63-72: Ensurepodman.podmanMachineEditCPUSupportedis refreshed on config changesThe context key itself is correctly declared and set:
- Constant at extensions/podman/packages/extension/src/extension.ts:1148
export const PODMAN_MACHINE_EDIT_CPU = 'podman.podmanMachineEditCPUSupported';- Runtime assignment at line 865:
extensionApi.context.setValue(PODMAN_MACHINE_EDIT_CPU, isEditCPUSupported);
where
• isEditCPUSupported = env.isMac || (env.isWindows && await isHyperVEnabled())
• covers all OS paths (false on Linux/WSL or Windows without Hyper-V).What’s missing is a listener to re-evaluate this key when the environment/configuration changes (e.g. Hyper-V toggled mid-session). No
onDidChangeConfigurationhandler exists in extension.ts, so users may see stale UI state until a provider refresh.• Add a
workspace.onDidChangeConfiguration(or equivalent withextensionApi) to recompute and re-setPODMAN_MACHINE_EDIT_CPUwhen relevant settings/OS capabilities change.
• Alternatively, confirm that your provider’s lifecycle refresh re-invokes the same context logic on config reloads.extensions/podman/packages/extension/src/extension.spec.ts (4)
1469-1471: LGTM! Proper testing of macOS edit capabilities.The assertions correctly verify that all machine editing context keys are set to
trueon macOS, which aligns with the platform support expectations.
1499-1518: Excellent test coverage for non-HyperV Windows scenario.This test correctly verifies that machine editing capabilities are disabled on Windows when HyperV is not available, ensuring the edit lifecycle method is undefined and all context keys are set to
false.
1520-1554: Well-implemented test for HyperV Windows scenario.The test effectively mocks HyperV detection using PowerShell commands and correctly verifies that CPU and memory editing are supported while disk size editing is restricted on Windows with HyperV.
1572-1574: Appropriate test coverage for Linux platform.The assertions correctly verify that machine editing capabilities are disabled on Linux, with all context keys set to
false, completing the comprehensive platform coverage.extensions/podman/packages/extension/src/extension.ts (2)
1148-1150: LGTM: Well-defined context keys for machine edit capabilities.The exported constants follow consistent naming conventions and clearly indicate their purpose for controlling machine edit functionality visibility.
893-893: LGTM: Correct platform restriction for disk size editing.The macOS-only restriction for disk size editing aligns with the context key setting and the PR objectives. This correctly ensures disk size editing is only available where supported.
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
ae5704c to
41f92cb
Compare
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler <ssandler@redhat.com>
Signed-off-by: Sonia Sandler ssandler@redhat.com
What does this PR do?
Right now, editing a podman machine is only possible on macOS. However, as part of #12781, the rootful status is also editable on both macOS and Windows, so we need to indicate which properties are editable only on macOS.
On HyperV, CPU and memory should be editable, and on macOS, all 3 properties are editable
Screenshot / video of UI
What issues does this PR fix or reference?
Related to #12781
Closes #12943
How to test this PR?