From c7e481a5389ecff4df1ad6e5a40ad6ab87aec20e 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 | 9 +++++++++ .../20230418075940_assing_fqdn_to_host_name.rb | 11 +++++++++++ .../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 +- .../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 +- 20 files changed, 69 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20230414091257_rename_append_domain_setting.rb create mode 100644 db/migrate/20230418075940_assing_fqdn_to_host_name.rb diff --git a/app/assets/javascripts/host_edit_interfaces.js b/app/assets/javascripts/host_edit_interfaces.js index f27c2760f97c..e8bbb5e38409 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 285ac8dffa0f..b3e8948ae00e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -411,6 +411,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 6aa7c02df098..213d38a5bd25 100644 --- a/app/models/host/base.rb +++ b/app/models/host/base.rb @@ -115,6 +115,7 @@ def dup :domain=, :domain_id=, :domain_name=, :to => :primary_interface attr_writer :updated_virtuals + def updated_virtuals @updated_virtuals ||= [] end @@ -351,6 +352,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 96b41976375e..53f8892d3f9d 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_('Foreman will display domain names appended to 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 dc00a585d5b4..91715c41c8bb 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 417623b70925..838c0753be65 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 c482f20851ba..8735d60cdd9d 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 def3da40e603..bf85cc97e0c3 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 b7416c426514..5283423b3b07 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 000000000000..e569ad1bd562 --- /dev/null +++ b/db/migrate/20230414091257_rename_append_domain_setting.rb @@ -0,0 +1,9 @@ +class RenameAppendDomainSetting < ActiveRecord::Migration[6.1] + def up + Setting.find_by(name: 'append_domain_name_for_hosts').update_attribute(:name, 'display_fqdn_for_hosts') + end + + def down + Setting.find_by(name: 'display_fqdn_for_hosts').update_attribute(:name, 'append_domain_name_for_hosts') + end +end diff --git a/db/migrate/20230418075940_assing_fqdn_to_host_name.rb b/db/migrate/20230418075940_assing_fqdn_to_host_name.rb new file mode 100644 index 000000000000..a7a3ce5863cb --- /dev/null +++ b/db/migrate/20230418075940_assing_fqdn_to_host_name.rb @@ -0,0 +1,11 @@ +class AssingFqdnToHostName < ActiveRecord::Migration[6.1] + def up + Host.all.each do |host| + if !host.domain.nil? && !host.name.nil? + unless host.name.ends_with? host.domain.name + host.update(name: host.fqdn) + end + 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 6df02ea4be45..de8a446c7bf1 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 930e5ae883c4..dab16c91dfd7 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 1bfab0e68ab8..3a254e138abd 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 ad002f8447d4..520e72c7614e 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/index.js b/webpack/assets/javascripts/react_app/components/HostDetails/index.js index aa6bc20f367d..282d20cf8012 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 e4cffa296a7c..8cff673b36e9 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 426570e3b6ba..4d12e0eb2ef9 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 1698947d4a0c..244ca6edcc36 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 ca0f5bb78297..4d2628c75ad4 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",