set correct entries for Puppet Master and CA entries during execution

[snagoor]

With #250, we now have the ability to set the arbitrary Puppet Master and CA options, but the current codebase sets both these values to smart_proxy_id, which in turn defaults to foreman_fqdn.

With this new change, added a couple of new entries to pull the right Puppet Master and CA Smart proxy IDs and set them accordingly.

[snagoor]

@evgeni can you help me with this PR

[evgeni]

@snagoor what kind of help do you need here? the code looks sane on the first glance.

[snagoor]

@evgeni, Sorry for the confusion. What I meant was to review this code change at your leisure and let me know if there are any changes required.

[evgeni]

@snagoor ah :)

So the code itself looks correct. We could argue if we want to "optimize" the runtime by only querying the IDs if the hostnames are different to foreman_fqdn, but I don't really think it's worth the effort.

However, I fear there is a conceptual problem here. You expect that puppet_server and puppet_ca_server exist in Foreman as Smart Proxies. But there is really no requirement for that. The user might very well run an external Puppet Server, or use a load-balanced hostname for it as we describe in Load-Balanced Smart Proxies with HAProxy. In both cases, you will get no results from the API.

What we could do is to call return_matching_foreman_key with null_result_ok=True and only call update_host_capsule_mapping if we actually got an id and not None.

What do you think?

[evgeni]

see comment above

[evgeni]

@snagoor ping? :)

[snagoor]

@snagoor ping? :)

Sorry, @evgeni for the delay. I will work on your suggestion

[evgeni]

you can write this without the is not None part, which I find better readable

Suggested change

	if puppet_master_smart_proxy_id is not None and puppet_ca_smart_proxy_id is not None:
	if puppet_master_smart_proxy_id and puppet_ca_smart_proxy_id:

[evgeni]

I wonder if we need this branching here. The if/else is only here to generate slightly different output, right? I think the user won't lose much if we drop the to %s part of the messages and then we can drop the whole if/else completely if we set puppet_master_smart_proxy_id and puppet_ca_smart_proxy_id to smart_proxy_id if the search didn't return anything.

Now, after writing this, I wonder if we need to set these to the Smart Proxy at all if the user asked for an external (not-found) Puppet setup. Because if the user bootstraps a machine against foreman.example.com, but is using puppet.example.com for Puppet Master and CA, why would we set the Puppet options in Foreman to foreman.example.com? That'd be wrong, no?

@sideangleside thoughts?