Fixes #39281 - puppet_conf.erb permit setting ca_port#10972
Fixes #39281 - puppet_conf.erb permit setting ca_port#10972jcpunk wants to merge 1 commit intotheforeman:developfrom
Conversation
jcpunk
force-pushed
the
feat-39281-caport
branch
from
f5bb4ed to
14f8c9d
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I like the idea, but you're missing the implementation of puppet_ca_server_port.
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I can't quite come up with the best phrasing, but code wise 👍.
jcpunk
force-pushed
the
feat-39281-caport
branch
2 times, most recently
from
279fcfd to
d567862
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Sorry for the stream of small comments. Slowly taking in the feature and thinking about the bigger picture.
| <%- if host_puppet_ca_server_port.present? -%> | ||
| ca_port = <%= host_puppet_ca_server_port %> | ||
| <%- end -%> |
There was a problem hiding this comment.
Thinking out loud: if a Smart Proxy is set this port will always be set. Should it only set ca_port if it is present and not 8140?
There was a problem hiding this comment.
My thinking is, if it is set - then you've done something explicit so you get something explicit.
There was a problem hiding this comment.
But if you assign a Smart Proxy as the Puppet CA it will always be set. That is IMHO the happy path that we expect all Puppet users to go through. In other words, it will be present for the majority of users and is that something we want?
There was a problem hiding this comment.
I put together a quick "is this the default" check with a hopefully clear name so it is obvious what the "magic constant" really means.
| @@ -78,6 +78,7 @@ class Configuration | |||
| :host_param, :host_param!, | |||
| :host_puppet_server, | |||
There was a problem hiding this comment.
Looking at this and wondering if for consistency we should also expose host_puppet_server_port, but I don't want to blow up the scope of this PR so feel free to leave that out.
There was a problem hiding this comment.
That is probably a good idea, but I'm unlikly to have the time in the next few weeks...
There was a problem hiding this comment.
Not my best ticket, but at least I got an issue opened : https://projects.theforeman.org/issues/39288
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
2 participants
This should permit me to run my puppet CA on an alternate port.