Yelp / puppet-uchiwa

Puppet module for installing Uchiwa
Apache License 2.0
20 stars 59 forks source link

Remove uchiwa::api and pass API data via class params #21

Closed queeno closed 9 years ago

queeno commented 9 years ago

The puppet-uchiwa module doesn't produce a clean puppet run unless a user defines at least one uchiwa::api.

It would be ideal if the module could define default parameters, which the user could over write, for example by using hiera.

For these two reasons, I changed the module's current structure, replacing uchiwa::api with an array of hashes, passed to the init.pp class.

In other words, what the user could achieve by declaring...:

uchiwa::api { ' API 1':
  host    => '10.56.5.8',
}

uchiwa::api { 'API 2':
  host    => '10.16.1.25',
  ssl     => true,
  port    => 7654,
  user    => 'sensu',
  pass    => 'saBEnX8PQoyz2LG',
  path    => '/sensu',
  timeout => 5000
}

...now they can achieve by passing an array of hashes to the uchiwa class declaration:

class { 'uchiwa':
  sensu_api_endpoints  =>  [{
                              name => 'API 1',
                              host   => '10.56.5.8'
                              },
                              {
                              name => 'API 2',
                              host => '10.16.1.25',
                              ssl    => true,
                              port   => 7654,
                              user  => 'sensu',
                              pass  => 'saBEnX8PQoyz2LG',
                              path   => '/sensu',
                              timeout => 5000,
                           }],
}

Or data can be overwritten by hiera:

uchiwa::sensu_api_endpoints:
  - name: 'API 1'
    host: '10.56.5.8'
  - name: 'API 2'
    host: '10.16.1.25'
    ssl: true
    port: 7654
    user: 'sensu'
    pass: 'saBEnX8PQoyz2LG'
    path: '/sensu',
    timeout: 5000

If $sensu_api_endpoints is not populated when including the class, then the default params defined in params.pp will be used:

  $sensu_api_endpoints  = [{
                            name      =>  'sensu',
                            host      =>  '127.0.0.1', 
                            ssl       =>  false,
                            insecure  =>  false,
                            port      =>  4567,
                            user      =>  'sensu',
                            pass      =>  'sensu',
                            path      =>  '',
                            timeout   =>  5000,
                          }]

If a hash lacks of some params, the defaults are used instead. For example, if we include the above defined uchiwa class in the puppet catalog, the nicely-indented uchiwa.json file will look like this:

{
"sensu": [
    {
      "name": "API 1",
      "host": "10.56.5.8",
      "ssl": false,
      "insecure": false,
      "port": 4567,
      "user": "sensu",
      "pass": "sensu",
      "path": "",
      "timeout": 5000
    },
    {
      "name": "API 2",
      "host": "10.16.1.25",
      "ssl": true,
      "insecure": false,
      "port": 7654,
      "user": "sensu",
      "pass": "saBEnX8PQoyz2LG",
      "path": "/sensu",
      "timeout": 5000
    }
  ],
  "uchiwa": {
    "host": "localhost",
    "port": 3000,
    "user": "",
    "pass": "",
    "stats": 10,
    "refresh": 10000
  }
}

This PR introduces incompatible changes with previous versions of puppet-uchiwa. Unfortunately leaving both methods of passing the sensu API endpoint params turned out not to be a very trivial task.

Since we don't make use of it, I removed the richardc/datacat dependency from the module.

I have also added relevant beaker tests and updated the documentation.

If this PR is merged, then #19 is superseded and can be closed without merging.

solarkennedy commented 9 years ago

I think this is pretty solid.

At yelp we use the defined type to our advantage so we can declare all our apis with a single (pre-existing) array.

I think this method is a bit easier to understand, drops the datacat complexity, and allows much better, real, defaults.

Using hiera to describe this, does make configuring this module easier.

Leaving to @pauloconnor for a second opinion / shipit

queeno commented 9 years ago

Hi @solarkennedy

Thanks very much for your comment and I am indeed very happy you like my changes!

I have raised another couple of pull requests (#22 and #23) and updated this one.

Uchiwa 0.3.0 has introduced some changes about config params: stat is removed, timeout now defaults to 5 seconds (rather than ms!) and refresh defaults to 5 seconds (rather than 10000 ms!).

I have linked the uchiwa commit where those changes are made.

I would suggest releasing a new version of puppet-uchiwa as soon as all these PRs get merged!

Thanks

Simon

solarkennedy commented 9 years ago

@pauloconnor do you have an opinion? Otherwise I'm going to merge this. It does mean a refactor on our side to ensure our APIs are built in a compatible fashion.

pauloconnor commented 9 years ago

This makes the module configuration very similar to the method I had in v0.1.1. The reason why the define was implemented was to make automatic configuration much easier. How would you see automated configuration working with this change?

I appreciate the pull request, but I'm not sure the benefits out weigh breaking compatibility.

queeno commented 9 years ago

Hi @pauloconnor

Could you please clarify what you mean by automated configuration? Do you mean exported resources in PuppetDB?

The main benefit of my approach is to define sensu API default params rather than expecting the user to declare uchiwa::api at least once. Also, you can entirely drive uchiwa's configuration via hiera. Another benefit is to get rid of datacat and rely on the template function to build uchiwa.json.

solarkennedy commented 9 years ago

Yea, I personally think those are all good things.

What @pauloconnor means is, at yelp we declare our backends with an array and create_resources.

That is ok, we can do the same thing, but just the other way. We'll build the hash first, and then extract an array out of it for our other things with the keys function.

I'm sure we can do some sort of zip/flatten/merge/hash magic.

solarkennedy commented 9 years ago

@pauloconnor I'm going to go ahead and merge this. I believe it is a better design, long-term, for this module, even though ti means some short term pain for us as we have to re-do our profile. I'll do it.