Fixes #38992 - Update host vmware form to PF5 by MariaAga · Pull Request #10817 · theforeman/foreman

MariaAga · 2026-01-14T15:03:27Z

Depends on #10811 (packing review is because of that PR)
Tests were created with help of AI.

To test:
In the host form -> select a compute resource with vmware type -> go to the virtual machine tab
a. see the new style of the alarm that says "Please select a cluster" at the bottom of the form
b. select a cluster, the Storage section is updated to PF5

Screenshots of the new form:

Copilot

Pull request overview

This PR updates the VMware host storage form components to use PatternFly 5 (PF5) components, migrating from the deprecated patternfly-react library. The changes modernize UI components including alerts, buttons, selects, form groups, and input fields.

Changes:

Migrated storage form components from patternfly-react to @patternfly/react-core
Replaced custom form components with PF5 native components and @patternfly/react-templates
Updated tests from Enzyme to React Testing Library
Added new FormStatus component for displaying loading and error states

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js	Updated Alert and Button imports, replaced AlertBody with PF5 Alert title prop, removed bsStyle prop
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js	Migrated to SimpleSelect, added Form/FormGroup structure, updated Button API, removed unused _updateDisk function
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/index.js	Complete rewrite using PF5 components (TypeaheadSelect, NumberInput, Checkbox), added state management for selections, implemented FormStatus integration
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/FormStatus.js	New component to display loading spinners and error popovers based on STATUS values
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.test.js	Rewrote tests using React Testing Library instead of Enzyme
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/controller.test.js	Rewrote tests using React Testing Library with fake timers for async operations
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/tests/integration.test.js	Updated integration tests to use rtlHelpers and React Testing Library
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.scss	Removed custom CSS classes replaced by PF5 components, kept minimal styling
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/controller.scss	Deleted file - CSS no longer needed with PF5 Form components
webpack/assets/javascripts/react_app/components/hosts/storage/vmware/StorageContainer.scss	Removed unused .controller-container margin rule
package.json	Updated PatternFly packages to 5.4.x versions, added @patternfly/react-templates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-14T15:33:20Z

+            onChange={event =>
+              updateDisk('sizeGb', parseInt(event.target.value, 10) || 0)
+            }


The NumberInput onChange handler may not handle all edge cases properly. When the input is cleared or contains invalid characters, parseInt(event.target.value, 10) || 0 will default to 0, which bypasses the min={1} constraint. This could result in a value of 0 being set even though the minimum is 1. Consider validating that the parsed value respects the minimum constraint before calling updateDisk.

Copilot · 2026-01-14T15:33:20Z

+            onMinus={() =>
+              updateDisk('sizeGb', (parseInt(sizeGb, 10) || 0) - 1)
+            }


The onMinus handler could produce a value less than the minimum (1) when sizeGb is 1. The calculation (parseInt(sizeGb, 10) || 0) - 1 would result in 0 when starting from 1. While the NumberInput component may have its own validation, it's better to enforce the minimum constraint explicitly in the handler to prevent invalid values from being set in the application state.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-15T08:00:34Z

+      }
+    });
+
+    it('handles null sizeGb value', () => {


The test name 'handles null sizeGb value' is misleading because the test actually uses sizeGb={0}, not null. The test should be renamed to 'handles zero sizeGb value' or the test should be updated to actually test with null/undefined.

Suggested change

it('handles null sizeGb value', () => {

it('handles zero sizeGb value', () => {

MariaAga · 2026-01-21T13:51:49Z

Updated buttons to only have 1 primary as before

adamruzicka

Not sure if it is caused by this, but submitting the form with fields missing leads to a page with rails traceback:

New:

Old:

adamruzicka · 2026-01-22T09:23:01Z

+                <Button
+                  variant="danger"
+                  className="close"
+                  onClick={removeDisk}
+                  ouiaId="btn-disk-remove"
+                  aria-label="Remove disk"
+                >


It was not immediately obvious to me what the red button would do with it being so close to the name of the disk. It was much clearer before this change

New:

Old:

I think it would be best to give it text, as I didnt see the old page like that, and with a wide screen the old page makes sense.

+1 for text

Added text to the button and moved it to the right

Will it be better to name it "remove volume" instead of "remove disk" to match "add volume"?

Yes, volumes all the way

updated, thanks

ekohl · 2026-05-18T17:15:29Z

Just based on the screenshot I think it's correct but I hope someone has the capacity to properly test it.

kmalyjur

LGTM from the javascript side. I hope the Rocket team will test the edge cases more.

stejskalleos

The PR is not for the full VMware form, only the storage section -> The commit message & PR title should reflect that.
Nitpick: I can type -20 into the Size (GB) field.
If I don't select required fields Storage Pod * or Data store, the form can be submitted. I know it's a React component inserted into the form, but it would be nice not to allow it. Anyway, not gonna block on this PR.

Can you please rebase the issue over the latest develop? (because of dc528f4)

ShimShtein · 2026-05-31T09:20:39Z

/packit rpm-build

packit-as-a-service · 2026-05-31T09:20:43Z

There appears to be a syntax error in the command provided. Please refer to the Packit documentation or use the /packit help command.

ShimShtein · 2026-05-31T09:21:55Z

/packit build

ShimShtein · 2026-05-31T12:10:06Z

Did some testing, OCP compute resource is not affected.
One nitpick: when showing the list of disks, only the first disk in a controller is expanded. Is it intentional?

While we are at it, the defaults are also not consistent: only the first disk is defined as "thin" by default, all others are defined as "thick" (thin checkbox is unmarked). Is it also intentional?
Otherwise LGTM.

MariaAga · 2026-06-01T07:54:03Z

Thanks all, I will get to the review comments this week, about

While we are at it, the defaults are also not consistent: only the first disk is defined as "thin" by default, all others are defined as "thick" (thin checkbox is unmarked). Is it also intentional?

This was the behaviour before, should I change it to the default being thin for all?

MariaAga · 2026-06-02T10:24:14Z

The PR is not for the full VMware form, only the storage section -> The commit message & PR title should reflect that.

Updated

Nitpick: I can type -20 into the Size (GB) field.

fixed

If I don't select required fields Storage Pod * or Data store, the form can be submitted. I know it's a React component inserted into the form, but it would be nice not to allow it. Anyway, not gonna block on this PR.

Thats not consistent with the host form behaviour for any other field (for example: name can be empty and the submit button is enabled)

ShimShtein · 2026-06-02T10:29:43Z

This was the behaviour before, should I change it to the default being thin for all?

If that was the original behavior, I think it's good enough. Let's keep it that way.

MariaAga · 2026-06-03T14:47:08Z

Talked to @ShimShtein, and he pointed out two issues, that are now fixed:

Volume expansion

On edit host (vmExists), all volumes are expanded by default so existing disk settings are visible without clicking each one.
On new host, the first volume and the most recently added volume stay expanded.
New disk defaults

New volumes no longer inherit Storage Pod or Data store from an existing disk when adding a volume or controller. Each new disk starts with empty storage fields and must be configured explicitly. (This was a leftover from a discussed logic, its not changing the original behaviour)

ShimShtein

LGTM

ShimShtein · 2026-06-09T07:37:03Z

I have tested the PR with packit build:

Create a host with the default disk
create the host and add more disks and controllers
Edit host with multiple disks and see the R/O disk info
make sure OCP-V compute resource still works as previously.

No longer relevant

MariaAga requested a review from a team as a code owner January 14, 2026 15:03

pr-processor Bot added the Not yet reviewed label Jan 14, 2026

github-actions Bot added the UI label Jan 14, 2026

MariaAga requested review from Copilot January 14, 2026 15:04

Copilot started reviewing on behalf of MariaAga January 14, 2026 15:05 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Jan 14, 2026

Copilot AI reviewed Jan 14, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from bbf7669 to 53c71ec Compare January 14, 2026 15:28

MariaAga requested a review from Copilot January 14, 2026 15:28

Copilot started reviewing on behalf of MariaAga January 14, 2026 15:29 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from 53c71ec to ead6fa7 Compare January 15, 2026 07:57

MariaAga requested a review from Copilot January 15, 2026 07:57

Copilot started reviewing on behalf of MariaAga January 15, 2026 07:57 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

MariaAga force-pushed the update-vmware branch from ead6fa7 to 49fed73 Compare January 15, 2026 09:39

This was referenced Jan 15, 2026

Fixes #38998 - Deprecate pf3 select form field #10818

Merged

Refs #38822: update pf3 buttons to pf5 - hosts #10808

Closed

MariaAga force-pushed the update-vmware branch from 49fed73 to e551a1a Compare January 16, 2026 11:24

MariaAga added the breaks-robottelo label Jan 21, 2026

MariaAga force-pushed the update-vmware branch from e551a1a to 9717b3b Compare January 21, 2026 13:51

adamruzicka previously requested changes Jan 22, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jan 22, 2026

MariaAga force-pushed the update-vmware branch from 9717b3b to 9ce5018 Compare January 22, 2026 15:25

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Jan 22, 2026

MariaAga force-pushed the update-vmware branch 2 times, most recently from c2b2c43 to 5c7c5e1 Compare May 18, 2026 15:52

kmalyjur approved these changes May 28, 2026

View reviewed changes

stejskalleos self-assigned this May 29, 2026

stejskalleos previously requested changes May 29, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label May 29, 2026

MariaAga force-pushed the update-vmware branch from 5c7c5e1 to d590bed Compare June 2, 2026 10:22

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Jun 2, 2026

pr-processor Bot added the Waiting on contributor label Jun 3, 2026

ShimShtein approved these changes Jun 4, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jun 4, 2026

Fixes #38992 - Update host vmware Storage form to PF5

2483f8e

MariaAga force-pushed the update-vmware branch from dac79ce to 2483f8e Compare June 8, 2026 14:24

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Jun 8, 2026

ShimShtein merged commit 62ef8d0 into theforeman:develop Jun 9, 2026
39 of 42 checks passed

amolpati30 mentioned this pull request Jun 30, 2026

Update locator for storage remove-controller action. SatelliteQE/airgun#2468

Merged

	it('handles null sizeGb value', () => {
	it('handles zero sizeGb value', () => {

Uh oh!

Conversation

MariaAga commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MariaAga commented Jan 21, 2026

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

adamruzicka Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

adamruzicka Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

MariaAga commented Jan 14, 2026 •

edited

Loading