Refs #28922: Communicate via localhost to Candlepin #321

ehelms · 2020-02-04T20:22:35Z

No description provided.

ehelms · 2020-02-24T18:55:47Z

I added a candlepin_host parameter and used that as the driver for configuration as it seemed more natural than going through a certs option

ekohl

Overall this looks like it's heading in the right direction

ekohl · 2020-02-26T16:29:56Z

manifests/application.pp

@@ -55,7 +55,7 @@
  $enable_deb = $katello::params::enable_deb
  $pulp_url = $katello::params::pulp_url
  $pulp_ca_cert = $certs::katello_server_ca_cert # TODO: certs::apache::...
-  $candlepin_url = $katello::params::candlepin_url
+  $candlepin_url = "https://${katello::candlepin_host}:8443/candlepin"


The katello::application should not assume the katello class exists. I should write a PR, but after 2.1 I want to deprecate it altogether. Anything that's shared between katello::application and katello::candlepin should go through katello::params (as the URL was previously). You can then remove the parameter from init.pp

I think I follow what you are suggesting. So check out this line and change now... and let me know if I got there or not.

ekohl · 2020-02-26T19:34:33Z

This is a valid way, but if you look at theforeman/foreman-installer#421 there's a few layers:

katello::global - all parameters on this class are exposed to installer users as --global-*
katello::params - these are shared parameters between multiple components but not exposed to installer users. They are intended for advanced users (directly using the Puppet modules or via custom-hiera.yaml).
application layer - all parameters are exposed to installer users
- katello::candlepin maps to --candlepin-*
- katello::pulp maps to --pulp-*
- katello::application maps to --application-*

Now for the installer we don't want to make this configurable while still allowing advanced users to change it. That means it's a parameter on katello::params. It would be confusing to have the parameters to that again at the application layer so on katello::candlepin there would not be a $host parameter but rather inside the body it'd have $body = $katello::params::candlepin_host. And the same for katello::application.

This way we avoid having installer parameters that the user is not supposed to touch while still providing all the power to advanced users who want to do custom things.

Right now I feel this design is still rather experimental and we're inventing a new pattern. This means we may find out it doesn't work or doesn't scale, but I'd like to try it because I feel it has the potential to strike the balance well.

jturel · 2020-02-26T23:24:11Z

I tested the trio of PRs and it works nicely. Thanks!

ehelms · 2020-02-27T01:37:05Z

OK... I think I get the gist of the idea now. Let's see what you think. The tests here won't pass until I merge the corresponding Candlepin and Certs changes.

ekohl

Other than the 1 line, this is indeed what I suggested.

ekohl · 2020-02-27T17:05:38Z

manifests/params.pp

  Stdlib::Httpsurl $pulp_url = "https://${facts['fqdn']}/pulp/api/v2/",
  Stdlib::Httpsurl $crane_url = "https://${facts['fqdn']}:5000",
  Stdlib::Host $qpid_hostname = 'localhost',
  String[1] $candlepin_oauth_key = $katello::globals::candlepin_oauth_key,
  String[1] $candlepin_oauth_secret = $katello::globals::candlepin_oauth_secret,
+  Stdlib::Host $candlepin_host = 'localhost',
+  Stdlib::Httpsurl $candlepin_url = "https://${candlepin_host}:8443/candlepin"


I don't know if this works. I've seen people use it, but never trusted it. Given this is the only way to deploy candlepin, I'd move the $candlepin_url part to the body of this class.

This worked in my testing running these changes live with the installer. If its defined in the body, will other modules be able to access it?

Yes, that still works.

ekohl

This also means increasing the minimum required version of the candlepin module. Perhaps the candlepin PR should bump the major version and this should update metadata.json.

ekohl · 2020-02-27T17:07:14Z

.fixtures.yml

@@ -24,6 +24,7 @@ fixtures:
      puppet_version: '>= 6.0.0'
    stdlib:        "https://github.com/puppetlabs/puppetlabs-stdlib.git"
    systemd:       "https://github.com/camptocamp/puppet-systemd.git"
+    transition:    "https://github.com/puppetlabs/puppetlabs-transition.git"


Nit: I'd do this as a separate commit. Fine to do it in the same PR.

ehelms · 2020-03-01T17:19:54Z

If I recap what you are saying, this change is a breaking change for puppet-candlepin? I'm not sure I follow why that is given we are adding a feature and changing a default. What is the breaking change?

…

