Skip to content

Enable alias configuration similar to nginx::vhost #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

raoulbhatia
Copy link
Contributor

Enable alias configuration similar to nginx::vhost.

@javierbertoli
Copy link
Member

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

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

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

What I think that my pull request is lacking

  • refactoring from vhost::serveraliases to vhost:server_aliases
  • keep vhost::serveraliases working for now but issue a deprecation warning (how to do that properly?)
  • adding tests for server_aliases

Feedback welcome!

Cheers,
Raoul

@javierbertoli
Copy link
Member

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:

  • If both $serveraliases and $server_aliases are set, raise an error that you can only set one, and the preferred one is $server_aliases.
  • If serveraliases is set, raise a warning ([check here for an example) and move on.
  • Is just $server_aliases is set, we copy it's value to $serveraliases.

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

adding tests for server_aliases

That would be nice 😃

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

@raoulbhatia raoulbhatia force-pushed the server_alias branch 3 times, most recently from be290e7 to fc74e7b Compare February 27, 2015 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants