Fixes #37855 - Populate OS fields and pass correct network during import #10333
Conversation
| @@ -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 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.
There was a problem hiding this comment.
@girijaasoni any thoughts on this?
There was a problem hiding this comment.
That's exactly what we are doing, we can override it as well but its just the same approach
There was a problem hiding this comment.
I meant, maybe we could make this clearer by adding a condition like this:
| {:onchange => 'architecture_selected(this);', :'data-url' => method_path('architecture_selected'), :'data-type' => "host", | |
| {:onchange => 'architecture_selected(this);', :'data-url' => method_path('architecture_selected'), :'data-type' => controller_name.singularize == 'compute_resources_vm' ? 'host' : controller_name.singularize, |
Since controller_name.singularize is already used elsewhere in the code, it might make sense to keep using it consistently here too.
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
No description provided.