Skip to content

Added apache__redirect_to_https variable #9

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.

Sign up for GitHub

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

Merged
ypid merged 4 commits into from
Feb 24, 2017
Merged

Added apache__redirect_to_https variable #9

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
146a3ec
fixed simple typo in apache template macros
muelli Feb 10, 2017
a249cf5
Added apache__redirect_to_https variable
muelli Feb 7, 2017
1982f74
added a changelog entry
muelli Feb 10, 2017
136af82
fixup whitespace issues reported by linter
muelli Feb 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ debops.apache v0.1.0 - unreleased
Added
~~~~~

- Add :envvar:`apache__redirect_to_https` to control the role's default behaviour for
redirecting to https. [muelli_]

@ypid ypid Feb 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are not yet defined in 80post.rst, feel free to add yourself to the file and run make includes/global.rst to get docs working.

Added a apache__redirect_to_https to control →

Add :envvar:`apache__redirect_to_https to control`

or

Add ``apache__redirect_to_https to control``

and debops-optimize will take care of it.

@muelli muelli Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hrm. creating that entry seems to require a non trivial addition to debops-keyring, too. Is the procedure defined anywhere? Anyway, I've tried to create the necessary pull requests.

But there is no change the file here requires, right?

@ypid ypid Feb 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, no change is needed in https://github.com/debops/ansible-apache.


- Initial coding and design. [ypid_]

- Add/Set the default `Referrer Policy`_ to ``no-referrer`` and made it
Expand Down
7 changes: 7 additions & 0 deletions defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,14 @@ apache__combined_snippets: '{{ apache__dependent_snippets
apache__https_enabled: '{{ ansible_local|d() and ansible_local.pki|d() and
(ansible_local.pki.enabled|d() | bool) and
apache__https_listen|length > 0 }}'
# ]]]

@ypid ypid Feb 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a linting tool available called yaml4rst which implements the current format. Can you give it a try?

@muelli muelli Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hm. doesn't look too good:

>/tmp/ansiblepip/bin/yaml4rst -e ansible_full_role_name=debops.apache defaults/main.yml 
Traceback (most recent call last):
  File "/tmp/ansiblepip/bin/yaml4rst", line 11, in <module>
    load_entry_point('yaml4rst==0.1.3', 'console_scripts', 'yaml4rst')()
  File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/cli.py", line 145, in main
    reformatter.reformat()
  File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/reformatter.py", line 150, in reformat
    yaml.load(self.get_content())
  File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/reformatter.py", line 116, in get_content
    return '\n'.join(self._lines)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 24: ordinal not in range(128)

@ypid ypid Feb 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems to be that yaml4rst is run by Python2 which is unsupported. Please run it with Python3.

@ypid ypid Feb 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you need further help? Have you had time to check with Python3?


# .. envvar:: apache__redirect_to_https [[[
#
# This defines the default for each vhost's ``redirect_to_https`` variable.
# Defaults to ``True``.

@ypid ypid Feb 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Defaults to True.

Seems redundant as the variable is a simple assignment it is clear what the default is 😉

@muelli muelli Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah. I wasn't sure how to handle that. I think I wanted to express that the ultimate decision is made based on the other variable but also make the documentation useful, e.g. to make it show the actual value that's relevant to the user. But I don't like the redundancy.
What do you think should be done here? Make it Defaults to apache__redirect_to_https and remove the previous sentence?

@ypid ypid Feb 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the defaults/main.yml and basically the last resort default, there is no more fallback default for this. You can simply remove the "Defaults to True." sentence I would say.

@muelli muelli Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've made a change. I hope it's better now.

apache__redirect_to_https: True
# ]]]
# PKI [[[
# ~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion docs/defaults-detailed.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ Refer to the `Apache Redirect directive documentation`_ for details.
HTTPS, by default 301 Moved Permanently.

``redirect_to_https``
Optional, boolean. Defaults to ``True``
Optional, boolean. Defaults to ``apache__redirect_to_https``
If ``True``, redirect connection from HTTP to the HTTPS version of the site.
Set to ``False`` to allow to serve the website via HTTP and HTTPS and don't
redirect HTTP to HTTPS.
Expand Down
6 changes: 3 additions & 3 deletions templates/etc/apache2/sites-available/apache__tpl_macros.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ ErrorLog ${APACHE_LOG_DIR}/{{ sanitized_name }}_error.log
{% set apache__tpl_status_enabled = item.status_enabled|d(apache__status_for_vhost_enabled) | bool %}
{{ get_server_status_directives(item, apache__tpl_status_enabled) -}}
{% if apache__tpl_status_enabled|bool and (
(mode == 'http' and (item.redirect_http|d() or item.redirect_to_https|d(https_enabled) | bool)) or
(mode == 'http' and (item.redirect_http|d() or item.redirect_to_https|d(apache__redirect_to_https) | bool)) or
(mode == 'https' and item.redirect_https|d())
) %}
RewriteEngine On
Expand All @@ -136,7 +136,7 @@ RewriteRule "^{{ item.status_location|d(apache__status_location) }}" "-" [L]
{% endif %}
{% if mode == 'http' and item.redirect_http|d() %}
{{ get_redirect(item.redirect_http_code|d(307), "/", item.redirect_http, apache__tpl_use_redirect_module) }}
{% elif mode == 'http' and item.redirect_to_https|d(https_enabled) | bool %}
{% elif mode == 'http' and item.redirect_to_https|d(apache__redirect_to_https) | bool %}
{{ get_redirect(item.redirect_to_https_with_code|d("301"), "/", "https://" + (debops__tpl_macros.get_yaml_list_for_elem(item.name) | from_yaml)[0], apache__tpl_use_redirect_module) }}
{% elif mode == 'https' and item.redirect_https|d() %}
{{ get_redirect(item.redirect_https_code|d(307), "/", item.redirect_https, apache__tpl_use_redirect_module) }}
Expand Down Expand Up @@ -255,7 +255,7 @@ Header {{ apache__tpl_directive_options }} X-Content-Type-Options "{{ item.http_
{% macro get_common_headers(item) %}{# [[[ #}
{% if item.http_clacks_overhead|d(apache__http_clacks_overhead|d(True)) | bool %}
{#
# Respect the will of the DebOps Creater.
# Respect the will of the DebOps Creator.
# Ref: https://github.com/debops/ansible-nginx/commit/d6cd455c68a7584b2592053fd98d3e539054e09a
#}
Header always set X-Clacks-Overhead "GNU Terry Pratchett"
Expand Down