Added parameter sensu::install_repo as the first condition to manage …

[mrodm]

Currently, if a user does not want to install the repositories by setting the parameter class install_repo to false, and this is declared in hiera, there is an error about duplicated resources apt::source (resource declared in manifests/repo/apt.pp file).

This PR changes the checks of the if statement to avoid this error about duplicated resources.

[jlambert121]

Where is the second declaration of apt::source{ 'sensu': }? Are you declaring it in your manifests as well? The module intentionally declares it so it can remove the repo if you don't want it and should be just settingensure => 'absent'` on the repo itself.

[mrodm]

I've defined this resource using hiera and create_resources.
The problem is that once the resource is defined by that file as ensure => absent (by setting $sensu::install_repo = false), I cannot define it by hiera.

The check of this PR is as it is performed for yum repos.

[jlambert121]

The packages create a repo with the name of sensu and setting install_repo ensures they get removed. If you'd like to define your own sensu repository, you'll need to name it something other than sensu.

[mrodm]

That's true, I could change just the name of the resource in hiera to solve the duplicated resource, but I did not explained myself well above.
This error appears because I need to manage apt packages/repositories by means of stages. My stages are defined as:

stage{ 'provisioning': }
Stage['provisioning'] -> Stage['main']
class { 'apt': stage => 'provisioning' }

In this way, the definition of this resource in the sensu module produces a circular dependency when used with stages, even when it is declared as absent by means of setting $sensu::install_repo = false.

This PR tries to solved this circular dependency by avoiding the declaration of the apt resource when the parameter install_repo is set to false.

[eholzbach]

I just hit the same issue today.

[jlambert121]

I'm not a fan of removing the management of the repo if install_repo is false because then the repo installed with the package never gets removed.

For your stage setup, what about just adding

Apt { stage => 'provisioning' }

[mrodm]

I'm thinking in another option to keep both approaches.
It could be added another boolean parameter to the module named "manage_repo" with default value true.

class sensu (
...
$manage_repo = true,
...

This new parameter 'manage_repo' would handle the apt resource definition and install_repo will keep enabling or removing the new repo as previously. Adding this parameter will allow us to break the circular dependency when stages are used if it is redefined by means of hiera.

if $sensu::manage_repo {
  if defined(apt::source) {

    $ensure = $sensu::install_repo ? {
      true    => 'present',
      default => 'absent'
    }

    if $sensu::repo_source {
      $url = $sensu::repo_source
    } else {
      $url = 'http://repos.sensuapp.org/apt'
    }

    apt::source { 'sensu':
      ensure   => $ensure,
      location => $url,
      release  => 'sensu',
      repos    => $sensu::repo,
      include  => {
        'src' => false,
      },
      key      => {
        'id'     => $sensu::repo_key_id,
        'source' => $sensu::repo_key_source,
      },
      before   => Package['sensu'],
    }

[jlambert121]

I could go for that. I don't like adding additional (potentially confusing) parameters, but I understand the problem and can't think of anything else. Could you wrap the yum repo as well with it?

[mrodm]

Thanks!
I'll do the change for yum repo too and I'll update this PR.

[jlambert121]

Perfect! If you get this in the next day or two I'll get it in the next release I need to push up.

[mrodm]

I've updated the PR adding the new variable $manage_repo in two conditions.

About yum, Introducing these changes into yum repos is not an easy task.
Depending on the puppet version, the yumrepo has different behavior. For instance, in puppet 3.3 yumrepo has no ensure parameter and it causes that the tests fail.

[jlambert121]

What about instead of putting it in the repo::type class we just put it in package.pp and avoid the class altogether?

[mrodm]

I'll give it a try

[mrodm]

Updated the PR, moving the condition using sensu::manage_repo to package.pp

[jlambert121]

Awesome! looks a whole lot cleaner. One last request and then I'll merge it and do a release today - can you throw a couple tests in for manage_repo = false and make sure the repo classes aren't included?

[mrodm]

I've added some tests to check the behavior using manage_repo variable

[jlambert121]

Thanks! Release coming shortly as well.