Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default #329

ehelms · 2021-02-19T20:36:05Z

No description provided.

ehelms · 2021-02-19T20:39:26Z

See theforeman/puppet-katello#387 for more details

ekohl · 2021-03-02T19:51:02Z

manifests/init.pp

@@ -146,7 +147,7 @@

  include foreman_proxy_content::pub_dir

-  if $qpid_router {
+  if $enable_katello_agent {


I'm wondering if we should implement an else for this where we ensure it's stopped.

Will we need to add some additional parameters to puppet-qpid to be able to ensure things are stopped? Given https://github.com/theforeman/puppet-qpid/blob/master/manifests/router/service.pp#L6

ehelms · 2021-03-08T19:09:46Z

Relies on theforeman/puppet-qpid#148

ekohl · 2021-03-30T13:32:46Z

manifests/init.pp

+  class { 'foreman_proxy_content::dispatch_router':
+    agent_addr     => $qpid_router_agent_addr,
+    agent_port     => $qpid_router_agent_port,
+    ssl_ciphers    => $qpid_router_ssl_ciphers,
+    ssl_protocols  => $qpid_router_ssl_protocols,
+    logging_level  => $qpid_router_logging_level,
+    logging        => $qpid_router_logging,
+    logging_path   => $qpid_router_logging_path,
+    service_ensure => $enable_katello_agent,
+    service_enable => $enable_katello_agent,
+  }
+  contain foreman_proxy_content::dispatch_router


The only concern I have here is that this now always installs the router packages, even if the service is then disabled. Previously it didn't even install the packages.

Do we actually gain anything by having

class { 'foreman_proxy_content::dispatch_router': ... } contain foreman_proxy_content::dispatch_router

Declared outside of the

if $enable_katello_agent { ... }

block?

It's not clear to me why you are moving it here.

It exists outside of the enable_katello_agent block because we don't know the previous state, and therefore we want to ensure the service isn't running if it previously was running. If we were to always put it within the enable_katello_agent block you could enable it and then disable it while leaving the services running.

The question I posed in a different forum, which @ekohl asks here (#329 (comment)) is whether it is sufficient to just disable the services when disabling katello-agent support (or when freshly setting things up to have no presence of) or if we need to propagate this effect to all entities: services, packages, config files. The same question applies here: https://github.com/theforeman/puppet-katello/pull/387/files#diff-4d995f8e71429e47718f73b6c9d8cad415c63a67017803143f5fa1a929d0f730R19

wbclark · 2021-03-30T18:19:10Z

This needs a puppet-qpid release ( theforeman/puppet-qpid#151 ) to properly test it

Once that happens it also needs the dependency bumped to get the version which added that feature

ekohl · 2021-03-31T06:40:25Z

This needs a puppet-qpid release ( theforeman/puppet-qpid#151 ) to properly test it

In the past I've changed .fixtures.yml to point to my branch just so CI would run. Obviously you need to update it prior to merging with a dependency on actual change but it's a good trick to remember.

ehelms · 2021-03-31T19:56:04Z

Updated the metadata for the qpid 7.1.0 release and fixed up some failing tests

manifests/dispatch_router.pp

manifests/init.pp

wbclark

The failure on bundle exec rake validate is happening because of a typo'd filename for the dispatch router acceptance test

I confirmed that was resolved with git mv spec/acceptance/dispatch_router_spec.{pp,rb}

ehelms · 2021-04-08T01:50:35Z

The failure on bundle exec rake validate is happening because of a typo'd filename for the dispatch router acceptance test

I confirmed that was resolved with git mv spec/acceptance/dispatch_router_spec.{pp,rb}

Thanks. Things were not failing locally as it didnt seem to care about the extension and I could not see the forest for the trees.

ekohl

Both this and theforeman/puppet-katello#387 use Fixes #32037. To me it's a bit confusing to have 2 PRs both close a Redmine issue. Since we'll likely need an installer migration to really finish it, should all of them be Refs and only the migration would use Fixes?

ekohl · 2021-04-08T09:50:29Z

manifests/dispatch_router.pp

 ) {

+  class { 'qpid::router':
+    ensure => $ensure,
+  }
  contain qpid::router

  include certs::qpid_router::server


Perhaps this should be inside if $ensure == 'present' {}. However, that also implies the ssl_profile should be hidden. That also makes me wonder if all router statements (listener and log) should be hidden.

manifests/init.pp

ekohl · 2021-04-08T09:53:03Z

metadata.json

@@ -26,7 +26,7 @@
    },
    {
      "name": "katello/qpid",
-      "version_requirement": ">= 4.5.0 < 8.0.0"
+      "version_requirement": ">= 7.1.0 < 8.0.0"


Same note here as Katello: 7.1.0 doesn't contain the required patches.

ehelms · 2021-04-08T16:12:38Z

Both this and theforeman/puppet-katello#387 use Fixes #32037. To me it's a bit confusing to have 2 PRs both close a Redmine issue. Since we'll likely need an installer migration to really finish it, should all of them be Refs and only the migration would use Fixes?

Will do.

ekohl · 2021-04-20T19:07:04Z

manifests/init.pp

+    logging_level => $qpid_router_logging_level,
+    logging       => $qpid_router_logging,
+    logging_path  => $qpid_router_logging_path,
+    ensure        => bool2str($enable_katello_agent, 'present', 'absent'),


Odd: I thought there was a linting rule that preferred ensure to be the first line if given.

ekohl · 2021-04-20T19:07:37Z

manifests/dispatch_router.pp

@@ -30,43 +34,50 @@
  String $logging_level = 'info+',
  Enum['file', 'syslog'] $logging = 'syslog',
  Stdlib::Absolutepath $logging_path = '/var/log/qdrouterd',
+
+  Enum['present', 'absent'] $ensure = 'present',


It is common to have $ensure as the first

…ble it by default

theforeman-bot added Needs testing Not yet reviewed labels Feb 19, 2021

ehelms mentioned this pull request Feb 19, 2021

Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default theforeman/puppet-katello#387

Merged

ekohl reviewed Mar 2, 2021

View reviewed changes

ehelms force-pushed the katello-agent-off branch from 7a373ce to c51de20 Compare March 8, 2021 19:09

ehelms marked this pull request as ready for review March 8, 2021 19:09

ehelms changed the title ~~Add flag to enable katello_agent infrastructure and disable it by def…~~ Fixes #32037: Add flag to enable katello_agent infrastructure and disable it by default Mar 8, 2021

ekohl reviewed Mar 30, 2021

View reviewed changes

ehelms force-pushed the katello-agent-off branch 2 times, most recently from d4d75a5 to 91c3954 Compare March 31, 2021 19:29

ekohl reviewed Apr 1, 2021

View reviewed changes

manifests/dispatch_router.pp Outdated Show resolved Hide resolved

manifests/init.pp Show resolved Hide resolved

ekohl added the Backwards incompatible label Apr 1, 2021

ehelms force-pushed the katello-agent-off branch 2 times, most recently from 30281b0 to aa84128 Compare April 7, 2021 19:20

wbclark reviewed Apr 7, 2021

View reviewed changes

ehelms force-pushed the katello-agent-off branch from aa84128 to 1f749e2 Compare April 8, 2021 01:50

ekohl reviewed Apr 8, 2021

View reviewed changes

ehelms force-pushed the katello-agent-off branch 2 times, most recently from 83527a2 to cd7857b Compare April 8, 2021 19:41

ehelms mentioned this pull request Apr 15, 2021

Fixes #32037 - Disable katello-agent on install, ensure enabled on up… theforeman/foreman-installer#675

Merged

ehelms force-pushed the katello-agent-off branch from cd7857b to de7a636 Compare April 15, 2021 18:07

ehelms changed the title ~~Fixes #32037: Add flag to enable katello_agent infrastructure and disable it by default~~ Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default Apr 15, 2021

ehelms force-pushed the katello-agent-off branch from de7a636 to 1683550 Compare April 15, 2021 19:10

ekohl approved these changes Apr 20, 2021

View reviewed changes

theforeman-bot removed the Not yet reviewed label Apr 20, 2021

ehelms force-pushed the katello-agent-off branch from 1683550 to bd35bd7 Compare April 21, 2021 14:56

Refs #32037: Add flag to enable katello_agent infrastructure and disa…

7999930

…ble it by default

ehelms force-pushed the katello-agent-off branch from bd35bd7 to 7999930 Compare April 21, 2021 14:56

ekohl approved these changes Apr 21, 2021

View reviewed changes

ehelms merged commit f26e941 into theforeman:master Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default #329

Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default #329

ehelms commented Feb 19, 2021 •

edited

Loading

ehelms commented Feb 19, 2021

ekohl Mar 2, 2021

ehelms Mar 2, 2021

ehelms commented Mar 8, 2021

ekohl Mar 30, 2021

wbclark Apr 1, 2021

ehelms Apr 1, 2021

wbclark commented Mar 30, 2021

ekohl commented Mar 31, 2021

ehelms commented Mar 31, 2021

wbclark left a comment

ehelms commented Apr 8, 2021

ekohl left a comment

ekohl Apr 8, 2021

ekohl Apr 8, 2021

ehelms commented Apr 8, 2021

ekohl Apr 20, 2021

ekohl Apr 20, 2021

Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default #329

Refs #32037: Add flag to enable katello_agent infrastructure and disable it by default #329

Conversation

ehelms commented Feb 19, 2021 • edited Loading

ehelms commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbclark commented Mar 30, 2021

ekohl commented Mar 31, 2021

ehelms commented Mar 31, 2021

wbclark left a comment

Choose a reason for hiding this comment

ehelms commented Apr 8, 2021

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Feb 19, 2021 •

edited

Loading