set correct entries for Puppet Master and CA entries during execution #262
Conversation
|
@evgeni can you help me with this PR |
|
@snagoor what kind of help do you need here? the code looks sane on the first glance. |
|
@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. |
|
@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 However, I fear there is a conceptual problem here. You expect that What we could do is to call What do you think? |
|
@snagoor ping? :) |
snagoor
force-pushed
the
fix-puppet-ca-master
branch
from
ff8b305
to
f1299b9
Compare
snagoor
force-pushed
the
fix-puppet-ca-master
branch
from
f1299b9
to
a5d738b
Compare
| print_running("Calling Foreman API to update Puppet master and Puppet CA for %s to %s" % (FQDN, options.foreman_fqdn)) | ||
| update_host_capsule_mapping("puppet_proxy_id", smart_proxy_id, current_host_id) | ||
| update_host_capsule_mapping("puppet_ca_proxy_id", smart_proxy_id, current_host_id) | ||
| if puppet_master_smart_proxy_id is not None and puppet_ca_smart_proxy_id is not None: |
There was a problem hiding this comment.
you can write this without the is not None part, which I find better readable
| 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: |
| update_host_capsule_mapping("puppet_proxy_id", puppet_master_smart_proxy_id, current_host_id) | ||
| print_running("Calling Foreman API to update Puppet CA for %s to %s" % (FQDN, options.puppet_ca_server)) | ||
| update_host_capsule_mapping("puppet_ca_proxy_id", puppet_ca_smart_proxy_id, current_host_id) | ||
| else: |
There was a problem hiding this comment.
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?
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 toforeman_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.