voxpupuli / puppet-elasticsearch

Elasticsearch Puppet module
Apache License 2.0
403 stars 478 forks source link

dependency on CERIT-SC/puppet-yum #558

Closed alex-harvey-z3q closed 7 years ago

alex-harvey-z3q commented 8 years ago

A decision was made to implement package pinning.

Personally, I do not recommend using Puppet to manage package versions at all, and package pinning is therefore a feature many people will not use.

Unfortunately, it has introduced a dependency on the CERIT-SC/puppet-yum module.

The CERIT-SC/puppet-yum module has a score of only 3.8 on Puppet Forge, so many people will already be using either their own yum module, or a different yum module from the Forge.

But if we are to use the elastic/puppet-elasticsearch module, we are now forced to use the CERIT-SC/puppet-yum module.

I propose that we remove the dependency on the CERIT-SC/puppet-yum module, and move any functionality borrowed from there inside the elasticsearch module.

alex-harvey-z3q commented 8 years ago

More on this issue http://stackoverflow.com/questions/30254335/how-do-i-deal-with-puppet-modules-with-classes-of-the-same-name

electrical commented 8 years ago

Hi,

Since the package pinning is enabled by default, not having that dependency will break default behaviour and causes bad initial experience. If there is a better module that supports package pinning I'm happy to use it.

alex-harvey-z3q commented 8 years ago

It's not a question of whether there's a better one; the issue there's no "standard" Puppet Labs yum module available.

For me to use this module, therefore, I would need to refactor large amounts of internal code to remove the yum module we wrote for ourselves. But that yum module is a good module, doing everything we need it to do. So I'd have no choice, realistically, but to fork this module and hack it.

Admittedly, it's the design of Puppet itself that is at fault and we shouldn't end up in this situation.

Even so, Puppet being what it is, and this project being a Puppet module, I don't think it's a reasonable design choice to force people to install somewhat obscure Forge modules, if there's a good chance they'll conflict with other modules people have already chosen, unless they are "standard" supported modules like puppetlabs/stdlib, puppetlabs/apache etc.

I don't want to break the default behaviour though.

Why can't we, as I said, simply copy that small bit of functionality from CERIT-SC/puppet-yum that we needed for package pinning inside this module?

electrical commented 8 years ago

We could copy it, but I'm planning to use the same thing in every module eventually. And what i don't like is having duplicate code everywhere.

alex-harvey-z3q commented 8 years ago

I think that forcing anyone who wants to use this module to also use this project's preferred yum module is introducing a massive limitation on the software that far eclipses even loads of duplicated code. Loads of sites will have written their own yum modules as we have.

How about this then: create a module elastic/elastic-yum with new class names that won't conflict with standard yum classes?

electrical commented 8 years ago

Hmm, that might be possible yeah. Let me think about it.

alex-harvey-z3q commented 8 years ago

I am more than happy to do the refactoring if you make a decision.

alex-harvey-z3q commented 8 years ago

@electrical Perhaps there's an easier solution for the same problem in lesaux/puppet-kibana4. That is,

How does this sound? Again, happy to implement if you're happy with the proposed solution.

jsnod commented 8 years ago

Just ran into this same problem. We've rolled our own yum module, so ceritsc-yum won't install because of the naming conflict. Which means elasticsearch-elasticsearch won't install because it is missing ceritsc-yum.

alex-harvey-z3q commented 8 years ago

@electrical ping

alex-harvey-z3q commented 8 years ago

@tylerjl Could I get an update here? I looked into using CERIT-SC/yum as a replacement of evenup/yum module but the CERIT-SC/yum doesn't manage Yumrepo's, only provides helper functions. So this module, basically, makes it impossible to have a class called yum that manages yumrepos.

tylerjl commented 8 years ago

I've been mulling it over and don't have a clear answer yet either. I do agree, though, that polluting the yum module namespace probably shouldn't happen for the reasons you've described.

Forking into elastic-yum isn't really ideal since it just compounds the complexity of keeping up with upstream, and removing the hard dependency would make it really obnoxious for new users when the module fails because all of the dependencies aren't explicitly declared.

Honestly, I think I'd probably fall on the side of forking it and changing the module namespace to place the burden of managing the dissonance on our side rather than making users mess around with undeclared dependencies.

cdenneen commented 8 years ago

I think best solution is to approach the 2 upstream modules and work on merging them. Namespace conflicts are inevitable until we can pair down many many modules into the "approved" ones.

I don't see why either module maintainer wouldn't take a PR to combine the functionality.

alex-harvey-z3q commented 8 years ago

Ok, for anyone else in a similar situation, here's what I've done, although requires Puppet 4.

class profile::base::yum (
  Hash $repos,
) {
  Yumrepo {
    stage => 'pre',
  }
  file { '/etc/yum.repos.d/':
    ensure  => directory,
    recurse => true,
    purge   => true,
  }
  create_resources(yumrepo, $repos)
  keys($repos).each |String $yumrepo| {
    file { "/etc/yum.repos.d/${yumrepo}.repo": }
    ->
    Yumrepo[$yumrepo]
  }
}
profile::base::yum::repos:
  'C7.0.1406-base':
    ensure: 'present'
    baseurl: 'http://vault.centos.org/7.0.1406/os/$basearch/'
    descr: 'CentOS-7.0.1406 - Base'
    enabled: '0'
    gpgcheck: '1'
    gpgkey: 'file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7'
  'C7.0.1406-centosplus':
    ensure: 'present'
    baseurl: 'http://vault.centos.org/7.0.1406/centosplus/$basearch/'
    descr: 'CentOS-7.0.1406 - CentOSPlus'
    enabled: '0'
    gpgcheck: '1'
    gpgkey: 'file:///etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-7'
...

The code ensures that purging is taken care of, and then CERIT-SC/yum can happily take over the yum namespace.

I feel a little uncomfortable about plagiarising the content of a very good module evenup/yum and merging it into the CERIT-SC/yum, just to work around this incredibly annoying issue, which is no one's fault.

cdenneen commented 8 years ago

@tylerjl Question... if I'm following this correctly the example42/yum actually has the versionlock support which is the only reason you were using cerit-sc/yum correct? So theoretically couldn't the dependency be switched and we no longer have namespace conflicts?

alex-harvey-z3q commented 8 years ago

@cdenneen That might be a solution although it's still a backwards-compatibility breaking change, and still doesn't avoid the issue that we're forcing people to use a specific yum module, and most people will have a yum module that they're already using.

I still think the best solution is to remove the dependency altogether and make it clear in the docs that to use the version lock feature you need CERIT-SC/yum or something that provides an equivalent yum::versionlock.

Then in the code:

if !defined('yum::versionlock') {
  fail("you need to use a yum module that provides a yum::versionlock defined type such as CERIT-SC/yum")
}

Remember that most people use R10K to manage their modules and IIRC R10K doesn't resolve dependencies in many situations. So I don't think people will be too upset or surprised to discover they need to manually install some dependencies.