benlangfeld / freeswitch-cookbook

Chef cookbook to install FreeSWITCH
24 stars 17 forks source link

local_ip_v4 #12

Closed wtcross closed 10 years ago

wtcross commented 10 years ago

Hi,

In vars.conf.xml FreeSWITCH will set local_ip_v4 dynamically if it is not present. This is true for several other variables as well.

FreeSWITCH vars.conf.xml Documentation

I am using Packer for the creation of reusable images targeting different platforms. When I build an image the IP address that it uses during the build is saved in the image. I have to remove the following two lines during creation of the image after this cookbook is used:

<X-PRE-PROCESS cmd="set" data="local_ip_v4=<%= @local_ip_v4 %>"/>
<X-PRE-PROCESS cmd="set" data="domain=<%= @domain %>"/>

This is not such a big deal, but I wanted to ask if you think that providing a flag through attributes that could disable setting local_ip_v4 in the vars.xml.erb would be acceptable to your project. This would enable FreeSWITCH to detect local_ip_v4 wherever I deploy the image. Of course, I wouldn't want to always disable domain. In my use case I do, but only local_ip_v4 would be affected by this change.

I would be more than happy to make a pull request if this sounds good to you.

benlangfeld commented 10 years ago

I think a nil value for these attributes should have the behaviour you describe, while retaining the default. I'd be happy to accept a PR to this effect.

wtcross commented 10 years ago

When setting the default attributes you are doing the following:

default['freeswitch']['domain'] = node['fqdn']
default['freeswitch']['local_ip'] = node['ec2'] ? node['ec2']['public_ipv4'] : node['ipaddress']

How do you feel about making it possible to do the following in the node JSON in order to disable automatically detecting it from the node?

{
    "freeswitch": {
        "domain": false,
        "local_ip": false
    }
}

I like the way it works now for almost all cases, but would like to be able to override that. I'm learning Chef and want to make sure my change is following best practices.

Here is the change I made to the template

benlangfeld commented 10 years ago

The change you've proposed looks fine. It requires setting these values to nil, not false, but I prefer nil. I would, however, like to make sure this has test coverage (see .kitchen.yml and test/) since it's adding some logic to the template which relies on Chef attribute precedence, which might be a bit fiddly when talking about nil override.

wtcross commented 10 years ago

I see. I'll definitely add tests before I submit a pull request. Thanks for your tips. Apparently I accidentally closed this issue before. Reopened.

wtcross commented 10 years ago

If I set domain and local_ip to null in the node object they aren't used. Do you know if Chef ignores null attributes in the node object?

benlangfeld commented 10 years ago

Honestly, I don't, and that's exactly why I suggested going test-first to make sure. If nil/null is not possible, then I guess false it must be...

wtcross commented 10 years ago

I'm going to close this because I found a better solution and the amount of code that I wrote for it was almost none. The tests weren't as easy as expected with serverspec.

benlangfeld commented 10 years ago

What was the solution?