Fixes #37834 - Set Firmware selection correct #10550

sbernhard · 2025-05-27T06:37:11Z

Without this PR, its is currently really broken. Without this PR, it does always! show 'Automatic' - except while editing hosts. For new/edit of Compute Profile or in the Create Host page - always 'Automatic' is shown. A user would never know what is really configured. This is because of [1].

If you create a host later, 'Automatic' is always displayed, regardless of which firmware is selected in the Compute Profile.

This change makes sure, that the selected firmware on the Compute Profile is the one which was saved previously and represents the value from the database while editing a existing Compute Profile.

The trick is to extend the new_vm() method with a "dry-run" parameter. dry-run is normally set to "true" as the new_vm() method is used at the compute_profile and hosts page for showing the form. The firmware modification should only be done in case the vm is later on really saved.

[1] https://github.com/theforeman/foreman/pull/10321/files#diff-c88b347f9af46e109987241498e9db3c776cc76433ffe3ba6f58ae1d8a74b9adR11

sbernhard · 2025-05-27T06:39:32Z

Another try, similar to #10529 to solve the firmware issue.
This PR tries to solve the root cause of the issue, that the original firmware form value is changed while showing / validating the form and not only just before generating the vm.

@nofaralfasi, please have a look. We can work together on a solution and jump on a call if you have further ideas.

nofaralfasi

The code looks good overall. It’s not the ideal solution, but it fixes the problem we’re trying to solve.
One small suggestion: I think the dry_run variable name could be more descriptive. Maybe something like new_host, or another name that better reflects its purpose.
Also, it might help future readers if you could add a short comment explaining why this variable is needed in the first place.
Also, could you update the tests and consider adding a few more for better coverage?

sbernhard · 2025-07-10T20:39:52Z

@nofaralfasi changed the name of the flag and changed it from additional function parameter to a attribute. This solved a lot of test issues. Additionally, I have added a comment.

I have adapted the tests. Foreman tests are green now.

I tested host create and edit manually with the UI and it worked.
I tested compute profile create and edit manually with the UI and it worked.
Tested host creation with existing compute profiles (Automatic, BIOS, UEFI, UEFI SECURE) and it worked. The host uses the correct profile afterwards:
- it choose BIOS if Automatic was used
- it choose UEFI Secure Boot if boot loader was Grub2 UEFI SecureBoot)

sbernhard · 2025-07-11T08:54:34Z

Looks like the test issues in Katello are not related. E.g. https://github.com/theforeman/foreman/actions/runs/16202971385/job/45746456820?pr=10600 is failing because of the same issue.

nofaralfasi

I feel that adding the create_vm_ctx variable in multiple places introduces unnecessary complexity and makes the code harder to follow and maintain. Is there a way we could simplify or centralize it?

sbernhard · 2025-07-15T09:41:52Z

I feel that adding the create_vm_ctx variable in multiple places introduces unnecessary complexity and makes the code harder to follow and maintain. Is there a way we could simplify or centralize it?

I don't know how because this flag need to be set so that it will do the magic. Do you have an idea?

sbernhard · 2025-07-17T19:14:19Z

all tests have passed.

nofaralfasi

Instead of introducing a new create_vm_ctx flag, have you considered using the host_compute_attrs method? It only passes a hash during actual VM creation from Foreman, so we might be able to leverage it to simplify the logic.

app/models/compute_resource.rb

app/models/compute_resources/foreman/model/libvirt.rb

sbernhard · 2025-07-20T20:30:22Z

Instead of introducing a new create_vm_ctx flag, have you considered using the host_compute_attrs method? It only passes a hash during actual VM creation from Foreman, so we might be able to leverage it to simplify the logic.

I will try this out tomorrow.
Other than that, thanks for your suggestions - I have applied them.

nofaralfasi · 2025-09-04T13:27:46Z

app/models/compute_resource.rb