On Sun, Mar 1, 2020, 10:34 AM Ewoud Kohl van Wijngaarden < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In manifests/params.pp <#321 (comment)> : > Stdlib::Httpsurl $pulp_url = "https://${facts['fqdn']}/pulp/api/v2/", Stdlib::Httpsurl $crane_url = "https://${facts['fqdn']}:5000", Stdlib::Host $qpid_hostname = 'localhost', String[1] $candlepin_oauth_key = $katello::globals::candlepin_oauth_key, String[1] $candlepin_oauth_secret = $katello::globals::candlepin_oauth_secret, + Stdlib::Host $candlepin_host = 'localhost', + Stdlib::Httpsurl $candlepin_url = "https://${candlepin_host}:8443/candlepin" Yes, that still works. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321?email_source=notifications&email_token=AACHT4YMKRYAITQVESDT7WDRFJ56VA5CNFSM4KP5MKR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXO7FPA#discussion_r386117451>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACHT46REJPQGG6XIJ5C22DRFJ56VANCNFSM4KP5MKRQ> .

ekohl · 2020-03-03T14:07:23Z

This also means increasing the minimum required version of the candlepin module. Perhaps the candlepin PR should bump the major version and this should update metadata.json.

The exact candlepin version doesn't matter that much, but the lower bound should set a version that's actually compatible:

puppet-katello/metadata.json

Line 25 in f6dda3e

"version_requirement": ">= 4.0.0 < 9.0.0"

Right now puppet-candlepin's master branch has version 8.0.0 which equals the already released version. In the past we just merged these things and worked it out around branching but I'd like to get in the habit of fixing it while we're making changes so I'm not stuck with a bunch of work around branching. theforeman/puppet-candlepin#146 & #325 work towards that.

ekohl

I was too soon. Please increase the minimum version in metadata.json to require at least candlepin 9.0.0.

theforeman-bot added Needs testing Not yet reviewed labels Feb 4, 2020

ehelms mentioned this pull request Feb 4, 2020

Fixes #28922: Deploy Candlepin on localhost by default theforeman/puppet-candlepin#142

Merged

ehelms force-pushed the refs-28922 branch 2 times, most recently from df95fd4 to a681cdd Compare February 24, 2020 18:42

ekohl requested changes Feb 26, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 26, 2020

ehelms force-pushed the refs-28922 branch from a681cdd to bc26a74 Compare February 26, 2020 18:40

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 26, 2020

ehelms force-pushed the refs-28922 branch from bc26a74 to 5e52329 Compare February 26, 2020 18:42

jturel approved these changes Feb 26, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 26, 2020

ehelms force-pushed the refs-28922 branch from 5e52329 to be99c97 Compare February 27, 2020 01:09

ekohl reviewed Feb 27, 2020

View reviewed changes

ehelms force-pushed the refs-28922 branch from be99c97 to d07f9db Compare February 27, 2020 21:40

ehelms force-pushed the refs-28922 branch from d07f9db to 084e662 Compare March 4, 2020 12:55

ekohl approved these changes Mar 4, 2020

View reviewed changes

ekohl requested changes Mar 4, 2020

View reviewed changes

theforeman-bot added the Waiting on contributor label Mar 4, 2020

ehelms force-pushed the refs-28922 branch from 084e662 to 6ada498 Compare March 4, 2020 13:08

theforeman-bot removed the Waiting on contributor label Mar 4, 2020

theforeman-bot added the Needs re-review label Mar 4, 2020

Refs #28922: Communicate via localhost to Candlepin

5326d9e

ehelms force-pushed the refs-28922 branch from 6ada498 to 5326d9e Compare March 4, 2020 13:27

ekohl approved these changes Mar 4, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Mar 4, 2020

ehelms merged commit dae20b5 into theforeman:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #28922: Communicate via localhost to Candlepin #321

Refs #28922: Communicate via localhost to Candlepin #321

ehelms commented Feb 4, 2020

ehelms commented Feb 24, 2020

ekohl left a comment

ekohl Feb 26, 2020

ehelms Feb 26, 2020

ekohl commented Feb 26, 2020

jturel commented Feb 26, 2020

ehelms commented Feb 27, 2020

ekohl left a comment

ekohl Feb 27, 2020

ehelms Feb 27, 2020

ekohl Mar 1, 2020

ekohl left a comment

ekohl Feb 27, 2020

ehelms Feb 27, 2020

ehelms commented Mar 1, 2020 via email

ekohl commented Mar 3, 2020

ekohl left a comment

Refs #28922: Communicate via localhost to Candlepin #321

Refs #28922: Communicate via localhost to Candlepin #321

Conversation

ehelms commented Feb 4, 2020

ehelms commented Feb 24, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 26, 2020

Choose a reason for hiding this comment

ehelms Feb 26, 2020

Choose a reason for hiding this comment

ekohl commented Feb 26, 2020

jturel commented Feb 26, 2020

ehelms commented Feb 27, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 27, 2020

Choose a reason for hiding this comment

ehelms Feb 27, 2020

Choose a reason for hiding this comment

ekohl Mar 1, 2020

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 27, 2020

Choose a reason for hiding this comment

ehelms Feb 27, 2020

Choose a reason for hiding this comment

ehelms commented Mar 1, 2020 via email

ekohl commented Mar 3, 2020

ekohl left a comment

Choose a reason for hiding this comment