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

Require explicit redirects and drop www_redirect #622

Merged
merged 5 commits into from
Jul 27, 2016
Merged

Require explicit redirects and drop www_redirect #622

merged 5 commits into from
Jul 27, 2016

Conversation

fullyint
Copy link
Contributor

Discontinues the reverse_www filter, overcoming its challenges in determining when to add/remove www in presence of subdomains.

Some history in #570 and #619.

Example of problem to solve: https://discourse.roots.io/t/https-www-unsecure-and-not-redirecting/7071/

site_hosts:
- canonical: example.com

The above is the minimum required. Multiple hosts and redirects are posible:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible


if Vagrant.has_plugin? 'vagrant-hostmanager'
config.hostmanager.enabled = true
config.hostmanager.manage_host = true
config.hostmanager.aliases = aliases + www_aliases
config.hostmanager.aliases = aliases + redirects
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  site_hosts = wordpress_sites.flat_map { |(_name, site)| site['site_hosts'] }

  site_hosts.each do |host|
    if !host.is_a?(Hash) or !host.has_key?('canonical')
      fail_with_message File.read(File.join(ANSIBLE_PATH, 'roles/common/templates/site_hosts.j2')).sub!('{{ env }}', 'development').gsub!(/com$/, 'dev')
    end
  end

  main_hostname, *hostnames = site_hosts.map { |host| host['canonical'] }
  config.vm.hostname = main_hostname

  redirects = site_hosts.flat_map { |host| host['redirects'] }.compact

  if Vagrant.has_plugin? 'vagrant-hostmanager'
    config.hostmanager.enabled = true
    config.hostmanager.manage_host = true
    config.hostmanager.aliases = hostnames + redirects
  else
    fail_with_message "vagrant-hostmanager missing, please install the plugin with this command:\nvagrant plugin install vagrant-hostmanager"
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your refactor of site_hosts mapping is sooo much better. Thanks!

@@ -1,5 +1,5 @@
server {
listen 80;
server_name {{ item.item.value.site_hosts | reverse_www(enabled=item.item.value.www_redirect | default(true)) | join(' ') }};
server_name{% for item in item.item.value.site_hosts %} {{ item.canonical }}{% for redirect in item.redirects | default([]) %} {{ redirect }}{% endfor %}{% endfor %};
Copy link
Member

@swalkinshaw swalkinshaw Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be site_hosts | join(' ')?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disappointment here is that it uses item.item.value.site_hosts.
We would need just item.value.site_hosts in order to use site_hosts from helpers.yml.

I tried opening the template with {% set item = item.item %} but it doesn't work.
If it feels cleaner or more consistent, we could open the template with
{% set canonical = ... [something] %}
{% set redirects = ... [something] %}
{% set site_hosts = ... [something with union] %}
and then do the site_hosts | join(' ')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth it, good as is 👌

@swalkinshaw
Copy link
Member

💯 so much better

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