From a406cf2b64ebe36ce2cd72fd3fc59e45f956fb62 Mon Sep 17 00:00:00 2001 From: Dyrkon Date: Thu, 2 Feb 2023 09:57:05 +0100 Subject: [PATCH] fixes #36160 - Redefine append domain names setting 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. --- app/assets/javascripts/host_edit_interfaces.js | 3 --- app/helpers/application_helper.rb | 1 + app/models/host/base.rb | 9 +++++++++ app/registries/foreman/settings/general.rb | 6 +++--- app/services/name_synchronizer.rb | 2 +- app/views/api/v2/hosts/base.json.rabl | 4 ++++ ..._hosts_in_build_mode_widget_host_list.html.erb | 2 +- .../_new_hosts_widget_host_list.html.erb | 2 +- app/views/hosts/_form.html.erb | 2 +- ...20230414091257_rename_append_domain_setting.rb | 5 +++++ .../20230418075940_assign_fqdn_to_host_name.rb | 9 +++++++++ .../api/v2/settings_controller_test.rb | 10 +++++----- test/integration/host_js_test.rb | 4 ++-- test/models/lookup_value_test.rb | 13 ------------- test/unit/name_synchronizer_test.rb | 2 +- .../Tabs/Details/Cards/SystemProperties/index.js | 4 +++- .../react_app/components/HostDetails/index.js | 15 +++++++++++++-- .../__tests__/SettingRecords.fixtures.js | 4 ++-- .../SettingRecordsReducer.test.js.snap | 4 ++-- .../SettingRecordsSelectors.test.js.snap | 4 ++-- .../__snapshots__/SettingsTable.test.js.snap | 2 +- 21 files changed, 66 insertions(+), 41 deletions(-) create mode 100644 db/migrate/20230414091257_rename_append_domain_setting.rb create mode 100644 db/migrate/20230418075940_assign_fqdn_to_host_name.rb diff --git a/app/assets/javascripts/host_edit_interfaces.js b/app/assets/javascripts/host_edit_interfaces.js index f27c2760f97..e8bbb5e3840 100644 --- a/app/assets/javascripts/host_edit_interfaces.js +++ b/app/assets/javascripts/host_edit_interfaces.js @@ -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(); diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 7b47676899f..364622423cc 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -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 diff --git a/app/models/host/base.rb b/app/models/host/base.rb index b79ef93f9f6..91cec5dadc3 100644 --- a/app/models/host/base.rb +++ b/app/models/host/base.rb @@ -118,6 +118,7 @@ def dup :domain=, :domain_id=, :domain_name=, :to => :primary_interface attr_writer :updated_virtuals + def updated_virtuals @updated_virtuals ||= [] end @@ -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) diff --git a/app/registries/foreman/settings/general.rb b/app/registries/foreman/settings/general.rb index 96b41976375..a30a0b7eb30 100644 --- a/app/registries/foreman/settings/general.rb +++ b/app/registries/foreman/settings/general.rb @@ -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. ' \ diff --git a/app/services/name_synchronizer.rb b/app/services/name_synchronizer.rb index dc00a585d5b..91715c41c8b 100644 --- a/app/services/name_synchronizer.rb +++ b/app/services/name_synchronizer.rb @@ -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 diff --git a/app/views/api/v2/hosts/base.json.rabl b/app/views/api/v2/hosts/base.json.rabl index 417623b7092..838c0753be6 100644 --- a/app/views/api/v2/hosts/base.json.rabl +++ b/app/views/api/v2/hosts/base.json.rabl @@ -1,3 +1,7 @@ object @host attributes :name, :id + +node :display_name do |host| + host.to_label +end diff --git a/app/views/dashboard/_hosts_in_build_mode_widget_host_list.html.erb b/app/views/dashboard/_hosts_in_build_mode_widget_host_list.html.erb index c482f20851b..8735d60cdd9 100644 --- a/app/views/dashboard/_hosts_in_build_mode_widget_host_list.html.erb +++ b/app/views/dashboard/_hosts_in_build_mode_widget_host_list.html.erb @@ -8,7 +8,7 @@ <% hosts.each do |host| %> - <%= host_build_status_icon(host) %> <%= link_to host.name, current_host_details_path(host) %> + <%= host_build_status_icon(host) %> <%= link_to host, current_host_details_path(host) %> <%= host.owner %> <%= build_duration(host) %> <%= date_time_relative(host.token&.expires)%> diff --git a/app/views/dashboard/_new_hosts_widget_host_list.html.erb b/app/views/dashboard/_new_hosts_widget_host_list.html.erb index def3da40e60..bf85cc97e0c 100644 --- a/app/views/dashboard/_new_hosts_widget_host_list.html.erb +++ b/app/views/dashboard/_new_hosts_widget_host_list.html.erb @@ -9,7 +9,7 @@ <% hosts.each do |host| %> - <%= link_to host.name, current_host_details_path(host) %> + <%= link_to host, current_host_details_path(host) %> <%= safe_join([icon(host.operatingsystem, :size => '16x16'), host.operatingsystem.to_label]) if host.operatingsystem.present? %> <%= host.owner %> <%= date_time_relative(host.created_at) %> diff --git a/app/views/hosts/_form.html.erb b/app/views/hosts/_form.html.erb index b7416c42651..5283423b3b0 100644 --- a/app/views/hosts/_form.html.erb +++ b/app/views/hosts/_form.html.erb @@ -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? %> diff --git a/db/migrate/20230414091257_rename_append_domain_setting.rb b/db/migrate/20230414091257_rename_append_domain_setting.rb new file mode 100644 index 00000000000..2547b496a2f --- /dev/null +++ b/db/migrate/20230414091257_rename_append_domain_setting.rb @@ -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 diff --git a/db/migrate/20230418075940_assign_fqdn_to_host_name.rb b/db/migrate/20230418075940_assign_fqdn_to_host_name.rb new file mode 100644 index 00000000000..42248a7f753 --- /dev/null +++ b/db/migrate/20230418075940_assign_fqdn_to_host_name.rb @@ -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 diff --git a/test/controllers/api/v2/settings_controller_test.rb b/test/controllers/api/v2/settings_controller_test.rb index 6df02ea4be4..de8a446c7bf 100644 --- a/test/controllers/api/v2/settings_controller_test.rb +++ b/test/controllers/api/v2/settings_controller_test.rb @@ -8,7 +8,7 @@ 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'] @@ -16,8 +16,8 @@ def setup 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 @@ -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'] diff --git a/test/integration/host_js_test.rb b/test/integration/host_js_test.rb index 930e5ae883c..dab16c91dfd 100644 --- a/test/integration/host_js_test.rb +++ b/test/integration/host_js_test.rb @@ -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) diff --git a/test/models/lookup_value_test.rb b/test/models/lookup_value_test.rb index 1bfab0e68ab..3a254e138ab 100644 --- a/test/models/lookup_value_test.rb +++ b/test/models/lookup_value_test.rb @@ -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 diff --git a/test/unit/name_synchronizer_test.rb b/test/unit/name_synchronizer_test.rb index ad002f8447d..520e72c7614 100644 --- a/test/unit/name_synchronizer_test.rb +++ b/test/unit/name_synchronizer_test.rb @@ -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 diff --git a/webpack/assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/SystemProperties/index.js b/webpack/assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/SystemProperties/index.js index c1b7e0b3e09..713caf973e1 100644 --- a/webpack/assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/SystemProperties/index.js +++ b/webpack/assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/SystemProperties/index.js @@ -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, @@ -48,7 +50,7 @@ const SystemPropertiesCard = ({ status, hostDetails }) => { hoverTip={__('Copy to clipboard')} clickTip={__('Copied to clipboard')} > - {name} + {displayFqdnForHosts ? name : name?.replace(`.${domain}`, '')} )} diff --git a/webpack/assets/javascripts/react_app/components/HostDetails/index.js b/webpack/assets/javascripts/react_app/components/HostDetails/index.js index aa6bc20f367..282d20cf801 100644 --- a/webpack/assets/javascripts/react_app/components/HostDetails/index.js +++ b/webpack/assets/javascripts/react_app/components/HostDetails/index.js @@ -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: { @@ -51,6 +52,7 @@ const HostDetails = ({ location: { hash }, history, }) => { + const { displayFqdnForHosts } = useForemanSettings(); const { response, status } = useAPI( 'get', `/api/hosts/${id}?show_hidden_parameters=true`, @@ -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}`, ''), + }, ]} /> )} @@ -136,7 +142,12 @@ const HostDetails = ({ headingLevel="h5" size="2xl" > - {response.name} + {displayFqdnForHosts + ? response.name + : response.name?.replace( + `.${response.domain_name}`, + '' + )} )} diff --git a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/SettingRecords.fixtures.js b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/SettingRecords.fixtures.js index e4cffa296a7..8cff673b36e 100644 --- a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/SettingRecords.fixtures.js +++ b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/SettingRecords.fixtures.js @@ -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, @@ -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' diff --git a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsReducer.test.js.snap b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsReducer.test.js.snap index 426570e3b6b..4d12e0eb2ef 100644 --- a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsReducer.test.js.snap +++ b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsReducer.test.js.snap @@ -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", @@ -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", diff --git a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsSelectors.test.js.snap b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsSelectors.test.js.snap index 1698947d4a0..244ca6edcc3 100644 --- a/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsSelectors.test.js.snap +++ b/webpack/assets/javascripts/react_app/components/SettingRecords/__tests__/__snapshots__/SettingRecordsSelectors.test.js.snap @@ -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", @@ -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", diff --git a/webpack/assets/javascripts/react_app/components/SettingsTable/__tests__/__snapshots__/SettingsTable.test.js.snap b/webpack/assets/javascripts/react_app/components/SettingsTable/__tests__/__snapshots__/SettingsTable.test.js.snap index ca0f5bb7829..4d2628c75ad 100644 --- a/webpack/assets/javascripts/react_app/components/SettingsTable/__tests__/__snapshots__/SettingsTable.test.js.snap +++ b/webpack/assets/javascripts/react_app/components/SettingsTable/__tests__/__snapshots__/SettingsTable.test.js.snap @@ -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",