Make bootstrap.py compatible with Puppet 4 #230

h4xr · 2018-01-03T12:30:08Z

Fixes #229

Signed-off-by: Saurabh Badhwar sbadhwar@redhat.com

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

evgeni · 2018-01-03T12:37:33Z

bootstrap.py

@@ -383,7 +383,10 @@ def install_puppet_agent():
    call_yum("install", "puppet")
    enable_service("puppet")

-    puppet_conf = open('/etc/puppet/puppet.conf', 'wb')
+    if os.path.isfile('/etc/puppet/puppet.conf'):


can we turn that around and handle /etc/puppetlabs as the special case? feels slightly "cleaner" to me

evgeni · 2018-01-03T12:38:15Z

bootstrap.py

@@ -407,7 +410,10 @@ def install_puppet_agent():
    print_generic("Running Puppet in noop mode to generate SSL certs")
    print_generic("Visit the UI and approve this certificate via Infrastructure->Capsules")
    print_generic("if auto-signing is disabled")
-    exec_failexit("/usr/bin/puppet agent --test --noop --tags no_such_tag --waitforcert 10")
+    if os.path.isfile('/usr/bin/puppet'):


can we turn that around and test for /opt/puppetlabs/puppet/bin/puppet? and in the else case, run puppet without a path at all, so the shell can find it even in another folder?

The changes add functionality for detecting the installed version of puppet and configures the puppet accordingly. Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

evgeni · 2018-01-05T08:13:56Z

bootstrap.py

+    return executable_path
+
+
+def get_puppet_version():


I think I'd still prefer using the RPMdb like this:

def get_puppet_version(): ts = rpm.TransactionSet() for name in ['puppet', 'puppet-agent']: mi = ts.dbMatch('name', name) for h in mi: puppet_version = int(h['version'].split('.')[0]) if h['name'] == 'puppet-agent' and puppet_version == 1: puppet_version = 4 return puppet_version

This works fine for Puppet 3, 4 and 5

@sideangleside whatcha think?

@evgeni If I am not getting wrong, we are comparing the puppet-agent packaging version here. I doubt if this will be future proof or not, in case packaging convention changes in future for the newer releases of Puppet.

correct, it's using the packaging version. I would not expect the major version of the packaging to differ from the major version of the agent in the future (well, puppet4 agent is an exemption here :()

I am strongly supportive of checking the RPMs version number as it is consistent with other checks for packages that we do. Additionally, checking via an executable's location is just as prone to packaging changes as anything else, so i'd want to be consistent.

evgeni · 2018-01-05T08:16:42Z

bootstrap.py

+    puppet_major_version = get_puppet_version()
+    if puppet_major_version == 3:
+        puppet_conf_file = '/etc/puppet/puppet.conf'
+    elif puppet_major_version == 4:


can we make this puppet_major_version in [4,5]? as Puppet5 uses the same path and that would make it more future proof

evgeni · 2018-01-05T08:19:26Z

bootstrap.py

@@ -218,6 +218,45 @@ def install_prereqs():
        delete_file('/etc/yum.repos.d/katello-client-bootstrap-deps.repo')


+def get_puppet_path():


this would be then (if we use the RPMdb)

puppet_major_version = get_puppet_version() if puppet_major_version == 3: return '/usr/bin/puppet' elif puppet_major_version in [4,5]: return '/opt/puppetlabs/puppet/bin/puppet' else: error

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

h4xr · 2018-01-09T08:04:48Z

@evgeni Addressed the changes in the commit. Please review :)

evgeni · 2018-01-09T09:38:26Z

code LGTM 👍

I'll give it a few test runs later today and merge afterwards, unless @sideangleside is faster ;)

evgeni · 2018-01-09T15:27:07Z

So I gave it a fair spin, and it seems to work fine. However I spotted a few differences in the puppet.conf compared to the version Foreman deploys by default:

our:

vardir = /var/lib/puppet
logdir = /var/log/puppet
rundir = /var/run/puppet
ssldir = $vardir/ssl

foreman:

vardir = /opt/puppetlabs/puppet/cache
logdir = /var/log/puppetlabs/puppet
rundir = /var/run/puppetlabs
ssldir = /etc/puppetlabs/puppet/ssl

I think we should align this for the Puppet4/5 case.

evgeni · 2018-01-09T15:28:11Z

and with that change, we should also extend https://github.com/h4xr/katello-client-bootstrap/blob/2a9f0662d26b90042141a79cda9d216eb3118079/bootstrap.py#L384 to wipe /opt/puppetlabs/puppet/cache and /etc/puppetlabs/puppet/ssl

evgeni · 2018-01-10T12:47:26Z

bootstrap.py

+rundir = /var/run/puppet
+ssldir = /etc/puppetlabs/puppet/ssl
+""")
+    else:


that code is unreached, we already verified that it's version 3, 4 or 5 when looking for the conf path

evgeni · 2018-01-10T12:49:33Z

bootstrap.py

+        print_error("Unsupported puppet version")
+        sys.exit(1)
+    puppet_conf = open(puppet_conf_file, 'wb')
+    if puppet_major_version == 3:


I think instead of having another set of if/elif here, I'd put the dirs (or the whole [main] section) into a variable during the conf path detection, and just use the variable in one write().

evgeni

two comments, which would make the code a bit cleaner (IMHO), but neither is really blocking and the code works just fine as it is. ACK from me.

would love to hear a second ACK from @sideangleside before merging, though

evgeni

one functional, and two cosmetic comments, good to go when at least the functional is addressed.

evgeni · 2018-01-12T09:05:46Z

bootstrap.py

+[main]
+vardir = /opt/puppetlabs/puppet/cache
+logdir = /var/log/puppetlabs/puppet
+rundir = /var/run/puppet


this should be /var/run/puppetlabs

evgeni · 2018-01-12T09:35:26Z

bootstrap.py

+    puppet_major_version = get_puppet_version()
+    if puppet_major_version == 3:
+        puppet_conf_file = '/etc/puppet/puppet.conf'
+        main_section = """


