Review needed: Drupal Charm

[Sebastian Ferrari]

Production and development ready Drupal charm with project import from an existing GIT repository. Is almost completed, just need to do some more relations and a lot of documentation, but before that, I want to finish the peer's relation to load balance the units of the service, so then Drupal can scale horizontally.

Here's the code, is all in the master branch:
https://github.com/sebas5384/charm-drupal

Launchpad:
https://code.launchpad.net/~sebas5384/charms/precise/drupal/trunk

[Darryl Weaver (dweaver)]

Hi Sebastian,

Great work on the charm so far.
I have reviewed the charm and I have a few comments:

I deployed the bash version of the drupal charm and it failed with the nfs relation.
I deployed with the default drupal charm options (except the admin password) and deployed an nfs charm on precise.
I added the relation to nfs and got the error:
unit-drupal-0: 2014-05-15 14:00:05 INFO juju-log nfs:14: We've got a mount
unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed cat: .deploy-branch: No such file or directory
unit-drupal-0: 2014-05-15 14:00:06 INFO juju-log nfs:14: Git checkout to the master branch.
unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed error: pathspec 'master' did not match any file(s) known to git.
unit-drupal-0: 2014-05-15 14:00:06 ERROR juju.worker.uniter uniter.go:486 hook failed: exit status 1

My config is using the default options, so deploy-source is "drupal.org".

I then changed the tuning option to "bare" and related the database and the deployment works OK on amazon EC2.
I was able to log in and configure the drupal site from there.

It is bad practice to set a default password in the charm config options as this would result in deployments with a default password set.

The default should be to include no default password and make the user supply one and only run site install when a password has been set, or generate an admin password on site install and output it to a file on the unit so the administrator can collect the password from the unit after installation has run (see nagios charm for an example).

Rather than using a tuning-level configuration option it would be better if the charm could detect which relations it has and tune itself automatically based on the available relationships, i.e. go from bare to single when the nfs relation is joined and to optimised when both the caching relation is available and the nfs relation is available. This would remove the need for the option.

More details in the readme for deploying an existing site from a git repository would be a good addition too, perhaps an example would be helpful.

Otherwise, excellent work and I look forward to seeing the next iteration.

[Sebastian Ferrari (sebas5384)]

Darryl, thank you!!! Made me really happy when i sow your review, and
feedback :)

   1. Bad practice of default password:
   I can make a random password, and then set it in the config of the
   charm. Thats possible, right?

Abs,
Sebas.

2014-05-15 17:18 GMT-03:00 Darryl Weaver <email address hidden>:

> Hi Sebastian,
>
> Great work on the charm so far.
> I have reviewed the charm and I have a few comments:
>
> I deployed the bash version of the drupal charm and it failed with the nfs
> relation.
> I deployed with the default drupal charm options (except the admin
> password) and deployed an nfs charm on precise.
> I added the relation to nfs and got the error:
> unit-drupal-0: 2014-05-15 14:00:05 INFO juju-log nfs:14: We've got a mount
> unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed cat:
> .deploy-branch: No such file or directory
> unit-drupal-0: 2014-05-15 14:00:06 INFO juju-log nfs:14: Git checkout to
> the master branch.
> unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed error:
> pathspec 'master' did not match any file(s) known to git.
> unit-drupal-0: 2014-05-15 14:00:06 ERROR juju.worker.uniter uniter.go:486
> hook failed: exit status 1
>
> My config is using the default options, so deploy-source is
> "drupal.org".
>
> I then changed the tuning option to "bare" and related the database and
> the deployment works OK on amazon EC2.
> I was able to log in and configure the drupal site from there.
>
> It is bad practice to set a default password in the charm config options
> as this would result in deployments with a default password set.
>
> The default should be to include no default password and make the user
> supply one and only run site install when a password has been set, or
> generate an admin password on site install and output it to a file on
> the unit so the administrator can collect the password from the unit
> after installation has run (see nagios charm for an example).
>
> Rather than using a tuning-level configuration option it would be better
> if the charm could detect which relations it has and tune itself
> automatically based on the available relationships, i.e. go from bare to
> single when the nfs relation is joined and to optimised when both the
> caching relation is available and the nfs relation is available. This
> would remove the need for the option.
>
> More details in the readme for deploying an existing site from a git
> repository would be a good addition too, perhaps an example would be
> helpful.
>
> Otherwise, excellent work and I look forward to seeing the next
> iteration.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1315428
>
> Title:
> Review needed: Drupal Charm
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1315428/+subscriptions
>

[Sebastian Ferrari (sebas5384)]

