Skip to content
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

Rename all vhost ocurrences to server #968

Closed
wants to merge 1 commit into from

Conversation

DavidS
Copy link

@DavidS DavidS commented Nov 10, 2016

This closes #348 and closes #932. It was created by running

git ls-files | xargs -i{} sed -i -e 's/vhost/server/g' {}

and manually fixing lint issues and renaming all files called 'vhost'.

This closes voxpupuli#348 and closes voxpupuli#932. It was created by running

    git ls-files | xargs -i{} sed -i -e 's/vhost/server/g' {}

and manually fixing lint issues and renaming all files called '*vhost*'.
@DavidS
Copy link
Author

DavidS commented Nov 10, 2016

cc @wyardley

@wyardley
Copy link
Collaborator

@DavidS Can't figure out how to submit a PR against your fork, but these are my formatting changes: https://github.com/wyardley/puppet-nginx/tree/rename_vhost_server_davids

Also got a couple instances of vHost.

FWIW, spec tests are passing.

@3flex
Copy link
Contributor

3flex commented Nov 11, 2016

Food for thought: the "server" block can be used in several contexts in nginx...

  • ngx_http_core_module
  • ngx_http_upstream_module
  • ngx_mail_core_module
  • ngx_stream_core_module
  • ngx_stream_upstream_module

This module supports some (all?) of those. Of course most people will assume a "server" will configure an HTTP server since that's most common, but perhaps resources should be namespaced somehow e.g. nginx::http::server and nginx::mail::server or something like that so that it's clear what type of server is being created.

I only mention because nginx::resource::mailhost also exists and that creates a server block too, but in the mail context. If you're renaming to align the resource name to the nginx config name then mailhost, streamhost and the upstream member(s) should be looked at too.

@wyardley
Copy link
Collaborator

@3flex Yeah, I'm not sure if there's an easy way to handle that. I agree that it's a little confusing, though maybe we should just do the great renaming (which is going to have to be re-done now, by the way, since stuff got merged), and make any other changes iteratively?

If it were namespaced nginx::http::server, how would we deal with situations like
nginx::nginx_vhosts? Would they become nginx::nginx_http_servers?

@hunner
Copy link
Member

hunner commented Nov 17, 2016

This needs a rebase (or a rerun of the sed)

@3flex
Copy link
Contributor

3flex commented Nov 18, 2016

I'd remove the "nginx" prefix from the parameter and have nginx::http_servers.

For the future, I THINK this makes sense...

  • Resources
    • nginx::resource::geo => nginx::http::geo
    • nginx::resource::location => nginx::http::location (this one's a bit funny, it doesn't quite match the pattern of the others)
    • nginx::resource::mailhost => nginx::mail::server
    • nginx::resource::map => nginx::http::map
    • nginx::resource::streamhost => nginx::stream::server
    • nginx::resource::upstream => nginx::http::upstream
    • nginx::resource::vhost => nginx::http::server
    • nginx::resource::upstream::member => nginx::http::upstream::server
  • Parameters on nginx class
    • geo_mappings => http_geos
    • string_mappings => http_maps
    • nginx_locations => http_locations (again, a bit funny...)
    • nginx_mailhosts => mail_servers
    • nginx_streamhosts => stream_servers
    • nginx_upstreams => http_upstreams
    • nginx_vhosts => http_servers
    • nginx_vhosts_defaults => http_servers_defaults

This is just one option, I'm not overly attached, but it's now very explicit as to what each resource will configure. One inconsistency with the above is locations, perhaps could go with nginx::http::server::location?

@sacres
Copy link
Member

sacres commented Nov 30, 2016

@wyardley Submitted PR #980 to address this for the rebase/sed and [vV]host

@wyardley
Copy link
Collaborator

Replaced by #980

@wyardley wyardley closed this Nov 30, 2016
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.

Rename vhost to server.d
5 participants