cosmetic: this makes main_section start with a newline, and thus the generated puppet.conf starts with two newlines. can we change this to main_section = """[main]…?

evgeni · 2018-01-12T09:35:33Z

bootstrap.py

+"""
+    elif puppet_major_version in [4, 5]:
+        puppet_conf_file = '/etc/puppetlabs/puppet/puppet.conf'
+        main_section = """


cosmetic: this makes main_section start with a newline, and thus the generated puppet.conf starts with two newlines. can we change this to main_section = """[main]…?

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

evgeni

ACK

waiting on @sideangleside

sideangleside · 2018-01-15T15:12:05Z

LGTM 👍

sideangleside · 2018-01-15T15:12:24Z

Thanks for the PR @h4xr

* make bootstrap.py compatible with Puppet 4 (Katello#230) * add ansible example playbook (Katello#197) * remove SAMs CA consumer RPM in remove_obsolete_packages() (Katello#234) * allow FIPS enabled systems to register successfully with puppet (Katello#236) * allow configuring timeout for API calls and subscription-manager (Katello#237) * implement switching proxy / caps without destroying the host in Foreman (Katello#227)

Make bootstrap.py compatible with Puppet 4

5e78683

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

evgeni requested changes Jan 3, 2018

View reviewed changes

h4xr force-pushed the puppet4-fix branch from 76bb294 to d9b10f3 Compare January 3, 2018 14:27

Allow puppet to be configured according to the detected version

a0dd7e1

The changes add functionality for detecting the installed version of puppet and configures the puppet accordingly. Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

h4xr force-pushed the puppet4-fix branch from d9b10f3 to a0dd7e1 Compare January 3, 2018 15:31

evgeni reviewed Jan 5, 2018

View reviewed changes

Use RPMdb for determining Puppet version

2a9f066

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

h4xr force-pushed the puppet4-fix branch from f5a706e to 2a9f066 Compare January 9, 2018 08:02

h4xr force-pushed the puppet4-fix branch from ba3d472 to 4af76eb Compare January 10, 2018 10:13

evgeni reviewed Jan 10, 2018

View reviewed changes

evgeni approved these changes Jan 10, 2018

View reviewed changes

h4xr force-pushed the puppet4-fix branch from 4af76eb to 2e2bc59 Compare January 12, 2018 08:06

evgeni requested changes Jan 12, 2018

View reviewed changes

Adapt puppet configuration for different versions of puppet

8e70aec

Signed-off-by: Saurabh Badhwar <sbadhwar@redhat.com>

h4xr force-pushed the puppet4-fix branch from 2e2bc59 to 8e70aec Compare January 15, 2018 08:12

evgeni approved these changes Jan 15, 2018

View reviewed changes

sideangleside merged commit 2787352 into Katello:master Jan 15, 2018

evgeni mentioned this pull request Jan 17, 2018

Capsule migration option, take 2 #227

Merged

sideangleside mentioned this pull request Jan 17, 2018

[RFE] Should be able to bootstrap with puppet 3 and puppet 4 #105

Closed

evgeni mentioned this pull request Jan 23, 2018

release 1.5.0 #239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bootstrap.py compatible with Puppet 4 #230

Make bootstrap.py compatible with Puppet 4 #230

h4xr commented Jan 3, 2018 •

edited

evgeni Jan 3, 2018

evgeni Jan 3, 2018

evgeni Jan 5, 2018 •

edited

h4xr Jan 5, 2018

evgeni Jan 5, 2018

sideangleside Jan 8, 2018

evgeni Jan 5, 2018

sideangleside Jan 8, 2018

evgeni Jan 5, 2018

h4xr commented Jan 9, 2018

evgeni commented Jan 9, 2018

evgeni commented Jan 9, 2018 •

edited

evgeni commented Jan 9, 2018

evgeni Jan 10, 2018

evgeni Jan 10, 2018

evgeni left a comment

evgeni left a comment

evgeni Jan 12, 2018

evgeni Jan 12, 2018

evgeni Jan 12, 2018

evgeni left a comment

sideangleside commented Jan 15, 2018

sideangleside commented Jan 15, 2018

		@@ -218,6 +218,45 @@ def install_prereqs():
		delete_file('/etc/yum.repos.d/katello-client-bootstrap-deps.repo')


		def get_puppet_path():

Make bootstrap.py compatible with Puppet 4 #230

Make bootstrap.py compatible with Puppet 4 #230

Conversation

h4xr commented Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h4xr commented Jan 9, 2018

evgeni commented Jan 9, 2018

evgeni commented Jan 9, 2018 • edited

evgeni commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni left a comment

Choose a reason for hiding this comment

evgeni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni left a comment

Choose a reason for hiding this comment

sideangleside commented Jan 15, 2018

sideangleside commented Jan 15, 2018

h4xr commented Jan 3, 2018 •

edited

evgeni Jan 5, 2018 •

edited

evgeni commented Jan 9, 2018 •

edited