Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(MODULES-8523) Remove legacy Puppet 3 code #374

Merged
merged 6 commits into from Jan 28, 2019

Conversation

ekinanp
Copy link
Contributor

@ekinanp ekinanp commented Jan 24, 2019

The puppet_agent module no longer supports upgrades from Puppet 3 nodes.
This commit removes the remaining Puppet 3 code from the manifest and
any remaining spec tests.

@ekinanp ekinanp force-pushed the MODULES-8523 branch 2 times, most recently from 06c6d61 to 56e40f5 Compare January 25, 2019 16:41
@puppetcla
Copy link

CLA signed by all contributors.

@ekinanp ekinanp force-pushed the MODULES-8523 branch 3 times, most recently from 01cb391 to f9e0166 Compare January 25, 2019 17:16
fail("invalid version ${package_version} requested")
}

# Strip git sha from dev builds
if ($package_version != undef and $package_version =~ /g/){
if $package_version =~ /g/ {
$_expected_package_version = split($package_version, /[.-]g.*/)[0]
} else {
$_expected_package_version = $package_version
}

$aio_upgrade_required = (getvar('::aio_agent_version') == undef) or
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if I can remove the (getvar('::aio_agent_version') == undef) check? That fact should be defined on Puppet 4 agents, shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should blow up a few lines further down if it's missing.

Copy link
Contributor Author

@ekinanp ekinanp Jan 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out I can remove it -- checking if it is undef is a quick way to see if you're running on a pre-Puppet 4 node. Updated the PR with the removal.

@ekinanp
Copy link
Contributor Author

ekinanp commented Jan 25, 2019

I've tested some PE upgrades on:

osx_1013_x86_64
debian_9_i386
solaris_10_i386
solaris_11_i386
windows 

and things seem to work fine. This is ready for merge.

EDIT: Not yet, there's a few classes that can be removed here too.

@ekinanp ekinanp force-pushed the MODULES-8523 branch 3 times, most recently from a3704d9 to 1d697bb Compare January 25, 2019 22:21
@@ -73,28 +67,27 @@
validate_absolute_path($install_dir)
}

Copy link
Contributor Author

@ekinanp ekinanp Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it'd be worth adding a check here to error if $::aio_agent_version is undefined (i.e. error if we're running on a pre-Puppet 4 node, assuming my Q. about $::aio_agent_version is true).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Q. about $::aio_agent_version is true, so my other Q. about whether it's worth erroring if that variable is undefined still stands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes. Major things depend on it being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll push up another commit to add this capability then.

@ekinanp
Copy link
Contributor Author

ekinanp commented Jan 25, 2019

Ok, now this is ready for merge.

@ekinanp
Copy link
Contributor Author

ekinanp commented Jan 25, 2019

It'd be good if somebody can double-check some of the logical substitutions made in this PR, as those can be subtle.

The puppet_agent module no longer supports upgrades from Puppet 3 nodes.
This commit removes the remaining Puppet 3 code from the main manifests
and any remaining spec tests.
@@ -105,7 +97,6 @@
# use the full version string for comparisons.
if $::operatingsystem == 'Solaris' and $::operatingsystemmajrelease == '11' {
# Strip letters from development builds. Unique to Solaris 11 packaging.
# Need to pass the regex as strings for Puppet 3 compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does it get us or lose us anything to keep passing as strings? Or is it a wash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to pass them in as patterns, but I don't want to risk accidentally breaking something due to some mal-formed regex (e.g. like escaping the wrong thing), so I chose to keep them in as strings.

:lsbdistcodename => 'baz',
:mco_server_config => nil,
:mco_client_config => nil,
:lsbdistcodename => 'baz'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: does this need a trailing comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

README.markdown Outdated
* `puppet_agent::osfamily::*`: Platform-specific preparation performed before upgrades.
* `puppet_agent::prepare`: Prepares the agent for upgrade.
* `puppet_agent::prepare::package`: Stages packages locally for install, on platforms that can't install from remote packages.
* `puppet_agent::prepare::*`: Prepare various file and ssl configuration.
* `puppet_agent::prepare::*`: Prepare various file configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configurations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ekinanp
Copy link
Contributor Author

ekinanp commented Jan 28, 2019

Pushed up another commit to error if the aio_agent_version fact is undefined.

An undefined aio_agent_version fact means we're running on a pre-Puppet
4 node. Since pre-Puppet 4 upgrades are no longer supported, we should
fail the run if aio_agent_version is undefined.
@mcdonaldseanp mcdonaldseanp merged commit 536a6a5 into puppetlabs:master Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants