Skip to content

Commit

Permalink
fixes #36160 - Redefine append domain names setting
Browse files Browse the repository at this point in the history
This PR aims to unify the format of host names stored in the database and the way they are displayed.
With this change, the name of the host is always going to be stored with the domain name appended.
The setting formerly named `append_domain_name_for_hosts` is now renamed to `display_fqdn_for_hosts`
because it will only impact how the names are displayed from now.
This means dashboards and breadcrumbs are going to display the whole FQDN if you choose to.
  • Loading branch information
Dyrkon authored and stejskalleos committed Oct 9, 2023
1 parent 5648302 commit a406cf2
Show file tree
Hide file tree
Showing 21 changed files with 66 additions and 41 deletions.
3 changes: 0 additions & 3 deletions app/assets/javascripts/host_edit_interfaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,9 +391,6 @@ $(document).on('change', '.virtual', function() {
function construct_host_name() {
var host_name_el = $('#host_name')
var host_name = host_name_el.val();
if (host_name_el.data('appendDomainNameForHosts') === false) {
return host_name;
}
var domain_name = primary_nic_form()
.find('.interface_domain option:selected')
.text();
Expand Down
1 change: 1 addition & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ def ui_settings
destroyVmOnHostDelete: Setting['destroy_vm_on_host_delete'],
labFeatures: Setting[:lab_features],
safeMode: Setting[:safemode_render],
displayFqdnForHosts: Setting[:display_fqdn_for_hosts],
}
end

Expand Down
9 changes: 9 additions & 0 deletions app/models/host/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def dup
:domain=, :domain_id=, :domain_name=, :to => :primary_interface

attr_writer :updated_virtuals

def updated_virtuals
@updated_virtuals ||= []
end
Expand Down Expand Up @@ -367,6 +368,14 @@ def render_template(template:, **params)
template.render(host: self, **params)
end

def to_label
if Setting[:display_fqdn_for_hosts]
name
else
name.split('.')[0]
end
end

private

def parse_ip_address(address, ignore_link_local: true)
Expand Down
6 changes: 3 additions & 3 deletions app/registries/foreman/settings/general.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@
description: N_("Whether or not to show a menu to access experimental lab features (requires reload of page)"),
default: false,
full_name: N_('Show Experimental Labs'))
setting('append_domain_name_for_hosts',
setting('display_fqdn_for_hosts',
type: :boolean,
description: N_('Foreman will append domain names when new hosts are provisioned'),
description: N_('Display names of hosts as FQDNs. If disabled, only display names of hosts as hostnames.'),
default: true,
full_name: N_('Append domain names to the host'))
full_name: N_('Display FQDN for hosts'))
setting('outofsync_interval',
type: :integer,
description: N_('Duration in minutes after servers are classed as out of sync. ' \
Expand Down
2 changes: 1 addition & 1 deletion app/services/name_synchronizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ def sync_required?
private

def interface_name
Setting[:append_domain_name_for_hosts] ? @interface.name : @interface.shortname
@interface.name
end
end
4 changes: 4 additions & 0 deletions app/views/api/v2/hosts/base.json.rabl
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
object @host

attributes :name, :id

node :display_name do |host|
host.to_label
end
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<tbody>
<% hosts.each do |host| %>
<tr>
<td class="ellipsis"><%= host_build_status_icon(host) %> <%= link_to host.name, current_host_details_path(host) %></td>
<td class="ellipsis"><%= host_build_status_icon(host) %> <%= link_to host, current_host_details_path(host) %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= host.owner %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= build_duration(host) %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= date_time_relative(host.token&.expires)%></td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/dashboard/_new_hosts_widget_host_list.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<tbody>
<% hosts.each do |host| %>
<tr>
<td class="ellipsis"><%= link_to host.name, current_host_details_path(host) %></td>
<td class="ellipsis"><%= link_to host, current_host_details_path(host) %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= safe_join([icon(host.operatingsystem, :size => '16x16'), host.operatingsystem.to_label]) if host.operatingsystem.present? %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= host.owner %></td>
<td class="hidden-tablet hidden-xs ellipsis"><%= date_time_relative(host.created_at) %></td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/hosts/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<%= text_f f, :name, :size => "col-md-4", :value => name_field(@host),
:input_group_btn => randomize_mac_link,
:help_inline => _("This value is used also as the host's primary interface name."),
:data => { 'append_domain_name_for_hosts' => Setting[:append_domain_name_for_hosts] }
:data => {}
%>

<% if show_organization_tab? %>
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20230414091257_rename_append_domain_setting.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RenameAppendDomainSetting < ActiveRecord::Migration[6.1]
def change
Setting.find_by(name: 'append_domain_name_for_hosts')&.update_attribute(:name, 'display_fqdn_for_hosts')
end
end
9 changes: 9 additions & 0 deletions db/migrate/20230418075940_assign_fqdn_to_host_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class AssignFqdnToHostName < ActiveRecord::Migration[6.1]
def up
Host.find_in_batches(batch_size: 1000) do |hosts|
hosts.each do |host|
host.update_attribute(:name, host.fqdn)
end
end
end
end
10 changes: 5 additions & 5 deletions test/controllers/api/v2/settings_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ def setup
end

test "should get all settings through index" do
Setting['append_domain_name_for_hosts'] = false
Setting['display_fqdn_for_hosts'] = false
get :index, params: { per_page: 'all' }
assert_response :success
settings = ActiveSupport::JSON.decode(@response.body)['results']
assert_equal Foreman.settings.count, settings.count
foreman_url = settings.detect { |s| s['name'] == 'foreman_url' }
assert_equal Setting['foreman_url'], foreman_url['value']
assert_equal Foreman.settings.find('foreman_url').default, foreman_url['default']
append_domain_name_for_hosts = settings.detect { |s| s['name'] == 'append_domain_name_for_hosts' }
assert_equal false, append_domain_name_for_hosts['value']
display_fqdn_for_hosts = settings.detect { |s| s['name'] == 'display_fqdn_for_hosts' }
assert_equal false, display_fqdn_for_hosts['value']
end

test "should get index with organization and location params" do
Expand Down Expand Up @@ -68,8 +68,8 @@ def setup
end

test "properly show overriden false value" do
Setting['append_domain_name_for_hosts'] = value = false
get :show, params: { :id => 'append_domain_name_for_hosts' }
Setting['display_fqdn_for_hosts'] = value = false
get :show, params: { :id => 'display_fqdn_for_hosts' }
assert_response :success
show_response = ActiveSupport::JSON.decode(@response.body)
assert_equal value, show_response['value']
Expand Down
4 changes: 2 additions & 2 deletions test/integration/host_js_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ class HostJSTest < IntegrationTestWithJavascript
find('h5', :text => /newhost2.*/) # wait for the new host details page
end

test "redirects correctly with append_domain_name_for_hosts turned off" do
Setting['append_domain_name_for_hosts'] = false
test "redirects correctly with display_fqdn_for_hosts turned off" do
Setting['display_fqdn_for_hosts'] = false
compute_resource = FactoryBot.create(:compute_resource, :libvirt)
os = FactoryBot.create(:ubuntu14_10, :with_associations)
Nic::Managed.any_instance.stubs(:dns_conflict_detected?).returns(true)
Expand Down
13 changes: 0 additions & 13 deletions test/models/lookup_value_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,6 @@ def valid_attrs2
end
end

test "can create lookup value if match fqdn= does match existing host" do
as_admin do
Setting[:append_domain_name_for_hosts] = false
domain = FactoryBot.create(:domain)
host = FactoryBot.create(:host, interfaces: [FactoryBot.build(:nic_managed, identifier: 'fqdn_test', primary: true, domain: domain)])
attrs = { :match => "fqdn=#{host.primary_interface.fqdn}", :value => "123", :lookup_key_id => lookup_key.id }
refute_match /#{domain.name}/, host.name, "#{host.name} shouldn't be FQDN"
assert_difference('LookupValue.count') do
LookupValue.create!(attrs)
end
end
end

test "can create lookup value if user has matching hostgroup " do
attrs = valid_attrs2 # create key outside as_user
as_user :one do
Expand Down
2 changes: 1 addition & 1 deletion test/unit/name_synchronizer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def setup

context "synchronizer build from host on shortnames" do
before do
Setting[:append_domain_name_for_hosts] = false
Setting[:display_fqdn_for_hosts] = false
end
test "#sync_required? detects difference between names" do
refute_equal @host.name, @host.primary_interface.shortname
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import Slot from '../../../../../common/Slot';
import SkeletonLoader from '../../../../../common/SkeletonLoader';
import DefaultLoaderEmptyState from '../../../../DetailsCard/DefaultLoaderEmptyState';
import { STATUS } from '../../../../../../constants';
import { useForemanSettings } from '../../../../../../Root/Context/ForemanContext';

const SystemPropertiesCard = ({ status, hostDetails }) => {
const { displayFqdnForHosts } = useForemanSettings();
const {
name,
model_name: model,
Expand Down Expand Up @@ -48,7 +50,7 @@ const SystemPropertiesCard = ({ status, hostDetails }) => {
hoverTip={__('Copy to clipboard')}
clickTip={__('Copied to clipboard')}
>
{name}
{displayFqdnForHosts ? name : name?.replace(`.${domain}`, '')}
</ClipboardCopy>
)}
</SkeletonLoader>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import BreadcrumbBar from '../BreadcrumbBar';
import { foremanUrl } from '../../common/helpers';
import { CardExpansionContextWrapper } from './CardExpansionContext';
import Head from '../Head';
import { useForemanSettings } from '../../Root/Context/ForemanContext';

const HostDetails = ({
match: {
Expand All @@ -51,6 +52,7 @@ const HostDetails = ({
location: { hash },
history,
}) => {
const { displayFqdnForHosts } = useForemanSettings();
const { response, status } = useAPI(
'get',
`/api/hosts/${id}?show_hidden_parameters=true`,
Expand Down Expand Up @@ -112,7 +114,11 @@ const HostDetails = ({
}}
breadcrumbItems={[
{ caption: __('Hosts'), url: foremanUrl('/hosts') },
{ caption: response.name },
{
caption: displayFqdnForHosts
? response.name
: response.name?.replace(`.${response.domain_name}`, ''),
},
]}
/>
)}
Expand All @@ -136,7 +142,12 @@ const HostDetails = ({
headingLevel="h5"
size="2xl"
>
{response.name}
{displayFqdnForHosts
? response.name
: response.name?.replace(
`.${response.domain_name}`,
''
)}
</Title>
)}
</SkeletonLoader>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const settings = [
updatedAt: '2018-01-22 14:03:38 +0100',
readonly: false,
id: 177,
name: 'append_domain_name_for_hosts',
name: 'display_fqdn_for_hosts',
fullName: 'Append domain names to the host',
selectValues: null,
value: true,
Expand Down Expand Up @@ -250,7 +250,7 @@ export const withHashSelection = settings.find(
item => item.name === 'global_PXELinux'
);
export const boolSetting = settings.find(
item => item.name === 'append_domain_name_for_hosts'
item => item.name === 'display_fqdn_for_hosts'
);
export const arraySetting = settings.find(
item => item.name === 'http_proxy_except_list'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Object {
"encrypted": false,
"fullName": "Append domain names to the host",
"id": 177,
"name": "append_domain_name_for_hosts",
"name": "display_fqdn_for_hosts",
"readonly": false,
"selectValues": null,
"settingsType": "boolean",
Expand Down Expand Up @@ -296,7 +296,7 @@ Object {
"encrypted": false,
"fullName": "Append domain names to the host",
"id": 177,
"name": "append_domain_name_for_hosts",
"name": "display_fqdn_for_hosts",
"readonly": false,
"selectValues": null,
"settingsType": "boolean",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Object {
"encrypted": false,
"fullName": "Append domain names to the host",
"id": 177,
"name": "append_domain_name_for_hosts",
"name": "display_fqdn_for_hosts",
"readonly": false,
"selectValues": null,
"settingsType": "boolean",
Expand Down Expand Up @@ -299,7 +299,7 @@ Array [
"encrypted": false,
"fullName": "Append domain names to the host",
"id": 177,
"name": "append_domain_name_for_hosts",
"name": "display_fqdn_for_hosts",
"readonly": false,
"selectValues": null,
"settingsType": "boolean",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ exports[`SettingsTable should render 1`] = `
"encrypted": false,
"fullName": "Append domain names to the host",
"id": 177,
"name": "append_domain_name_for_hosts",
"name": "display_fqdn_for_hosts",
"readonly": false,
"selectValues": null,
"settingsType": "boolean",
Expand Down

0 comments on commit a406cf2

Please sign in to comment.