Fixes #32766 - adds scheme and pulp content path to setting value #200
Conversation
manifests/plugin/ansible.pp
Outdated
| @@ -16,7 +16,7 @@ | |||
| } | |||
|
|
|||
| pulpcore::plugin { 'ansible': | |||
| config => "ANSIBLE_API_HOSTNAME = \"${pulpcore::servername}\"\nANSIBLE_CONTENT_HOSTNAME = \"${pulpcore::servername}\"", | |||
| config => "ANSIBLE_API_HOSTNAME = \"${pulpcore::servername}\"\nANSIBLE_CONTENT_HOSTNAME = \"https://${pulpcore::servername}/pulp/content/\"", | |||
There was a problem hiding this comment.
I don't like constructing a URL again and would prefer to reuse.
We have this here:
puppet-pulpcore/manifests/apache.pp
Line 19 in 22db71c
What we should do is construct the URL. Technically speaking it should also take $https_port into account and fail if https is turned off. So roughly:
if ! $pulpcore::apache_https_vhost {
# TODO: or can it be served over HTTP?
fail('HTTPS must be turned on for Ansible content')
} elsif $pulpcore::apache::https_port != 443 {
# Technically we can't determine this when this module doesn't manage the vhost
# It's up to the user to keep them in sync
$external_content_url = "https://${pulpcore::servername}:${pulpcore::apache::https_port}${pulpcore::apache::content_path}"
} else {
$external_content_url = "https://${pulpcore::servername}${pulpcore::apache::content_path}"
}We're not so complete here:
It may be an idea to add a $external_content_url in apache.pp so that it can be used in puppet-foreman_proxy_content too.
There was a problem hiding this comment.
It may be an idea to add a
$external_content_urlinapache.ppso that it can be used in puppet-foreman_proxy_content too.
I like this idea, and I think it's best that we tackle the change in apache.pp it in a separate PR.
We could fix the immediate issue here, then improve the cross-module implementation details in a PR that is "Refs #32766 - ..." (really two separate PRs, since we'd also want to update puppet-FPC at that point)
spec/classes/plugin_ansible_spec.rb
Outdated
| @@ -25,7 +25,7 @@ | |||
| .with_content(expected_vhost_content) | |||
| is_expected.to contain_concat__fragment('plugin-ansible') | |||
| .with_content(/^ANSIBLE_API_HOSTNAME = "foo.example.com"/) | |||
| .with_content(/^ANSIBLE_CONTENT_HOSTNAME = "foo.example.com"/) | |||
| .with_content(%r{^ANSIBLE_CONTENT_HOSTNAME = "https://foo.example.com/pulp/content/"}) | |||
There was a problem hiding this comment.
The percent literal is a lot nicer here. 👍
manifests/plugin/ansible.pp
Outdated
| pulpcore::plugin { 'ansible': | ||
| config => "ANSIBLE_API_HOSTNAME = \"${pulpcore::servername}\"\nANSIBLE_CONTENT_HOSTNAME = \"${pulpcore::servername}\"", | ||
| config => "ANSIBLE_API_HOSTNAME = \"${pulpcore::servername}\"\nANSIBLE_CONTENT_HOSTNAME = \"${external_content_url}/\"", |
There was a problem hiding this comment.
Just wondering: does it need a trailing slash?
There was a problem hiding this comment.
I assumed it was needed. However, I just tested this without a trailing slash. It appears that pulp3 handles this without the trailing slash without any issues.
The current value omits the scheme and pulp content path, which causes issues when syncing content across capsules (and probably will cause other failures too).