+      firmware = resolve_automatic_firmware(firmware, firmware_type)
+      attrs[:firmware] = normalize_firmware_type(firmware)
+    else
+      attrs[:firmware] = firmware || 'bios'


Why use bios instead of automatic? I think it would be better to set the default to automatic.

to be "in sync" with

https://github.com/ATIX-AG/foreman/blob/develop/app/models/compute_resource.rb#L422

https://github.com/ATIX-AG/foreman/blob/develop/app/models/compute_resource.rb#L433

After processing the user’s input, the default type should be BIOS. For a new form, the default should be Automatic. References:

VMware: https://github.com/nofaralfasi/foreman/blob/develop/app/models/compute_resources/foreman/model/vmware.rb#L823
https://github.com/nofaralfasi/foreman/blob/develop/app/views/compute_resources_vms/form/vmware/_base.html.erb#L10

Libvirt:
https://github.com/nofaralfasi/foreman/blob/develop/app/models/compute_resources/foreman/model/libvirt.rb#L300
https://github.com/nofaralfasi/foreman/blob/develop/app/views/compute_resources_vms/form/libvirt/_base.html.erb#L11

Hi @sbernhard, any updates on this?

Hi @sbernhard, any updates on this?

Sorry, have a look at the last changes. There are 2 commits: 1 with the "original" implementation and the second commit for changing the default from "bios" to "automatic" and the required *-test changes.

Let me know what you think about. If ok, I can squash it.

Hi @sbernhard, yes please squash it and I will merge this PR. Thanks!

Without this PR, its is currently really broken. Without this PR, it does always! show 'Automatic' - except while editing hosts. For new/edit of Compute Profile or in the Create Host page - always 'Automatic' is shown. A user would never know what is really configured. This is because of [1]. If you create a host later, 'Automatic' is always displayed, regardless of which firmware is selected in the Compute Profile. This change makes sure, that the selected firmware on the Compute Profile is the one which was saved previously and represents the value from the database while editing a existing Compute Profile. The `process_firmware_attributes` is modified so that the methods `resolve_automatic_firmware` and `normalize_firmware_type` are only used if a host is created on the compute resource. [1] https://github.com/theforeman/foreman/pull/10321/files#diff-c88b347f9af46e109987241498e9db3c776cc76433ffe3ba6f58ae1d8a74b9adR11

nofaralfasi

Thank you @sbernhard!!

nlaphine · 2025-11-23T20:52:56Z

I know this is "Closed and Merged", but why "After processing the user’s input, the default type should be BIOS"
We live in modern times. EFI is available on every new machine. BIOS is the odd one out now.

Just a question to prompt a change of thinking for the future if I may be so bold.

Thanks
Peter

nofaralfasi · 2025-11-24T10:51:36Z

I know this is "Closed and Merged", but why "After processing the user’s input, the default type should be BIOS" We live in modern times. EFI is available on every new machine. BIOS is the odd one out now.

Hi Peter,
The default is set to BIOS only when no PXE Loader is selected and the Firmware type is set to Automatic. This has been the existing behavior until now, and this PR does not change it.

If we want to change that behavior, we should open a new issue explaining the rationale, discuss it as a team, and then address it in a separate PR.

This PR focuses on fixing a different problem, so we shouldn’t mix the two concerns.

pr-processor bot added the Not yet reviewed label May 27, 2025

nofaralfasi reviewed Jul 8, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jul 8, 2025

sbernhard force-pushed the fix_37834__2 branch 8 times, most recently from 09172e7 to 6d0aa2d Compare July 10, 2025 18:13

pr-processor bot added the Waiting on contributor label Jul 10, 2025

sbernhard force-pushed the fix_37834__2 branch from 9c92983 to 9f27184 Compare July 10, 2025 19:57

pr-processor bot added the Needs re-review label Jul 10, 2025

sbernhard force-pushed the fix_37834__2 branch from 9f27184 to cc0de04 Compare July 10, 2025 20:07

