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.

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

Fixes #37584 - Host details - Fix append domain setting #10217

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

stejskalleos
Copy link
Contributor

It doesn't work for hosts with long names like
meh-3-3.vms.meh.meh.foreman.com, and for hosts
without domain (unmanaged hosts).

@nofaralfasi
Copy link
Contributor

@stejskalleos, could you please provide a more detailed description of the problem? I'm not entirely sure what I need to check here.

@stejskalleos
Copy link
Contributor Author

Yop.

  1. In the rails console, create a host without an assigned domain:
Host.new(name: "dhcp-3-3.vms.example.something.foreman.com", managed: false, build: false, organization_id: 1, location_id: 2, ip: "192.168.24.33").save
  1. Change the append domain name setting to false
  2. Go to the new host detail

Without this change, the hostname on the new host detail page would be dhcp-3-3.vms.example.something.foreman.com. Which is wrong, it should be only dhcp-3-3.

@nofaralfasi
Copy link
Contributor

I see it only happens for hosts without a domain, which makes sense.
Can we use the to_label host's method here instead of the current implementation? (see theforeman/foreman_remote_execution#859 (comment))
Also, we have the same issue here HostDetails/index.js#L129

It doesn't work for hosts with long names like
meh-3-3.vms.meh.meh.foreman.com, and for hosts
without domain (unmanaged hosts).
@stejskalleos
Copy link
Contributor Author

@nofaralfasi rebased and updated with the display_name

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

Works well for me.
I would also suggest fixing the breadcrumb. However, it might be more complicated to fix, so we can leave it for a future PR.

@nofaralfasi
Copy link
Contributor

Update:
Regarding fixing the breadcrumb -
The value displayed in the selection menu is taken from here.
If we want to display the correct value, we need to adjust the Api::V2::HostsController#index accordingly.

@stejskalleos
Copy link
Contributor Author

stejskalleos commented Jun 27, 2024

I looked at the BreadcrumbBar, and it won't be that easy to fix IMHO. We can set the nameField on the resource, but it's then used in loadSwitcherResourcesByResource for the API call, which breaks the behavior, because for the API calls we need FQDN.

@stejskalleos
Copy link
Contributor Author

@nofaralfasi, if you are okay with it, can you merge?

Copy link

@Gauravtalreja1 Gauravtalreja1 left a comment

Choose a reason for hiding this comment

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

I've tested the fix, and it works for newly registered host with FQDN like meh-3-3.vms.meh.meh.foreman.com,
But when we've previous hosts with FQDN like meh-3-3.vms.meh.meh.foreman.com, before applying this patch, then after patch host remains same which has No Domain assigned in HostDetails page and host is displayed the same as earlier with FQDN in title and breadcrumbs

@nofaralfasi
Copy link
Contributor

I've tested the fix, and it works for newly registered host with FQDN like meh-3-3.vms.meh.meh.foreman.com, But when we've previous hosts with FQDN like meh-3-3.vms.meh.meh.foreman.com, before applying this patch, then after patch host remains same which has No Domain assigned in HostDetails page and host is displayed the same as earlier with FQDN in title and breadcrumbs

Have you tried it in a different browser?
I'm asking because I had the same issue in Chrome (my regular browser), however in Firefox it works well. So I'm guessing it has to do with the cache or something.

@nofaralfasi
Copy link
Contributor

@nofaralfasi, if you are okay with it, can you merge?

Yes :)

@nofaralfasi
Copy link
Contributor

I looked at the BreadcrumbBar, and it won't be that easy to fix IMHO. We can set the nameField on the resource, but it's then used in loadSwitcherResourcesByResource for the API call, which breaks the behavior, because for the API calls we need FQDN.

Correct. Only setting the nameField won't work; we need to add the display_name field to the Api::V2::HostsController#index, as I mentioned here.
IMHO, it shouldn't be too complicated, since we are already loading the host objects in the index method.

@stejskalleos
Copy link
Contributor Author

The thing is that the display_name is already there. ANyway, I'm merging the issue as the fix was resolved and other changes can be fixed in follow up PR

@stejskalleos stejskalleos merged commit 15ae143 into theforeman:develop Jul 1, 2024
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants