Feature/apache status, rework and improvments #8

ypid · 2017-01-09T18:36:05Z

@drybjed A short peek is probably enough. I did some more crazy (not too crazy actually) Jinja2 temperating stuff which you might find interesting 😉
I can merge it the next few days after I looked over it myself again. No hurry.

First should be the basics, followed by the advanced stuff.

s/apache__ref_vhosts_/apache__ref_vhost_/g

ypid · 2017-01-09T19:09:15Z

I will check the build error in a few days. Not yet sure about "RepresenterError: cannot represent an object:". If anyone has an idea.

drybjed · 2017-01-10T07:09:57Z

@ypid It seems to be an issue with Ansible, got another error like this in debops.bootstrap.

Related to: debops/ansible-owncloud#79 Related to: debops/ansible-apache#8

muelli · 2017-02-07T14:37:30Z

templates/etc/apache2/sites-available/apache__tpl_macros.j2

-    {{ item.root_directives | indent(4) }}
-{% endif %}
-
+    {{ debops__tpl_macros.indent(get_document_root_directives(item), 4) -}}
 </Directory>


please double check that the closing directive is on a new line.
I've checked out 5e76849 and it produces the closing Directory on the same line, i.e. Require all granted</Directory>. That makes apache unhappy: Expected </Directory> but saw </VirtualHost>\n"}. That reminds me of https://github.com/debops/ansible-postfix/issues/80

Thanks for the review. ~~I just double checked and it looks good.~~ If it makes Apache unhappy by failing to (re)start it that is easy to catch. Harder would be if it would not complain but do the wrong thing. Luckily, a syntax check already tells the problem in this case. But I can see the potential problem.

apache2ctl -t apache2: Syntax error on line 219 of /etc/apache2/apache2.conf: Syntax error on line 19 of /etc/apache2/sites-enabled/debops.apache-status.conf: Expected </Location> but saw </VirtualHost>

Ah, right, this happens in the CI test. Damn.

@muelli

Thanks to: @muelli Ref: debops#8 (comment)

drybjed

Very nice work. :-) Still some things I'd like to comment on.

drybjed · 2017-02-07T18:25:44Z

CHANGES.rst

+~~~~~~~
+
+- Change default virtual host server name from ``000-default`` to ``default.{{
+  apache__domain }}`` to increase the changes that a valid certificate is


Perhaps "increase the chances"?

drybjed · 2017-02-07T18:34:12Z

defaults/main.yml

+apache__domain: '{{ ansible_local.core.domain
+                    if (ansible_local|d() and ansible_local.core|d() and
+                        ansible_local.core.domain|d())
+                    else ansible_domain }}'


👍 Although to be on the safe side (no debops.core role present), I would finish this with:

else (ansible_domain if ansible_domain else ansible_hostname)

drybjed · 2017-02-07T18:47:16Z

handlers/main.yml

-  notify: [ 'Restart apache' ]
+# - name: Test apache and restart
+#   command: apache2ctl configtest
+#   notify: [ 'Restart apache' ]


Forgotten debugging? I would leave unused handlers uncommented, they shouldn't affect the role. Commented out code looks weird, IMO.

I understand but I don’t intent to release the role yet so I don’t mind that much. This is just to force me to edit this file and then I should notice the comment that I should also update the part of the docs which refers to this (as documented in this file as well). Thanks for the thoughtful review 👍

drybjed · 2017-02-07T18:49:32Z

tasks/main.yml

@@ -65,7 +38,7 @@
    group: 'root'
    mode: '0644'
  when: (item.value.state|d("present") != "absent" and
-         (item.value.type|d("default") not in ["divert"] or item.value.raw|d()))
+         (item.value.type|d("default") not in ["divert", "dont-create"] or item.value.raw|d()))


I'm leaning towards ignore option for this case, see for example debops.cron or debops.ifupdown role. Seems to better mesh with present and absent. Perhaps a parameter name change?

I noticed the ignore and I agree with it, however, I already tried to name the option as good as possible and dont-create does exactly what it says. With this, snippets can still be enabled and disabled, but the role does not try to create the template for them. This is why I thought making it explicit by this name is preferred. ignore does not really hit the nail on the head in this case 😉

ypid · 2017-02-07T19:22:14Z

@muelli All updated. Can you update your review? Thanks again!

muelli · 2017-02-08T20:46:24Z

templates/etc/apache2/sites-available/debops__tpl_macros.j2

@@ -66,3 +96,13 @@
       ansible_local.pki.gnutls_version|d())
   else "0.0.0" }}
 {% endmacro %}{# ]]] #}
+
+{% macro indent(content, width=4, indentfirst=False) %}{# [[[ #}


just a thought: Would Jinja be interested in providing such a filter directly? Has there been a discussion, i.e. bug report?

Good question. Done

muelli · 2017-02-08T20:49:44Z

CHANGES.rst

+Changed
+~~~~~~~
+
+- Change default virtual host server name from ``000-default`` to ``default.{{


How much does debops care about users who upgrade this role? E.g. should it be mentioned what the user has to do to get rid of old, unmaintained files? In this case, it seems that 000-default needs to be deleted manually.

ah. no. sorry, I was confused.

Before you delete any files, check if they are configuration files included in a package:

dpkg-query -s apache2 dpkg -S /etc/apache2/sites-available/000-default

If so, you shouldn't remove them. This might hinder future upgrades, automated or not. Instead, you should divert the affected files either in the same directory with a different suffix (usually .dpkg-divert) or to another directory altogether so that the package manager still knows that they exist and can perform the package upgrades correctly.

This is different with the configuration files generated on the package installation dynamically, or managed by the ucf system. In this case, you should check the postinstall script of a given package for some clues how to handle this.

This does not change the filename and thus does not leave unmaintained files danging around. But to your question, sure, something like this would be noted if it can not be avoided (which would be preferred obviously). But this does not necessarily apply to a unreleased role but thanks for checking back 😉

Ref: debops/docs#160

@drybjed The role already takes care of this 😉

ypid · 2017-02-10T08:10:27Z

@muelli Does it work for you now?

muelli · 2017-02-10T10:01:54Z

yeah, it seems to work :)
I'm wondering though if it's possible to get rid of the template macro code by providing separate templates. But that can also be done later.

ypid · 2017-02-10T10:21:42Z

You mean to stop using macros and do it using multiple templates? I don’t think that would be a good idea in terms of maintainability. I don’t like the way some older DebOps roles handle this. All that redundancy 😉

ypid · 2017-02-11T11:39:40Z

@muelli Can you update your GitHub review?

muelli · 2017-02-11T11:49:42Z

ah. hm. how do I do that?

ypid · 2017-02-11T12:18:50Z

https://github.com/debops/ansible-apache/pull/8/files → "Review changes"

ypid · 2017-02-11T13:39:25Z

Thanks guys, merged.

ypid added 12 commits January 4, 2017 13:21

Add the apache__mpm_max_connections_per_child variable

76dcb7a

Move OCSP Stapling default vars below basic TLS configuration

98e39f0

First should be the basics, followed by the advanced stuff.

Ensure that the shared object cache provider is loaded when required

6d97fe5

Change default virtual host to default.{{ + apache__domain }}

014abc8

Add support to enable and configure Apache mod_status

83d3086

Support generic Apache template generation using the IfVersion directive

dccad54

Regen README

c6d7cb8

Fix indention of directives applied to the root dir of vhosts

7cc530d

Fix spelling in docs

3f7d6ab

Change apache__hsts_preload from True to False by default

15a1227

Fix spelling of apache__snippets type introduced in this branch

505e6f2

Use singular apache__ref_vhost_ label as docs anchor

bbd9061

s/apache__ref_vhosts_/apache__ref_vhost_/g

ypid added bug enhancement labels Jan 9, 2017

ypid requested a review from drybjed January 9, 2017 18:36

ypid added 2 commits January 13, 2017 11:40

Fix role run with apache__deploy_state: absent after present was set

2a0c2b6

Add Ansible tags for env roles

e7e00ba

ypid added a commit to ypid/debops-playbooks that referenced this pull request Jan 17, 2017

Update Ansible tags for ownCloud and Apache playbooks

fb8cd7e

Related to: debops/ansible-owncloud#79 Related to: debops/ansible-apache#8

ypid mentioned this pull request Jan 17, 2017

Update Ansible tags for ownCloud and Apache playbooks debops/debops-playbooks#332

Merged

Add missing filename suffixes in docs

5e76849

muelli suggested changes Feb 7, 2017

View reviewed changes

ypid added 3 commits February 7, 2017 17:36

Fix spelling

9354f6d

Fix missing newline in configuration which produced invalid syntax

185e1bf

Thanks to: @muelli Ref: debops#8 (comment)

Improve spelling and indention

b1231f7

This was referenced Feb 7, 2017

Added apache__redirect_to_https variable #9

Merged

Do not https_redirect 000-default vhost #11

Closed

drybjed requested changes Feb 7, 2017

View reviewed changes

ypid force-pushed the feature/apache_status branch from f1faaa9 to c90da65 Compare February 7, 2017 18:58

ypid force-pushed the feature/apache_status branch from c90da65 to 5f0bebb Compare February 7, 2017 19:02

ypid added 2 commits February 7, 2017 20:02

Ensure that the rewrite module is loaded when required by the role

5f0bebb

Incorporate review by @drybjed, thanks!

04d02e3

drybjed approved these changes Feb 7, 2017

View reviewed changes

Prepare debops__tpl_macros.j2 upstreaming

22d4718

muelli reviewed Feb 8, 2017

View reviewed changes

muelli approved these changes Feb 11, 2017

View reviewed changes

ypid merged commit 22d4718 into debops:master Feb 11, 2017

ypid added a commit that referenced this pull request Feb 11, 2017

Merge branch 'feature/apache_status'. Merge #8.

4e67545

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/apache status, rework and improvments #8

Feature/apache status, rework and improvments #8

ypid commented Jan 9, 2017 •

edited

Loading

ypid commented Jan 9, 2017

drybjed commented Jan 10, 2017

muelli Feb 7, 2017

ypid Feb 7, 2017 •

edited

Loading

drybjed left a comment

drybjed Feb 7, 2017

drybjed Feb 7, 2017

drybjed Feb 7, 2017

ypid Feb 7, 2017

drybjed Feb 7, 2017

ypid Feb 7, 2017

drybjed Feb 7, 2017

ypid commented Feb 7, 2017

muelli Feb 8, 2017

ypid Feb 8, 2017

muelli Feb 8, 2017

muelli Feb 8, 2017

drybjed Feb 8, 2017

ypid Feb 8, 2017

ypid Feb 8, 2017

ypid commented Feb 10, 2017

muelli commented Feb 10, 2017

ypid commented Feb 10, 2017

ypid commented Feb 11, 2017

muelli commented Feb 11, 2017

ypid commented Feb 11, 2017

ypid commented Feb 11, 2017

Feature/apache status, rework and improvments #8

Feature/apache status, rework and improvments #8

Conversation

ypid commented Jan 9, 2017 • edited Loading

ypid commented Jan 9, 2017

drybjed commented Jan 10, 2017

Choose a reason for hiding this comment

ypid Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

drybjed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypid commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypid commented Feb 10, 2017

muelli commented Feb 10, 2017

ypid commented Feb 10, 2017

ypid commented Feb 11, 2017

muelli commented Feb 11, 2017

ypid commented Feb 11, 2017

ypid commented Feb 11, 2017

ypid commented Jan 9, 2017 •

edited

Loading

ypid Feb 7, 2017 •

edited

Loading