pr-processor bot removed the Waiting on contributor label Jul 10, 2025

sbernhard force-pushed the fix_37834__2 branch from cc0de04 to c076dc1 Compare July 11, 2025 09:32

nofaralfasi reviewed Jul 15, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Jul 15, 2025

sbernhard force-pushed the fix_37834__2 branch from c076dc1 to dd8f5c2 Compare July 15, 2025 11:25

nofaralfasi reviewed Jul 20, 2025

View reviewed changes

sbernhard force-pushed the fix_37834__2 branch from dd8f5c2 to bca7725 Compare July 20, 2025 20:31

pr-processor bot added the Waiting on contributor label Jul 20, 2025

sbernhard force-pushed the fix_37834__2 branch from bca7725 to d1694fe Compare July 20, 2025 20:38

sbernhard force-pushed the fix_37834__2 branch from 0c9a273 to 287832d Compare July 30, 2025 13:56

pr-processor bot added Needs re-review and removed Needs re-review labels Jul 30, 2025

sbernhard force-pushed the fix_37834__2 branch from 287832d to 71e8425 Compare July 30, 2025 15:56

pr-processor bot added Needs re-review and removed Needs re-review labels Jul 30, 2025

sbernhard force-pushed the fix_37834__2 branch from 71e8425 to 688794d Compare August 18, 2025 16:53

pr-processor bot added Needs re-review and removed Waiting on contributor labels Aug 18, 2025

sbernhard force-pushed the fix_37834__2 branch from 688794d to b6a28a5 Compare August 19, 2025 13:24

pr-processor bot removed the Needs re-review label Aug 19, 2025

sbernhard force-pushed the fix_37834__2 branch from b6a28a5 to dd79fd8 Compare August 19, 2025 21:22

nofaralfasi reviewed Sep 4, 2025

View reviewed changes

sbernhard force-pushed the fix_37834__2 branch from dd79fd8 to 25060c2 Compare November 3, 2025 16:13

pr-processor bot added the Waiting on contributor label Nov 3, 2025

sbernhard force-pushed the fix_37834__2 branch from 25060c2 to 3f5ef80 Compare November 3, 2025 16:35

pr-processor bot added the Needs re-review label Nov 3, 2025

sbernhard force-pushed the fix_37834__2 branch from 3f5ef80 to 2923033 Compare November 4, 2025 07:57

pr-processor bot removed the Needs re-review label Nov 4, 2025

sbernhard force-pushed the fix_37834__2 branch from 2923033 to 5158c02 Compare November 21, 2025 12:45

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 21, 2025

nofaralfasi approved these changes Nov 21, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 21, 2025

nofaralfasi merged commit c04d6ae into theforeman:develop Nov 21, 2025
35 checks passed

sbernhard mentioned this pull request Dec 5, 2025

Fixes #37834 - Set Firmware selection correct #10529

Closed

maximiliankolb deleted the fix_37834__2 branch December 8, 2025 07:04

Fixes #37834 - Set Firmware selection correct #10550

Fixes #37834 - Set Firmware selection correct #10550

Conversation

sbernhard commented May 27, 2025

Uh oh!

sbernhard commented May 27, 2025

Uh oh!

nofaralfasi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernhard commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbernhard commented Jul 11, 2025

Uh oh!

nofaralfasi left a comment

Choose a reason for hiding this comment

Uh oh!

sbernhard commented Jul 15, 2025

Uh oh!

sbernhard commented Jul 17, 2025

Uh oh!

nofaralfasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbernhard commented Jul 20, 2025

Uh oh!

nofaralfasi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

sbernhard Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

nofaralfasi Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

nofaralfasi Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

sbernhard Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nofaralfasi Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

nofaralfasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlaphine commented Nov 23, 2025

Uh oh!

nofaralfasi commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nofaralfasi left a comment •

edited

Loading

sbernhard commented Jul 10, 2025 •

edited

Loading

sbernhard Nov 4, 2025 •

edited

Loading