zuazo / owncloud-cookbook

Chef cookbook to install and configure ownCloud.
https://supermarket.chef.io/cookbooks/owncloud
Apache License 2.0
37 stars 31 forks source link

support non standard postgresql port #23

Closed MichaelSp closed 9 years ago

zuazo commented 9 years ago

Hi @MichaelSp,

Thanks for the PR and sorry for the delay :smile:

After reviewing it, I think some things are missing:

My opinion about some changes you propose:

I think It would be better to set the port attribute to nil and then calculate it within the recipe:

# attributes/default.rb:
default['owncloud']['config']['dbport'] = nil

Then, in the owncloud::default recipe, something similar to the following (untested code):

# recipes/default.rb:
when 'mysql'
  if node['owncloud']['config']['dbport'].nil?
    # Avoid using `Node#override` here
    node.default['owncloud']['config']['dbport'] = '3306'
  end
  # [...]
  mysql_service dbinstance do
    # [...]
    port node['owncloud']['config']['dbport']
  end
  mysql_connection_info = {
    :host => '127.0.0.1',
    :port => node['owncloud']['config']['dbport'],
    :username => 'root',
    :password => node['owncloud']['database']['rootpassword']
  }
  # [...]
when 'pgsql'
  if node['owncloud']['config']['dbport'].nil?
    node.default['owncloud']['config']['dbport'] = node['postgresql']['config']['port']
  else
    node.default['postgresql']['config']['port'] = node['owncloud']['config']['dbport']
  end
  include_recipe 'postgresql::server'
  # [...]
  postgresql_connection_info = {
    :host => 'localhost',
    :port => node['owncloud']['config']['dbport'],
    :username => 'postgres',
    :password => node['postgresql']['password']['postgres']
  }
# [...]

template 'autoconfig.php' do
  # [...]
  variables(
    # Avoid using both `"#{}"` and `+` here:
    :dbhost => "#{node['owncloud']['config']['dbhost']}:#{node['owncloud']['config']['dbport']}",
    # [...]
  )
end

What do you think?

MichaelSp commented 9 years ago

Sounds good to me. I incorporated your suggestions. Sorry but I can't write the test as I'm totally new to this. Would take too much time to do this.

zuazo commented 9 years ago

OK, thanks @MichaelSp. I fixed your PR in the 170358e commit and added some integration tests.

zuazo commented 9 years ago

Released in 1.0.0.