netmanagers / puppet-nginx

[UNMAINTAINED] Puppet Nginx module
http://www.example42.com
Other
37 stars 60 forks source link

Enable alias configuration similar to nginx::vhost #63

Open raoulbhatia opened 10 years ago

raoulbhatia commented 10 years ago

Enable alias configuration similar to nginx::vhost.

javierbertoli commented 10 years ago

@raoulbhatia, nginx::vhost has $serveraliases as the name of the parameter, while you're adding $server_aliases. Although I like it better the way you wrote, I'd accept both as aliases (with a deprecation warning to serveraliases, probably), or either just leave it the same as in nginx::vhost in order to, sooner or later, unify all the management of vhosts in a single place.

Also, as we are currently doing in the bacula module, we might accept either a string or an array, check that and convert it to an array to accept multiple values (see here in the manifest and the template)

What do you think? Does this suit you? Do you want me to write it?

raoulbhatia commented 10 years ago

Hi @javierbertoli and thank you for your feedback.

I would be in favor in renaming $serveraliases to $server_aliases in nginx::vhost and to add a deprecation warning there. However, I do not see a need for adding serveraliases to nginx::resource::vhost.

I think that it is a good idea to allow either string or array. I would like to take a look at this myself first (to learn more about puppet and ruby) and will update the pull request accordingly.

Cheers, Raoul

raoulbhatia commented 10 years ago

Hi. I tried to update my pull request accordingly - is this how you envision it?

What I think that my pull request is lacking

Feedback welcome!

Cheers, Raoul

javierbertoli commented 10 years ago

Hi. I tried to update my pull request accordingly - is this how you envision it?

Yes, exactly.

What I think that my pull request is lacking

refactoring from vhost::serveraliases to vhost:server_aliases

Yes.

keep vhost::serveraliases working for now but issue a deprecation warning (how to do that properly?)

I would, for now, expose both variables ($serveraliases and $server_aliases) and do the needed work to, in the end, make all the code of the module work with $serveraliases:

This will end adding the new format while being backward compatible.

adding tests for server_aliases

That would be nice :smiley:

If you just want me to give you a hand or write some parts of these, let me know.