Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #37855 - Populate OS fields and pass correct network during import #10333

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixes #37855 - Populate OS fields and pass correct network during import #10333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/views/compute_resources_vms/form/vmware/_network.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, we are supposed to remove the :disabled right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's why I'm asking why you chose to do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:size => "col-md-8",
:label => _('Network'), :label_size => "col-md-3" %>
2 changes: 1 addition & 1 deletion app/views/hosts/_operating_system.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that's the way to go, since we are also using this form for hostgroups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using https://github.com/theforeman/foreman/blob/develop/app/views/common/os_selection/_operatingsystem.html.erb for hostgroups

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right we are not using this one, my bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@girijaasoni girijaasoni Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can override the controller_name to "host" for the compute_resources_vm controller but I guess that could maybe break things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:help_inline => :indicator, :required => true} %>

<span id="os_select">
Expand Down
Loading