[sorry, sent email to early ¬¬]
Darryl, thank you!!! Made me really happy when i sow your review, and
feedback :)

   1. Bad practice of default password:
   I can make a random password, and then set it in the config of the
   charm. Thats possible, right?

   2. NFS is not working:
   Yet!! the relation is there, but i never tested, so thank you! I'm going
   to test it to see the error.

   3. Tuning level detected by relations:
   Thats a brilliant ideia! I will implement that.

   4. Better documentation about importing an existing project:
   Yes! That's something I'm not pretty sure if it's working, so after
   testing, I will add more doc about that.

* What's the best way to do horizontally scaling, when you only have more
than 1 unit, and a mysql relation, so no nfs relation added to the service.
So how the units can share the same mounting point of files?

Again, thanks!

Cheers,
Sebas.

2014-05-16 0:26 GMT-03:00 Sebastian <email address hidden>:

> Darryl, thank you!!! Made me really happy when i sow your review, and
> feedback :)
>
>
> 1. Bad practice of default password:
> I can make a random password, and then set it in the config of the
> charm. Thats possible, right?
>
>
> Abs,
> Sebas.
>
>
>
> 2014-05-15 17:18 GMT-03:00 Darryl Weaver <email address hidden>:
>
> Hi Sebastian,
>>
>> Great work on the charm so far.
>> I have reviewed the charm and I have a few comments:
>>
>> I deployed the bash version of the drupal charm and it failed with the
>> nfs relation.
>> I deployed with the default drupal charm options (except the admin
>> password) and deployed an nfs charm on precise.
>> I added the relation to nfs and got the error:
>> unit-drupal-0: 2014-05-15 14:00:05 INFO juju-log nfs:14: We've got a mount
>> unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed cat:
>> .deploy-branch: No such file or directory
>> unit-drupal-0: 2014-05-15 14:00:06 INFO juju-log nfs:14: Git checkout to
>> the master branch.
>> unit-drupal-0: 2014-05-15 14:00:06 INFO nfs-relation-changed error:
>> pathspec 'master' did not match any file(s) known to git.
>> unit-drupal-0: 2014-05-15 14:00:06 ERROR juju.worker.uniter uniter.go:486
>> hook failed: exit status 1
>>
>> My config is using the default options, so deploy-source is
>> "drupal.org".
>>
>> I then changed the tuning option to "bare" and related the database and
>> the deployment works OK on amazon EC2.
>> I was able to log in and configure the drupal site from there.
>>
>> It is bad practice to set a default password in the charm config options
>> as this would result in deployments with a default password set.
>>
>> The default should be to include no default password and make the user
>> supply one and only run site install when a password has been set, or
>> generate an admin password on site install and output it to a file on
>> the unit so the administrator can collect the password from the unit
>> after installation has run (see nagios charm for an example).
>>
>> Rather than using a tuning-level configuration option it would be better
>> if the charm could detect which relations it has and tune itself
>> automatically based on the available relationships, i.e....

[Sebastian Ferrari (sebas5384)]

I made a port to Ansible, so any previous review are not directly related to this new version.

TODO'S:
=======================
- Make tests with Amulet.
- Drupal 8.x support.
- Some PHP configuration, like memory or max upload.
- PHP-FPM charm relation, making built-in PHP an option.
- Some Nginx configuration.
- Gluster FS relation, for scale/redundancy between units.
- Varnish relation.
- HA-Proxy relation.
- Apache Solr relation.
- Redis relation.

[Benjamin Saller (bcsaller)]

Thanks for the continued development of this!

Nice use of ansible roles from the ecosystem.

I'd suggest removing the `deploy` target from the makefile, it adds very little and is just another thing to think about when making this cross series.

However I was unable to deploy under either precise or trusty. Under trusty the package name of a dep must have changed, but under precise which is expected to work I got the following:

2014-08-19 03:37:12 INFO install TASK: [pbuyle.nginx-drupal | Create sites-enabled configuration directory] ****
2014-08-19 03:37:12 INFO install changed: [localhost]
2014-08-19 03:37:12 INFO install
2014-08-19 03:37:12 INFO install TASK: [pbuyle.nginx-drupal | Create available sites configuration files] ******
2014-08-19 03:37:12 INFO install fatal: [localhost] => {'msg': "AnsibleUndefinedVariable: One or more undefined variables: 'dict object' has no attribute 'limit_conn'", 'failed': True}
2014-08-19 03:37:12 INFO install fatal: [localhost] => {'msg': 'One or more items failed.', 'failed': True, 'changed': False, 'results': [{'msg': "AnsibleUndefinedVariable: One or more undefined variables: 'dict object' has no attribute 'limit_conn'", 'failed': True}]}
2014-08-19 03:37:12 INFO install
2014-08-19 03:37:12 INFO install FATAL: all hosts have already failed -- aborting
2014-08-19 03:37:12 INFO install
2014-08-19 03:37:12 INFO install PLAY RECAP ********************************************************************
2014-08-19 03:37:12 INFO install to retry, use: --limit @/home/ubuntu/site.yaml.retry
2014-08-19 03:37:12 INFO install
2014-08-19 03:37:12 INFO install localhost : ok=30 changed=22 unreachable=1 failed=0

Additionally I was unable to run the unit tests.

EEE
======================================================================
ERROR: Most of the hooks let ansible do all the work.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/precise/drupal/unit_tests/test_hooks.py", line 57, in test_default_hooks
    hooks.execute([hook])
AttributeError: 'module' object has no attribute 'execute'

======================================================================
ERROR: test_applies_install_playbook (test_hooks.InstallHookTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/precise/drupal/unit_tests/test_hooks.py", line 37, in test_applies_install_playbook
    hooks.execute(['install'])
AttributeError: 'module' object has no attribute 'execute'

======================================================================
ERROR: test_installs_ansible_support (test_hooks.InstallHookTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/precise/drupal/unit_tests/test_hooks.py", line 30, in test_installs_ansible_support
    hooks.execute(['install'])
AttributeError: 'module' object has no attribute 'execute'

----------------------------------------------------------------------
Ran 3 tests in 0.024s

FAILED (errors=3)

[Sebastian Ferrari (sebas5384)]

Hey Benjamin! thanks for the review!!

I'm really busy this days, but I promise to take a look at the problems you
describe.

At first the first error seems wired because we are using this charm
already, but I will deploy another to see what happens.

The tests are really fake, and we where without time to develop tests for
the scenarios the charm already covers.

I guess till next week I will reply with some new status :)

Again, thanks!

Abs,
Sebas.

2014-08-19 1:01 GMT-03:00 Benjamin Saller <email address hidden>:

> Thanks for the continued development of this!
>
> Nice use of ansible roles from the ecosystem.
>
> I'd suggest removing the `deploy` target from the makefile, it adds very
> little and is just another thing to think about when making this cross
> series.
>
> However I was unable to deploy under either precise or trusty. Under
> trusty the package name of a dep must have changed, but under precise
> which is expected to work I got the following:
>
> 2014-08-19 03:37:12 INFO install TASK: [pbuyle.nginx-drupal | Create
> sites-enabled configuration directory] ****
> 2014-08-19 03:37:12 INFO install changed: [localhost]
> 2014-08-19 03:37:12 INFO install
> 2014-08-19 03:37:12 INFO install TASK: [pbuyle.nginx-drupal | Create
> available sites configuration files] ******
> 2014-08-19 03:37:12 INFO install fatal: [localhost] => {'msg':
> "AnsibleUndefinedVariable: One or more undefined variables: 'dict object'
> has no attribute 'limit_conn'", 'failed': True}
> 2014-08-19 03:37:12 INFO install fatal: [localhost] => {'msg': 'One or
> more items failed.', 'failed': True, 'changed': False, 'results': [{'msg':
> "AnsibleUndefinedVariable: One or more undefined variables: 'dict object'
> has no attribute 'limit_conn'", 'failed': True}]}
> 2014-08-19 03:37:12 INFO install
> 2014-08-19 03:37:12 INFO install FATAL: all hosts have already failed --
> aborting
> 2014-08-19 03:37:12 INFO install
> 2014-08-19 03:37:12 INFO install PLAY RECAP
> ********************************************************************
> 2014-08-19 03:37:12 INFO install to retry, use: --limit
> @/home/ubuntu/site.yaml.retry
> 2014-08-19 03:37:12 INFO install
> 2014-08-19 03:37:12 INFO install localhost : ok=30
> changed=22 unreachable=1 failed=0
>
>
> Additionally I was unable to run the unit tests.
>
> EEE
> ======================================================================
> ERROR: Most of the hooks let ansible do all the work.
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/tmp/precise/drupal/unit_tests/test_hooks.py", line 57, in
> test_default_hooks
> hooks.execute([hook])
> AttributeError: 'module' object has no attribute 'execute'
>
> ======================================================================
> ERROR: test_applies_install_playbook (test_hooks.InstallHookTestCase)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/tmp/precise/drupal/unit_tests/test_hooks.py", line 37, in
> test_applies_install_playbook
> hooks.execute(['install'])
> AttributeError: 'module' ...

[Sebastian Ferrari (sebas5384)]

BTW, could we get some help to get this charm gracefully done with tests and all?

I could arrange a day to focus in this, and if someone from the charm reviewers could join me, would be pretty NEAT !!!

At Taller (taller.net.br) we participate actively in the Drupal community, and we know that finishing and getting this charm promulgated will boost Juju in the Drupal community, which is huge!! :)

So, what do you think Benjamin and Juju community? let's get this charm done?

[Jorge Castro (jorge)]

Ok we've moved the test charm school discussion to mail, please update the status of this bug when you're ready for another round of review, thanks!