search

bflad / chef-stash

Chef Cookbook for Atlassian Stash
Other
37 stars 42 forks source link

Fixes for the Stash library #44

Closed legal90 closed 9 years ago

legal90 commented 9 years ago
1. Default settings should not be specified in attribute files

Chef::Recipe::Stash.settings method is going to set default settings:

settings ||= node['stash']
settings['database']['port'] ||= default_database_port settings['database']['type']
settings['database']['testInterval'] ||= 2

But it will never be done because the node will always have these attributes specified in attributes/default.rb. For example, even if we set ['database']['type'] = 'postgresql', the port will be "3306" anyway. So that, these attributes should not be specified in attribute file.

2. default_database_port method is unavailable

This method should be a class method, instead of an instance method. Otherwise, it can not be called from Chef::Recipe::Stash.settings method

3. settings should be a hash.

While copying settings ||= node['stash'] it should be also converted to Hash object, becouse Node object is read-only and overriding below will be unavailable.

legal90 commented 9 years ago

@bflad, Please, check this out. The Travis build failure is caused by updated Rubocop policies and is not related to this PR directly.

legal90 commented 9 years ago

I've just improved the definition of Stash settings in library/stash.rb: Now default settings can be defined by node attributes and some of them can be overwritten by values from data bag.

legal90 commented 9 years ago

I've fixed some issues by applying the deep merge to hashes. Now it is working fine, look at the example below. Let's assume, that node has the following attributes:

# Attributes
node.set['stash']['database']['host']     = 'localhost'
node.set['stash']['database']['name']     = 'stash'
node.set['stash']['database']['password'] = 'changeit'
node.set['stash']['database']['user']     = 'stash'
...
node.set['stash']['jvm']['minimum_memory']  = '512m'
node.set['stash']['jvm']['maximum_memory']  = '768m'

At the same time there is a stash data bag containing the following settings:

# Data bag
{
  "id": "stash",
  "test": {
    "database": {
      "user": "overridden_user",
      "password": "overridden_password"
    },
    "jvm": {
      "maximum_memory": "2048m"
    }
  }
}

In the result values from attributes will be overridden by settings from the data bag:

# Result settings:
settings['database']['host']     == 'localhost'
settings['database']['name']     == 'stash'
settings['database']['password'] == 'overridden_password'
settings['database']['user']     == 'overridden_user'
...
settings['jvm']['minimum_memory']  = '512m'
settings['jvm']['maximum_memory']  = '2048m'
bflad commented 9 years ago

@legal90 this is an excellent improvement - thanks! Sorry it took me so long to get this in here. I double checked the README documentation and it already mentions the override behavior which this actually makes work as expected.

bflad commented 9 years ago

Will release in 3.13.0