Fixes #37855 - Populate OS fields and pass correct network during import #10333
Conversation
| @@ -1,9 +1,9 @@ | |||
| <%= select_f f, :type, compute_resource.nictypes, :first, :last, { }, | |||
| :class => "col-md-3 vmware_type", | |||
| :size => "col-md-8", :disabled => !new_vm, | |||
There was a problem hiding this comment.
We shouldn't remove this, since we're not allowing to update the compute_attributes for existing VMs.
| @@ -1,6 +1,6 @@ | |||
|
|
|||
| <%= select_f f, :architecture_id, accessible_resource(f.object, :architecture), :id, :to_label, {:include_blank => true}, | |||
| {:onchange => 'architecture_selected(this);', :'data-url' => method_path('architecture_selected'), :'data-type' => controller_name.singularize, | |||
| {:onchange => 'architecture_selected(this);', :'data-url' => method_path('architecture_selected'), :'data-type' => "host", | |||
There was a problem hiding this comment.
I'm not sure if that's the way to go, since we are also using this form for hostgroups.
There was a problem hiding this comment.
we are using https://github.com/theforeman/foreman/blob/develop/app/views/common/os_selection/_operatingsystem.html.erb for hostgroups
There was a problem hiding this comment.
Yeah, you are right we are not using this one, my bad.
There was a problem hiding this comment.
I believe this template is only used in the host form (which includes VMs as well). I'm considering whether we should just pass data-type as host, or if it would be clearer and more consistent with the rest of the code to add a specific check for this scenario.
There was a problem hiding this comment.
We can override the controller_name to "host" for the compute_resources_vm controller but I guess that could maybe break things.
There was a problem hiding this comment.
I wouldn't modify the controller_name, but I'm wondering whether hard-coding it on the frontend is the best approach in this case.
There was a problem hiding this comment.
I believe we have no other option in this case!
I think it shall most probably not impact other use cases but I'm open to any alternative approaches anyone may have!
There was a problem hiding this comment.
Since all other forms use the same syntax (e.g., 'data-type' => controller_name.singularize), I suggest we only handle the special case where the controller is ComputeResourcesVms.
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
| @@ -5,5 +5,5 @@ | |||
| <% cluster_id = params.fetch(:host, {}).fetch(:compute_attributes, {}).fetch(:cluster, nil).presence %> | |||
| <%= select_f f, :network, vsphere_networks(compute_resource), :first, :last, { }, | |||
| :class => "col-md-3 vmware_network", | |||
| :size => "col-md-8", :disabled => !new_vm, | |||
There was a problem hiding this comment.
Why do we need to remove this?
There was a problem hiding this comment.
I don't understand, we are supposed to remove the :disabled right?
There was a problem hiding this comment.
No, that's why I'm asking why you chose to do that.
There was a problem hiding this comment.
The suggestion to remove the :disabled => !new_vm, option was only for testing. Now that you added the fix for this issue, we should keep it as is.
There was a problem hiding this comment.
The fix does not work without removing disabled => !new_vm
There was a problem hiding this comment.
What do you get when keeping the disabled => !new_vm?
There was a problem hiding this comment.
I tested it again today, and it is working without removing the disabled => !new_vm.
Let's connect to see what's not working for you.
No description provided.