Skip to content

Conversation

@alexjfisher
Copy link
Contributor

Support for puppet 3 was dropped in an earlier commit.

From puppetlabs-pe_gem README

As of Puppet 4.0, this module has been deprecated. Please use Puppet
4.0's built-in puppet_gem provider instead.

Since install.pp was a private class with only a single resouce I've
moved the declaration of the 'faraday' package resource to init.pp

Support for puppet 3 was dropped in an earlier commit.

From puppetlabs-pe_gem README
> As of Puppet 4.0, this module has been deprecated. Please use Puppet
> 4.0's built-in puppet_gem provider instead.

Since install.pp was a private class with only a single resouce I've
moved the declaration of the 'faraday' package resource to init.pp
gem 'rspec-puppet', :require => false
gem 'puppetlabs_spec_helper', :require => false
gem 'puppet-lint', :require => false
gem 'metadata-json-lint', :require => false
Copy link

@bastelfreak bastelfreak Nov 2, 2017

Choose a reason for hiding this comment

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

Why did you add the gem? If this is a needed dependency for one of the existing gems, then this should be addressed in a separate commit.

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’s a common mistake to remove the ‘dependencies’ key from the metadata if there are no dependencies. But this would make the metadata invalid, (if there are no dependencies, you still need an empty array). This gem guards against this from happening in the future.

@@ -1,14 +0,0 @@
# Private class
class f5::install {
if $::puppetversion and $::puppetversion =~ /Puppet Enterprise/ {
Copy link
Contributor

Choose a reason for hiding this comment

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

The better way to handle this would be to leave the f5::install class as many pre-existing users are using this and just comment out the pe_gem dependency.

cc @ericzji

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? The f5:install class isn't mentioned in the README and is marked Private class on line 1. I could move the faraday package resource back to this class, but there really did seem little point in keeping it. I also thought committing commented out code was considered bad practice these days?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the cisco_ios module uses this model:

https://github.com/puppetlabs/cisco_ios/blob/master/manifests/init.pp

And the device_manager module automatically leverages this model:

https://github.com/puppetlabs/device_manager/blob/master/manifests/init.pp#L76

@tkishel
Copy link
Contributor

tkishel commented Oct 21, 2019

At this point, implementing

class f5::install {
  package { 'faraday':
    ensure   => present,
    provider => 'puppet_gem',
  }
}

... in install.pp is reasonable.

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.

4 participants