-
Notifications
You must be signed in to change notification settings - Fork 2
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
Role rework, update to latest Roundcube 1.3 #24
Conversation
Fixes "Uncaught Error: Class 'PEAR' not found in /srv/www/roundcube/sites/roundcube.example.com/public/program/lib/Roundcube/bootstrap.php:101"
Add new variables 'roundcube__database_password_path' and 'roundcube__database_name'.
Fixes "Call to undefined function mb_strtolower()"
Very nice work! 👍 I'll look at it in a few days while working on Dovecot support. |
@cultcom If you like, can you also have a look at the proposed changes? I didn't had time yet, to add an installation method via archive download therefore I couldn't really implement a fix for #17 I know, the PHP package installation is still kind of a mess. Unfortunately it seems that there is now way that we can still support Jessie or comparably old Ubuntu releases as their PHP dependencies are too old and they miss PHP composer. Not sure yet how to handle that. If you want to test, just make sure, that you have Debian Stretch or something comparable new. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! There are some changes I would like to see, check the other comments.
I'm fine with sticking to the Debian Stable release and dropping support for Oldstable due to composer
support. It will happen sooner or later, and since the role is not yet part of the DebOps core, I think that's fine.
defaults/main.yml
Outdated
# - 'openssl' | ||
# - 'session' | ||
# - 'sockets' | ||
# - 'xml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how that will be parsed by yaml2rst
. Perhaps a better way would be to move the list of included packages to the comment above, and make it an inline list instead of one entry for a line, which should make the comment smaller. Ditto for other instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied that from debops.owncloud
. Will have a look at it.
defaults/main.yml
Outdated
# ----------------------------------- | ||
# List of additional Debian packages (e. g. language dictionaries) that should | ||
# be installed with Roundcube. | ||
roundcube__extra_packages: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention I'm using is to have the list of packages in a variable named with a prefix, ie.:
roundcube__base_packages: [ 'curl', 'file', 'unzip' ]
Then the users are free to use the "normal" variation in an inventory if they wish:
roundcube__packages: []
defaults/main.yml
Outdated
# | ||
# ----------------------------- | ||
# Packages and installation | ||
# ----------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could convert the defaults/main.yml
to the current standard by using the section headers with just one line underneath, and include vim
fold markers. The whole file should also be enclosed in vim
folds with a file header. See for example debops.apt
role.
defaults/main.yml
Outdated
|
||
# .. envvar:: roundcube__git_checkout | ||
# ]]] | ||
# .. envvar:: roundcube__git_checkout [[[ | ||
# | ||
# Default path where Roundcube source files will be deployed | ||
roundcube__git_checkout: '{{ roundcube__www + "/sites/" + | ||
(roundcube__domain if roundcube__domain is string else roundcube__domain[0]) + | ||
"/public" }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps using a domain directly here should be dropped, since the role maintains just one Roundcube install. This would also avoid issues if the roundcube__domain
value is changed. Multiple Roundcube installations on one host could be handled via internal LXC containers if necessary.
defaults/main.yml
Outdated
roundcube__database_user: 'roundcube' | ||
|
||
# Database definition to use from the :envvar:`roundcube__database_map`. | ||
roundcube__database: 'sqlite-default' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily in this PR, but role could check if a MariaDB database is installed on the host via Ansible facts and switch to MariaDB database automatically - one less thing to handle via inventory. You can see how it's done in debops.gitlab
role.
roundcube__database_password_path: '{{ secret + "/credentials/" + ansible_fqdn | ||
+ "/roundcube/" + roundcube__database | ||
+ "/" + roundcube__database_user + "/password" }}' | ||
# ]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the database user account is handled by debops.mariadb
role, the password stored in the secret/
directory is in a different directory. Did you test this PR with a MariaDB database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand. This would give the debops.mariadb
role the path to store the password. Works perfectly fine.
See:
- https://github.com/debops-contrib/ansible-roundcube/pull/24/files#diff-7eeda618087b49ae876084ab6c73fdbbR232
- https://github.com/ganto/ansible-roundcube/blob/5e8c63943219bf8fe298389b18ccb8ce60344a7b/defaults/main.yml#L255
- https://github.com/debops-contrib/ansible-roundcube/pull/24/files#diff-5c059070706cb70613b3010f4f0e679eR51
defaults/main.yml
Outdated
fastcgi_busy_buffers_size 256k; | ||
fastcgi_temp_file_write_size 256k; | ||
|
||
# ]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the YAML text block above, the vim
fold marker will be included in the configuration file. It won't prevent nginx
from working but looks wonky. Put the php_upstream
key below the YAML text block to avoid this.
@@ -0,0 +1,2 @@ | |||
{% set _composer_json = (roundcube__register_composer_json.stdout | from_json) %} | |||
{{ _composer_json | to_nice_json }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? It seems to be a roundabout way to copy the contents of the dist
file to another file right next to it. Won't simple cp
or even fancy copy
module with remote_src: True
do the trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not fully sure 😉 No, of course at the moment not. But I wanted to have a way to influence the composer.json
so that I can remove/add some packages according to our needs. E.g. add the LDAP driver. The idea was, to install as much as possible via package manager and only the remaining via composer. But I didn't test yet, if that works.
Hmn, after updating the issue in the
|
The role should be in an acceptable shape now. It is able to successfully setup Roundcube. Some tasks that are still giving me some headache are the PHP composer and JS downloads. They are not especially safe especially regarding man-in-the-middle and I don't know yet, ho well they can handle a Roundcube update. Any suggestions are welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work!
There was one more issue popping up. When upgrading the release, Roundcube needs to run a
When this is fixed, I'm confident, that this PR is in a state where it can be merged. |
@drybjed or @ypid: Could you please trigger the documentation rebuild of https://debops-contrib.readthedocs.io/en/latest/? Thanks |
Very well done @ganto ! Done and up and running. Enjoy :) |
Thanks, glad you like it 👍 |
As @drybjed has started refreshing the mail stack, I thought it would finally be time to also update the roles that I contributed. Starting with the (hopefully) easier one, I reworked the Roundcube role which is currently stuck at upstream release
1.1.x
so that it can finally also install the latest1.3.0
release (Fixes #19).I also updated the DebOps dependencies and ported the role to
debops.php
(Fixes #9).I still have to check how (if) I want to support older distribution releases. At the moment the PR depends on PHP composer which is only available in stretch/xenial and newer.