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

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/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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@girijaasoni any thoughts on 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.

That's exactly what we are doing, we can override it as well but its just the same approach

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, maybe we could make this clearer by adding a condition like this:

Suggested change
{: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.

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

<span id="os_select">
Expand Down
Loading