Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Fixes for the Stash library #44

Merged
merged 4 commits into from
Jan 23, 2015
Merged

Conversation

legal90
Copy link
Contributor

@legal90 legal90 commented Nov 11, 2014

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.

Since the "settings" library method is used, these attributes should not be defined in attribute files.
Since node object is read-only, 'settings' should be a hash.
@legal90
Copy link
Contributor Author

legal90 commented Nov 17, 2014

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

If collision occurs, then values from data bag have a higher priority than attribute values.
@legal90
Copy link
Contributor Author

legal90 commented Nov 29, 2014

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
Copy link
Contributor Author

legal90 commented Dec 2, 2014

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
Copy link
Owner

bflad commented Jan 23, 2015

@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
Copy link
Owner

bflad commented Jan 23, 2015

Will release in 3.13.0

bflad added a commit that referenced this pull request Jan 23, 2015
@bflad bflad merged commit 247ab07 into bflad:master Jan 23, 2015
@legal90 legal90 deleted the fix-database-attributes branch January 23, 2015 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants