[WIP] Fixes #20852 - Refactor module #136
Conversation
|
@ekohl: I know that is is not meant for a review. But I revied it anyways. This looks already pretty good to me and I'd love to have this as a base for further improvement. What do you think is missing here? |
|
@timogoebel I don't recall if this current iteration actually worked. Other than that I want to rename the |
|
@ekohl: Maybe we can extract that change first to make this less complex. I'm working on a PR to separate the upstreams for qpid and the reverse proxy. I'll send that over in a bit. I'd like if this PR would be just refactoring without changing anything. |
|
@timogoebel that's always a good idea. |
|
@timogoebel rebased. It's not split into a bit more separate commits. I still feel like the pulp refactoring could be split up further for easier review but I think the first commits can be merged already. |
ekohl
force-pushed
the
refactor
branch
3 times, most recently
from
7528bf0 to
6e2beea
Compare
|
|
||
| $pulp = $::foreman_proxy::plugin::pulp::pulpnode_enabled | ||
| $pulp_node = $::foreman_proxy::plugin::pulp::pulpnode_enabled |
There was a problem hiding this comment.
We really should replace the node nomenclature.
There was a problem hiding this comment.
Indeed, but what should be the correct name?
There was a problem hiding this comment.
I reached out to pulp-dev list to look for commonly accepted nomenclature. The best I could arrive at is 'parent' and 'child' from the responses I got.
There was a problem hiding this comment.
Given in how many places this naming is currently used, I'm leaning to keeping that outside of this refactor.
| String $broker_host = $::foreman_proxy_content::qpid_router_broker_addr, | ||
| Integer[0, 65535] $broker_port = $::foreman_proxy_content::qpid_router_broker_port, | ||
| ) { | ||
| if $qpid_router { |
There was a problem hiding this comment.
Maybe this should be it's own manifest? Pulp does not actually talk to the router and you may choose to deploy the router separately from your Pulp host or even your qpid host. That is, you could have Pulp server, Qpid server and Qdrouter running all separately or router and qpid paired.
| ca_cert => $::certs::ca_cert, | ||
| data_dir => '/var/lib/pulp/published/docker/v2/app', | ||
| subscribe => Class['certs::apache'], | ||
| } |
There was a problem hiding this comment.
This can run on a Smart proxy as well serving up docker content off the Pulp server there.
There was a problem hiding this comment.
That's why I added enable_crane there.
| # The pulp node configuration | ||
| # | ||
| # In this scenario we assume there is another pulp instance somewhere and this | ||
| # talks to a qpid router somewhere. |
There was a problem hiding this comment.
This is technically speaking incorrect. This connection diagram may help with wrapping your brain around all the connection points.
The dispatch router serves as a public interface for services that need to talk to qpid. Generally speaking thats only clients talking to dispatch router which then talks to either Qpid directly (on a server) or talks to another dispatch router (on a smart proxy). Currently Pulp talks directly to qpid over localhost. If we worked on a spread deployment and split out qpid we'd need to consider having Pulp talk to the router but for current setups it does not.
manifests/pulp/node.pp
Outdated
| owner => 'root', | ||
| group => 'root', | ||
| mode => '0644', | ||
| } |
There was a problem hiding this comment.
This block can be dropped from sight including the template.
|
|
||
| include ::certs::apache | ||
| include ::certs::qpid_client | ||
| class { '::pulp': |
There was a problem hiding this comment.
Is it worth considering centralizing Pulp install and configuration in one place at this juncture? I don't want to over complicate this large re-factor but as we've pointed out in the past we setup Pulp here and in puppet-katello.
There was a problem hiding this comment.
I considered moving it all into puppet-katello, but you're right we have overlap. We should at least centralize pulp in one place.
Since all notifies are handled inside the class, they don't have to be duplicated outside of the class. We also default to the correct variables so no need to explicitly initialize.
Since certs::katello now inherits from certs, this require is no longer needed.
While it looks like it's no longer possible to override the proxy hostname on the cert, this can now be done through the certs module.
|
Closing this now but will pick this up via other PRs. |
This is not ready for review at all and only meant for me to run the forklift pipeline with it.