Fixes #37834 - Set Firmware selection correct

[sbernhard]

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]

Another try, similar to https://github.com/theforeman/foreman/pull/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.

[sbernhard]

@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]

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.

[sbernhard]

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]

all tests have passed.

[sbernhard]

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.

[sbernhard]

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.

I had a look at the host_compute_attrs method. The only thing what would be possible is, to set the create_modeattribute in this method - but, actually to set this attribute "just" before the call tonew_vm is better in my opionion.

[nofaralfasi]

I had a look at the host_compute_attrs method. The only thing what would be possible is, to set the create_modeattribute in this method - but, actually to set this attribute "just" before the call tonew_vm is better in my opionion.

What about using the existing :firmware_type field from host_compute_attrs to detect VM creation?

[sbernhard]

I had a look at the host_compute_attrs method. The only thing what would be possible is, to set the create_modeattribute in this method - but, actually to set this attribute "just" before the call tonew_vm is better in my opionion.

What about using the existing :firmware_type field from host_compute_attrs to detect VM creation?

Isn't this "more hackish" than have a well-documented flag with exactly this use-case?

[sbernhard]

All checks have passed again. As said, if it works and there are not other, even better ideas to solve the really bad issue, I would vote to fix it for now. Maybey future-foreman-dev has some even better ideas to improve but for now we should use what we have.

[nofaralfasi]

Isn't this "more hackish" than have a well-documented flag with exactly this use-case?

That's a fair question, perhaps it’s worth getting a third opinion on this. @ekohl can we get your take on this?

Problem: Firmware shows “Automatic” instead of the saved value because new_vm() processes firmware during form display. Option 1: Rely on existing fields - host_compute_attrs includes :firmware_type, which isn’t present during form display, so we detect creation via attr.key?(:firmware_type). Pros: no new params, leverages existing structure Cons: implicit detection

Option 2: Add a create_mode flag before calling new_vm. Pros: explicit control flow, clear intent Cons: more code changes

[sbernhard]

Isn't this "more hackish" than have a well-documented flag with exactly this use-case?

That's a fair question, perhaps it’s worth getting a third opinion on this. @ekohl can we get your take on this?

I had a look at this again. If we would change it to firmware_type it would be possible to do all the magic at this step: https://github.com/theforeman/foreman/blob/658ff7ebd6c755b47a5501abb6bf95b74723599b/app/models/compute_resource.rb#L447

This would then depend on the host_compute_attrs -> firmware_type and hopefully, this flag will never be changed / adapted. To use the "create_mode" flag is more is clearer and more obvious in my opinion.

Suggestion: to get this solved, can we discuss this on a video call?

Update: I will try out this solution now.