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

Getting my feet wet with sensu-puppet #2

Merged
merged 12 commits into from
Jul 25, 2012
Merged

Conversation

shaftoe
Copy link

@shaftoe shaftoe commented Jul 5, 2012

Reading Sensu wiki I've seen there's already an APT repo for sensu, adding few manifests to deal with that

Alexander Fortin added 4 commits July 5, 2012 11:34
- Adding apt/* and debian manifests to deal with APT sources.
  So far hardcoding Sensuapp ones as from Sensu wiki

- Add init.pp to be compliant with Puppet module guidelines
@joemiller
Copy link

I believe @rodjek purposely left apt and yum recipes out of this module because he believes most folks already have their own mechanism for managing apt and yum repos.

That said, perhaps we can include apt and yum recipes but make them selectable via configuration param (or similar) and default to 'off'? Might be nice for folks who don't, for whatever reason, have their own module for managing repos yet. Also makes testing the module with the sensu-test integration test suite easier (/selfish) =)

@shaftoe - also, can you add support for the sensu-testing repo, again, perhaps as a config option

@nstielau
Copy link

nstielau commented Jul 5, 2012

@shaftoe I don't 'do' puppet, just wanted to say your contribution and improvements are much appreciated! Woot!

@portertech
Copy link
Contributor

This is fantastic, reduces adoption friction for puppet users. Not enforcing the use of the apt/yum manifests will do the trick, modular is good :)

@shaftoe
Copy link
Author

shaftoe commented Jul 5, 2012

Hi folks, first thing thanks for the warm welcome!

Yep, my (totally selfish) idea was to make my sensu evaluation/testing easier leveraging puppet/vagrant, but I agree with @joemiller it should be a choice and not be mandatory to have it installed via (those) apt repository, ah and yes the 'testing" like parameter is nice to have too. I'll fix that soon.

@nstielau I've not been into puppet that much but seems that finally I've found a way to contribute, it feels good!

@portertech that's the idea, yes. My first feeling is that @joemiller blog post is great, but a puppet cross-distro module could encourage even more Sensu adoption (and testers ;) )

Alexander Fortin added 2 commits July 5, 2012 20:15
Refactored sensu::debian to be more flexible, now declaring
a sensu::debian class you can remove the repository or
setup the unstable repo, i.e.

  class { 'sensu::debian': ensure => 'absent' }
  class { 'sensu::debian': repo => 'unstable }
@rodjek
Copy link
Contributor

rodjek commented Jul 6, 2012

@shaftoe rather than duplicate the logic of existing apt types, what I'd do instead is detect if the user has the puppetlabs apt module installed (or something compatible) and use that instead

if defined(Apt::Source) {
  apt::source { 'sensu':
    location => 'http://repos.sensuapp.org/apt',
    release  => 'sensu',
    repos    => 'main',
  }
}

@shaftoe
Copy link
Author

shaftoe commented Jul 6, 2012

@rodjek yep good idea. So you suggest no fallback if the APT module is not installed? I'd add at least an else notifying the module is missing

@joemiller
Copy link

is there something similar we could/should do for yum distros here?

@shaftoe
Copy link
Author

shaftoe commented Jul 7, 2012

@joemiller yes, on the Sensu wiki there's a reference to a yum repo as well. I don't know RH based distributions very well but I'm quite sure there'll be a puppet module to manage yum repos we can use in the same way as for apt

@joemiller
Copy link

there's a yumrepo resource in puppet.

some brainstorming here... pseudo-code-ish:

class sensu::repo {

   case $operatingsystem {
     'debian|ubuntu': { include sensu::repo::apt },
    'fedora|rhel|centos': { include sensu::repo::yum }
  }
} 

sensu::repo class could then be left as an option to the user to decide if they want to include it or use their own.

Might also be a good idea to have a sensu::params class to centralize variables as well. This seems to becoming standard practice in puppetland. And we could have a $sensu::repo var for specifying the main or unstable repos. Default being main of course.

I'm going to try to bring @jamtur01 into the conversation for some feedback on the approach too.

@jamtur01
Copy link
Contributor

jamtur01 commented Jul 7, 2012

@joemiller The pseudo-code is broadly fine.

Regarding the lack of a params.pp that is a style thing and I'd have to understand what @rodjek was/is planning. Currently this isn't actually a valid module - @rodjek is there any reason no init.pp?

Alexander Fortin added 4 commits July 9, 2012 15:08
- Removed sensu::apt classes
- Added sensu::repo class. So far it handles apt only. Manifest seems
  fine, but it breaks with http://repos.sensuapp.org/apt, seems that
  Release file for that APT repo is missing
@joemiller
Copy link

looks good to me. +1

next, we need a readme soon with mention of including sensu::repo class for those who want it =)

@shaftoe
Copy link
Author

shaftoe commented Jul 10, 2012

@joemiller nah, truth is I'm a bad git user, I still have to figure out how to properly rebase code and avoid pushing unfinished code to master... by the way, as it is right now, at least the repo::apt class is not working properly, but it looks like an apt misconfiguration other than a puppet bug. Still have to test repo::yum too

@joemiller
Copy link

more info on the apt repo issue would be appreciated. i saw the note about
missing Release file, but it's there, and the apt bits are working in the
sensu-chef code base.

what OS/distro/version? got a gist of detailed output from apt command(s)
that are failing?

On Tue, Jul 10, 2012 at 3:34 AM, Alexander Fortin <
reply@reply.github.com

wrote:

@joemiller nah, truth is I'm a bad git user, I still have to figure out
how to properly rebase code and avoid pushing unfinished code to master...
by the way, as it is right now, at least the repo::apt class is not working
properly, but it looks like an apt misconfiguration other than a puppet
bug. Still have to test repo::yum too


Reply to this email directly or view it on GitHub:
#2 (comment)

@shaftoe
Copy link
Author

shaftoe commented Jul 10, 2012

Here it breaks: https://gist.github.com/3083227
I'm testing it with Vagrant 0.8.10 and Puppet 2.7.17 on a Debian Squeeze 6.0 32bit box

Strange thing is that "aptitude update" doesn't complain, apt-get update does. Seems because of the deb-src line the apt module automatically adds

deb http://repos.sensuapp.org/apt sensu main
deb-src http://repos.sensuapp.org/apt sensu main

Removing second line seems to fix it

@joemiller
Copy link

Can you show the contents of the repo file puppet creates? I suspect it is
adding a 'deb-src' line but we do not have a source repo, only binaries
repo.

Looking at the apt module, maybe fixable by adding 'include_src => false'
to apt::source resource.

On Tuesday, July 10, 2012, Alexander Fortin wrote:

Here it breaks: https://gist.github.com/3083227
I'm testing it with Vagrant 0.8.10 and Puppet 2.7.17 on a Debian Squeeze
6.0 32bit box


Reply to this email directly or view it on GitHub:
#2 (comment)

@shaftoe
Copy link
Author

shaftoe commented Jul 10, 2012

@joemiller I did updated my previous reply, anyway yes it's as you thought, I'm going to fix with the include_src parameter

@shaftoe
Copy link
Author

shaftoe commented Jul 25, 2012

Hi folks, finally found some time to dig into this again. What's left to fix here? In the meanwhile I'm thinking about adding some testing, maybe with the new puppetlabs_spec_helper gem.

@portertech
Copy link
Contributor

@shaftoe Hey! If the Sensu puppeteers agree on the changes above, I'd like to merge this sucker in :) Further additions, testing etc, can be a separate PR.

@joemiller
Copy link

+1 from me

@portertech
Copy link
Contributor

@shaftoe I'd love to see these changes merged in, and then have a PR for Vagrant and testing ;)

@jamtur01
Copy link
Contributor

Seems reasonable to me. +1.

@portertech
Copy link
Contributor

Ok, thanks guys, merging.

portertech added a commit that referenced this pull request Jul 25, 2012
Getting my feet wet with sensu-puppet
@portertech portertech merged commit 7298fc5 into sensu:master Jul 25, 2012
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.

6 participants