Refactor the dhcp_update_required? method

[archanaserver]

Based on discussion: https://github.com/theforeman/foreman/pull/10068/files/5dc13229ccc9e02791121f055b7a7b7d4a14918e#r1699753281

Extracted complex condition into separate methods and handle the TODO comment related to jumpstart logic.

[archanaserver]

can we refactor it more like:

return true if requires_jumpstart_update?
  false

[adamruzicka]

Wouldn't even just requires_jumpstart_update? do the trick? Or double-negate it to cast it from a truthy/falsey value into a bool?

[ekohl]

What I meant was that perhaps it already is properly abstracted away.

The whole method tries to determine if the DHCP record needs to be updated. That is if any parameter has changed. We Net::DHCP::Record and subclasses in lib/net/dhcp.

These are called in ProxyAPI::DHCP:

foreman/app/services/proxy_api/dhcp.rb

Lines 41 to 57 in 2f947f7

	# Retrieves a DHCP entry for a mac
	# [+subnet+] : String in dotted decimal format
	# [+mac+] : String in coloned sextuplet format
	# Returns : Hash or false
	def record(subnet, mac)
	response = parse(get("#{subnet}/mac/#{mac}"))
	attrs = response.merge(:network => subnet, :proxy => self)
	if response.keys.grep(/Sun/i).empty?
	Net::DHCP::Record.new attrs
	else
	Net::DHCP::SparcRecord.new attrs
	end
	rescue RestClient::ResourceNotFound
	nil
	rescue => e
	raise ProxyException.new(url, e, N_("Unable to retrieve DHCP entry for %s"), mac)
	end

And indirectly in orchestration:

foreman/app/models/concerns/orchestration/dhcp.rb

Lines 101 to 109 in 2f947f7

	def build_dhcp_record(record_mac)
	raise ::Foreman::Exception.new(N_("DHCP not supported for this NIC")) unless dhcp?
	record_attrs = dhcp_attrs(record_mac)
	record_type = operatingsystem.dhcp_record_type
	handle_validation_errors do
	record_type.new(record_attrs)
	end
	end

The latter one is more relevant (it's also this file), so dhcp_update_required? should compare all attributes that dhcp_attrs can return.

Note it has this bit already:

foreman/app/models/concerns/orchestration/dhcp.rb

Lines 129 to 130 in 2f947f7

	if jumpstart?
	jumpstart_arguments = os.jumpstart_params host, model.vendor_class

So I'd expect you can compare os.jumpstart_params and old.os.jumpstart_params.

[adamruzicka]

Just to be extra sure I'm not missing anything, this just extracts a chunk of code into a method, correct? There are no actual changes to what are all the things that get checked, right?

Edit: somehow I haven't seen Ewoud's previous comment, oh well

[adamruzicka]

Wouldn't even just requires_jumpstart_update? do the trick? Or double-negate it to cast it from a truthy/falsey value into a bool?