search

afroyd / Zend-Server-juju

MIT License
2 stars 2 forks source link

Charm Review for zend-server #6

Closed mbruzek closed 10 years ago

mbruzek commented 10 years ago

Hello Amir,

First off thank you for all the work you put in on this charm! I can tell by the charm code that you are a great developer and know the Zend product very well! I did a full review of the code and it is rather long. Please do not take my comments as a negative toward the code you wrote, there are some differences when writing charms for Juju that are different that you may not have known when writing this charm.

Review

charm proof

We have a tool to do some static lint tests for charms called charm proof. It helps us identify some things that are missing or problematic with the charm.

$ charm proof W: no copyright file

It looks like you are missing a copyright file. This is not the copyright for Zend-Server software itself, it is for the charm code. It must be a free license ( http://opensource.org/osd ).

metadata.yaml

This file needs a summary and a better description (two or three sentences on what Zend Server). Think of this as your marketing material to people who might not know what Zend Server is. The information in this file is displayed in the GUI when the customer clicks on the zend-server charm.

config.yaml

This file looks good, all configuration options have a default value and a description. I would suggest going only installing one test application or none by default.

I wanted to emphasize that the juju user could change any one of these configuration values at anytime. Only the config-changed hook is run when “juju set” or configuration changes on the GUI. This means that the config-changed hook must read all the configuration values (directly or indirectly) and react to them appropriately, or it will break the user experience. For example if the user deploys zend-server with the default values and then changes the version, the user would expect to see a different version after that action. I noticed some of your configuration is only read in the install hook which makes the configuration immutable.

hooks

The documentation for hooks can be found here: https://juju.ubuntu.com/docs/authors-charm-hooks.html

install

This hook reads in most of the configuration and install is not called again in a normal charm lifecycle. The configration read here is immutable and should move into the config-changed hook. I understand wanting to read the version in the install hook, but the user would not know what values can change after install is finished running. In practicse the install hook is very small only installing the prerequisites (like mysql-client) for the main service, and other things that will not change. Where the config-changed hook does the installing.

Remember that all hooks must be idempotent ( http://en.wikipedia.org/wiki/Idempotence ) meaning they can run multiple times with the same outcome as running once. There are ways (juju debug-hooks) to run even the install hook multiple times for testing if you need. So watch things like creating keys if the key already exists, or deleting directories before putting new ones down.

config-changed

This hook is pretty empty and should contain most of the code. This is the only hook called when configuration changes so it is very important to read in all the configuration values and act on them appropriately.

upgrade-charm

It does not look like the upgrade-charm hook does anything substantial, so this is a candidate to remove the hook. This hook runs on an upgrade operation, like if the charm code has changed. What I would see here is calling install or config-changed from this charm and doing anything else to upgrade, like database reset.

database-relation-changed

Zend Server is very dependent on a database connection. We have other examples of charms that have to wait until a database connection is created before they will install too. Other charms write out a dot file (such as .database) with the connection details, then other hooks could check for this file before doing database related operations. There is also some immutable configuration in this hook, some configuration options are only read at database relation creation time. I noticed this in the demo, we could not change which demo software was installed because those configuration values are only read when the DB relation was created, which happens only once in a normal lifecycle.

I know the charm works, but when handling communication between charms always use the private-address (unit-get private-address). When using the local (LXC) provider the private-address and public-address are the same, but on OpenStack and AWS clouds the addresses are different. This is a common mistake we see in some charms developed on local provider. The public-address being used in several places most of which look appropriate but the “zs-manage vhost-add” line looks like a place where you may want to consider using a private address.

README.md

The readme converts from markdown to HTML correctly! Some sentences need to be capitalized but other than that it looks good. You should and add a few more sections at the end with links/information about the upstream project, contact information, and where to go if they have problems with the Zend Server software (bug tracker).

I also did not see what the difference is between the commercial Zend product and this version. The readme is your opportunity to write as much or as little as you want about the products available.

tests

We require tests for trusty charms. The charm should contain a tests directory with executable tests (commonly amulet tests) or a Makefile with a “test” target. We have automated tools that run on trusty charms to ensure our charms are high quality. I sent a separate email about tests that contained an example. If you need others please let me know.

deploy

I deployed the zend-server on my local system (amd64 on LXC) according to the readme.

juju deploy --repository ../../ local:trusty/zend-server
juju deploy cs:trusty/mysql
juju add-relation zend-server mysql
juju deploy cs:trusty/haproxy
juju add-relation zend-server:website haproxy:reverseproxy

The other charms deployed successfully but the zend-server had an error in the database-relation changed hook. Here is the last part of the unit-zend-server-0.log file:

2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 + eval /usr/local/zend/bin/zs-manage app-deploy --pkgFile files/demo.zpk --baseUrl http://10.0.3.238/demo --appName demoApp --defaultServer TRUE -N adminKey -K f13b874b8eac6aebb8f0244716f97dc02adf12bf10ced201771e755c4691923e 2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 ++ /usr/local/zend/bin/zs-manage app-deploy --pkgFile files/demo.zpk --baseUrl http://10.0.3.238/demo --appName demoApp --defaultServer TRUE -N adminKey -K f13b874b8eac6aebb8f0244716f97dc02adf12bf10ced201771e755c4691923e 2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 Error: failed accessing Zend Server through the Web API 2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 WebAPI error message : 2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 (Library) zend framework 2: Zend Framework 2 must be deployed 2014-10-23 23:10:16 INFO unit.zend-server/0.database-relation-changed context.go:473 WebAPI error code : unmetDependency 2014-10-23 23:10:16 ERROR juju.worker.uniter uniter.go:566 hook failed: exit status 1

I have seen the zend-server charm work in the demo, so not sure how to fix this error, please let me know if you need the logs on this problem.

GitHub process

There is a special process to get github charms in the charm store. After you pass the review criteria we can work on that process together to get this charm in the Juju Charm Store.

Conclusion

Thank you so much for your work on the zend-server charm! Let me know when have addressed the issues above either with code changes or comments, and wish to have another review.

The charm is really looking good. We appreciate your submission so far and look forward to the next round of reviews!

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list juju@lists.ubuntu.com

AdamIsrael commented 10 years ago

Hi Amir,

I just took a look at your recent changes. I'd like to echo Matt's comments about the quality of work that's gone into the charm so far. I'm very much looking forward to seeing this added to the Juju Charm Store.

That said, I did run into some issues while testing.

Manual deployment

I was able to install zend-server with no relations, and it worked very well. The first time, I didn't set the gui password but the zend UI allowed me to set the admin and developer password. Everything I tried here seemed functional.

MySQL

The problems began when I attempted to relate mysql. The 'database-relation-changed' hook failed shortly after running juju add-relation zend-server mysql. I attempted to retry several times with no success.

2014-11-07 16:43:51 INFO config-changed + . /root/.ZendServerStatus 2014-11-07 16:43:51 INFO config-changed ++ ZS_installed=false 2014-11-07 16:43:51 INFO config-changed ++ ZS_installed=true 2014-11-07 16:43:51 INFO config-changed + phpVersion=5.5 2014-11-07 16:43:51 INFO config-changed + adminKey=adminKey 2014-11-07 16:43:51 INFO config-changed ++ config-get adminKeyHash 2014-11-07 16:43:51 INFO config-changed + adminKeyHash=OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:43:51 INFO config-changed ++ config-get guiPassword 2014-11-07 16:43:51 INFO config-changed + guiPassword=password 2014-11-07 16:43:51 INFO config-changed ++ config-get zrayEnable 2014-11-07 16:43:51 INFO config-changed + zrayEnable=False 2014-11-07 16:43:51 INFO config-changed + production=true 2014-11-07 16:43:51 INFO config-changed ++ cut -f1 -d/ 2014-11-07 16:43:51 INFO config-changed ++ awk '{print $2}' 2014-11-07 16:43:51 INFO config-changed ++ tail -n1 2014-11-07 16:43:51 INFO config-changed ++ grep 'state UP' -A2 2014-11-07 16:43:51 INFO config-changed ++ ip addr 2014-11-07 16:43:51 INFO config-changed + private_address=10.0.3.149 2014-11-07 16:43:51 INFO config-changed + '[' true = false ']' 2014-11-07 16:43:51 INFO config-changed + '[' -z OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk ']' 2014-11-07 16:43:51 INFO config-changed + '[' -z '' ']' 2014-11-07 16:43:51 INFO config-changed + /usr/local/zend/bin/zs-manage api-keys-add-key -n adminKey -s OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:43:52 INFO config-changed KEY_NAME = adminKey 2014-11-07 16:43:52 INFO config-changed KEY_USER_NAME = admin 2014-11-07 16:43:52 INFO config-changed KEY_HASH = OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:43:52 INFO config-changed + echo ZS_api_key_hash=OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:43:52 INFO config-changed + echo ZS_api_key=adminKey 2014-11-07 16:43:52 INFO config-changed + '[' -z '' ']' 2014-11-07 16:43:52 INFO config-changed + '[' '!' -z password ']' 2014-11-07 16:43:52 INFO config-changed + /usr/local/zend/bin/zs-client.sh bootstrapSingleServer --adminPassword=password --production=true --acceptEula --wait 2014-11-07 16:43:52 INFO config-changed 2014-11-07 16:43:52 INFO config-changed Notice: Undefined index: HOMEDRIVE in phar:///usr/local/zend/share/sdk/zs-client.phar/module/Client/config/module.config.php on line 2 2014-11-07 16:43:52 INFO config-changed 2014-11-07 16:43:52 INFO config-changed Notice: Undefined index: HOMEPATH in phar:///usr/local/zend/share/sdk/zs-client.phar/module/Client/config/module.config.php on line 2 2014-11-07 16:44:01 INFO config-changed admin 2014-11-07 16:44:01 INFO config-changed 301b755ce9691db32929b4e0806ecdddc724c8ad67d14b573d2e539a4e00b481 2014-11-07 16:44:01 INFO config-changed + echo ZS_bootstrapped=true 2014-11-07 16:44:01 INFO config-changed + echo ZS_guiPassword=password 2014-11-07 16:44:01 INFO config-changed + echo ZS_zrayEnabled=False 2014-11-07 16:44:01 INFO config-changed + open-port 80 2014-11-07 16:44:01 INFO config-changed + open-port 443 2014-11-07 16:44:01 INFO config-changed + open-port 10082 2014-11-07 16:44:02 INFO config-changed + open-port 10081 2014-11-07 16:44:02 INFO config-changed + restart=true 2014-11-07 16:44:02 INFO config-changed + '[' '' = true ']' 2014-11-07 16:44:02 INFO config-changed + '[' '' = true ']' 2014-11-07 16:44:02 INFO config-changed + '[' true = true ']' 2014-11-07 16:44:02 INFO config-changed + /usr/local/zend/bin/zs-manage restart-php -N adminKey -K OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:44:02 INFO config-changed vagrant-local-machine-2 pendingRestart 2014-11-07 16:44:07 INFO database-relation-changed ++ relation-get user 2014-11-07 16:44:07 INFO database-relation-changed + db_user= 2014-11-07 16:44:07 INFO database-relation-changed ++ relation-get password 2014-11-07 16:44:07 INFO database-relation-changed + db_pass= 2014-11-07 16:44:07 INFO database-relation-changed ++ relation-get private-address 2014-11-07 16:44:07 INFO database-relation-changed + db_host=10.0.3.222 2014-11-07 16:44:07 INFO database-relation-changed + zs_db_name=ZendServer 2014-11-07 16:44:07 INFO database-relation-changed + '[' -z 10.0.3.222 ']' 2014-11-07 16:44:07 INFO database-relation-changed + '[' -z ']' 2014-11-07 16:44:07 INFO database-relation-changed + '[' -z ']' 2014-11-07 16:44:07 INFO database-relation-changed + echo ZS_db_pass= 2014-11-07 16:44:07 INFO database-relation-changed + echo ZS_db_user= 2014-11-07 16:44:07 INFO database-relation-changed + echo ZS_db_host=10.0.3.222 2014-11-07 16:44:07 INFO database-relation-changed + echo ZS_db_name=ZendServer 2014-11-07 16:44:07 INFO database-relation-changed ++ cut -f1 -d/ 2014-11-07 16:44:07 INFO database-relation-changed ++ awk '{print $2}' 2014-11-07 16:44:07 INFO database-relation-changed ++ tail -n1 2014-11-07 16:44:07 INFO database-relation-changed ++ grep 'state UP' -A2 2014-11-07 16:44:07 INFO database-relation-changed ++ ip addr 2014-11-07 16:44:07 INFO database-relation-changed + private_address=10.0.3.149 2014-11-07 16:44:07 INFO database-relation-changed + . /root/.ZendServerStatus 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_installed=false 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_installed=true 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_api_key_hash=OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_api_key=adminKey 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_bootstrapped=true 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_guiPassword=password 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_zrayEnabled=False 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_db_pass= 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_db_user= 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_db_host=10.0.3.222 2014-11-07 16:44:07 INFO database-relation-changed ++ ZS_db_name=ZendServer 2014-11-07 16:44:07 INFO database-relation-changed + '[' '' '!=' true ']' 2014-11-07 16:44:07 INFO database-relation-changed + juju-log 'Connecting Zend Server to cluster by adding it to MySQL' 2014-11-07 16:44:07 INFO juju-log database:2: Connecting Zend Server to cluster by adding it to MySQL 2014-11-07 16:44:07 INFO database-relation-changed + /usr/local/zend/bin/zs-manage server-add-to-cluster -n zend-server/0- -i 10.0.3.149 -o 10.0.3.222 -u -p -d ZendServer -r 60 -w 5 -s -N adminKey -K OGFmNjE0MWExNzBkNDMxN2YwNThjYmUyMDNkMjIzMTEwMGQyOTY4Y2FlZDQzMzdk 2014-11-07 16:44:07 INFO database-relation-changed Error: DB Password not specified 2014-11-07 16:44:07 ERROR juju.worker.uniter uniter.go:486 hook failed: exit status 3

Unit Testing

Next, I reset the environment and attempted to run the unit tests using bundletester, which is the tool we use during our automated testing.

vagrant@vagrant-ubuntu-trusty-64:/charms/trusty/zend-server$ time bundletester -F -l DEBUG -v DEBUG:bundletester.utils:Updating JUJU_ENV: "" -> "local" DEBUG:root:Bootstrap environment: local DEBUG:deployer.env:Connecting to environment... DEBUG:deployer.env:Connected to environment DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/usr/bin/charm-proof'] DEBUG:runner:

DEBUG:runner:Exit Code: 0 zend-server charm-proof PASS DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/charms/trusty/zend-server/tests/00-setup'] DEBUG:runner:

DEBUG:runner:Exit Code: 0 00-setup PASS DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/charms/trusty/zend-server/tests/10-deploy'] DEBUG:runner: 2014-11-07 16:26:11 Starting deployment of local 2014-11-07 16:26:19 Deploying services... 2014-11-07 16:26:19 Deploying service mysql using cs:precise/mysql-50 2014-11-07 16:26:28 Deploying service zend-server using local:precise/zend-server 2014-11-07 16:26:29 Error deploying service 'zend-server' 2014-11-07 16:26:29 Command (juju deploy -e local --config /tmp/tmpl8WZg9 --repository=/charms local:precise/zend-server zend-server) Output:

Added charm "local:precise/zend-server-1" to the environment. ERROR option "adminKeyHash" expected string, got 1.2345678901234569e+62

2014-11-07 16:26:29 Deployment stopped. run time: 18.46 Traceback (most recent call last): File "/charms/trusty/zend-server/tests/10-deploy", line 17, in d.setup(timeout=1100) File "/usr/lib/python3/dist-packages/amulet/deployer.py", line 241, in setup ], cwd=self.deployer_dir) File "/usr/lib/python3.4/subprocess.py", line 557, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['juju-deployer', '-W', '-L', '-c', '/tmp/amulet-juju-deployer-sk_3yo_c.json', '-e', 'local', '-t', '1200', 'local']' returned non-zero exit status 1

DEBUG:runner:Exit Code: 1 10-deploy ERROR DEBUG:bundletester.utils:Updating JUJU_ENV: "local" -> ""

ERROR: zend-server::10-deploy [/charms/trusty/zend-server/tests/10-deploy exit 1] 2014-11-07 16:26:11 Starting deployment of local 2014-11-07 16:26:19 Deploying services... 2014-11-07 16:26:19 Deploying service mysql using cs:precise/mysql-50 2014-11-07 16:26:28 Deploying service zend-server using local:precise/zend-server 2014-11-07 16:26:29 Error deploying service 'zend-server' 2014-11-07 16:26:29 Command (juju deploy -e local --config /tmp/tmpl8WZg9 --repository=/charms local:precise/zend-server zend-server) Output:

Added charm "local:precise/zend-server-1" to the environment. ERROR option "adminKeyHash" expected string, got 1.2345678901234569e+62

2014-11-07 16:26:29 Deployment stopped. run time: 18.46 Traceback (most recent call last): File "/charms/trusty/zend-server/tests/10-deploy", line 17, in d.setup(timeout=1100) File "/usr/lib/python3/dist-packages/amulet/deployer.py", line 241, in setup ], cwd=self.deployer_dir) File "/usr/lib/python3.4/subprocess.py", line 557, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['juju-deployer', '-W', '-L', '-c', '/tmp/amulet-juju-deployer-sk_3yo_c.json', '-e', 'local', '-t', '1200', 'local']' returned non-zero exit status 1

PASS: 2 ERROR: 1 Total: 3 (32.346263 sec)

Conclusion

Thank you again for all of your work on this charm. I am happy to see the changes made based on Matt's review. Once the issues I encountered are resolved, please let Matt or myself know and we can re-review.

Thanks! Adam Israel

afroyd commented 10 years ago

Thank you Adam for the review. Hopefully fixed the MySQL relation problem. Just a silly mistake of using '|' for or operator instead of '||'

I'm still not sure how to run the tests when the charm is on local file system. I get: time bundletester -F -l DEBUG -v DEBUG:bundletester.utils:Updating JUJU_ENV: "" -> "amazon" DEBUG:bundletester.utils:Updating JUJU_ENV: "amazon" -> "" Traceback (most recent call last): File "/usr/local/bin/bundletester", line 9, in load_entry_point('bundletester==0.4.3', 'console_scripts', 'bundletester')() File "/usr/local/lib/python2.7/dist-packages/bundletester/tester.py", line 78, in main [report.emit(result) for result in run()] File "/usr/local/lib/python2.7/dist-packages/bundletester/runner.py", line 125, in call self.build() File "/usr/local/lib/python2.7/dist-packages/bundletester/runner.py", line 110, in build self.builder.build_virtualenv(vpath) File "/usr/local/lib/python2.7/dist-packages/bundletester/builder.py", line 115, in build_virtualenv stdout=open('/dev/null', 'w')) File "/usr/lib/python2.7/subprocess.py", line 535, in check_call retcode = call(_popenargs, _kwargs) File "/usr/lib/python2.7/subprocess.py", line 522, in call return Popen(_popenargs, _kwargs).wait() File "/usr/lib/python2.7/subprocess.py", line 710, in init errread, errwrite) File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child raise child_exception OSError: [Errno 2] No such file or directory

AdamIsrael commented 10 years ago

Hi Amir,

For that, you'll need to install the python-virtualenv package.

AdamIsrael commented 10 years ago

Hi Amir,

I just noticed your latest changes (I only get notifications if a new comment is posted on the issue) and re-tested the zend server charm.

There is one minor change that I'd like you to make, to tests/10-deploy. Before d.setup is run (I put it on line 15, before d.relate), add this line:

d.configure('mysql', {'dataset-size':'512M'})

There is a known issue [1] with the MySQL charm's memory allocation when running under some providers (local and digital ocean). Setting the dataset-size key resolves that issue, and with that change in place I am able to successfully run the unit tests.

Thanks for all of your work on this. If you can make that one change, I'll refer this back to Matt or another Charmer for addition to the charm store.

[1] https://manage.jujucharms.com/charms/trusty/mysql, under Caveats

afroyd commented 10 years ago

Thanks Adam. I updated the test installed the package and now it takes longer for the test to fail :) Maybe this time it is not finding the local charm?

time bundletester -F -l DEBUG -v DEBUG:bundletester.utils:Updating JUJU_ENV: "" -> "amazon" DEBUG:root:Bootstrap environment: amazon DEBUG:deployer.env:Connecting to environment... DEBUG:deployer.env:Connected to environment DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/usr/bin/charm-proof'] DEBUG:runner:

DEBUG:runner:Exit Code: 0 zend-server charm-proof PASS DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/home/amir/charms/trusty/zend-server/tests/00-setup'] DEBUG:runner:

DEBUG:runner:Exit Code: 0 00-setup PASS DEBUG:deployer.env: Terminating machines DEBUG:root:Waiting for services to be removed... DEBUG:runner:call ['/home/amir/charms/trusty/zend-server/tests/10-deploy'] DEBUG:runner: 2014-11-11 21:06:05 Starting deployment of amazon Traceback (most recent call last): File "/usr/bin/juju-deployer", line 9, in load_entry_point('juju-deployer==0.4.1', 'console_scripts', 'juju-deployer')() File "/usr/lib/python2.7/dist-packages/deployer/cli.py", line 127, in main run() File "/usr/lib/python2.7/dist-packages/deployer/cli.py", line 225, in run importer.Importer(env, deployment, options).run() File "/usr/lib/python2.7/dist-packages/deployer/action/importer.py", line 186, in run self.get_charms() File "/usr/lib/python2.7/dist-packages/deployer/action/importer.py", line 63, in get_charms no_local_mods=self.options.no_local_mods) File "/usr/lib/python2.7/dist-packages/deployer/deployment.py", line 140, in fetch_charms charm.fetch() File "/usr/lib/python2.7/dist-packages/deployer/charm.py", line 128, in fetch self.vcs.branch() File "/usr/lib/python2.7/dist-packages/deployer/vcs.py", line 103, in branch self._call(params, self.err_branch, os.path.dirname(self.path)) File "/usr/lib/python2.7/dist-packages/deployer/vcs.py", line 30, in _call args, cwd=cwd or self.path, stderr=stderr) File "/usr/lib/python2.7/subprocess.py", line 566, in check_output process = Popen(stdout=PIPE, _popenargs, *_kwargs) File "/usr/lib/python2.7/subprocess.py", line 710, in init errread, errwrite) File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child raise child_exception TypeError: execv() arg 2 must contain only strings Traceback (most recent call last): File "/home/amir/charms/trusty/zend-server/tests/10-deploy", line 18, in d.setup(timeout=1100) File "/usr/lib/python3/dist-packages/amulet/deployer.py", line 241, in setup ], cwd=self.deployer_dir) File "/usr/lib/python3.4/subprocess.py", line 557, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['juju-deployer', '-W', '-L', '-c', '/tmp/amulet-juju-deployer-0x3ayz3i.json', '-e', 'amazon', '-t', '1200', 'amazon']' returned non-zero exit status 1

DEBUG:runner:Exit Code: 1 10-deploy ERROR DEBUG:bundletester.utils:Updating JUJU_ENV: "amazon" -> ""

ERROR: zend-server::10-deploy [/home/amir/charms/trusty/zend-server/tests/10-deploy exit 1] 2014-11-11 21:06:05 Starting deployment of amazon Traceback (most recent call last): File "/usr/bin/juju-deployer", line 9, in load_entry_point('juju-deployer==0.4.1', 'console_scripts', 'juju-deployer')() File "/usr/lib/python2.7/dist-packages/deployer/cli.py", line 127, in main run() File "/usr/lib/python2.7/dist-packages/deployer/cli.py", line 225, in run importer.Importer(env, deployment, options).run() File "/usr/lib/python2.7/dist-packages/deployer/action/importer.py", line 186, in run self.get_charms() File "/usr/lib/python2.7/dist-packages/deployer/action/importer.py", line 63, in get_charms no_local_mods=self.options.no_local_mods) File "/usr/lib/python2.7/dist-packages/deployer/deployment.py", line 140, in fetch_charms charm.fetch() File "/usr/lib/python2.7/dist-packages/deployer/charm.py", line 128, in fetch self.vcs.branch() File "/usr/lib/python2.7/dist-packages/deployer/vcs.py", line 103, in branch self._call(params, self.err_branch, os.path.dirname(self.path)) File "/usr/lib/python2.7/dist-packages/deployer/vcs.py", line 30, in _call args, cwd=cwd or self.path, stderr=stderr) File "/usr/lib/python2.7/subprocess.py", line 566, in check_output process = Popen(stdout=PIPE, _popenargs, *_kwargs) File "/usr/lib/python2.7/subprocess.py", line 710, in init errread, errwrite) File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child raise child_exception TypeError: execv() arg 2 must contain only strings Traceback (most recent call last): File "/home/amir/charms/trusty/zend-server/tests/10-deploy", line 18, in d.setup(timeout=1100) File "/usr/lib/python3/dist-packages/amulet/deployer.py", line 241, in setup ], cwd=self.deployer_dir) File "/usr/lib/python3.4/subprocess.py", line 557, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['juju-deployer', '-W', '-L', '-c', '/tmp/amulet-juju-deployer-0x3ayz3i.json', '-e', 'amazon', '-t', '1200', 'amazon']' returned non-zero exit status 1

PASS: 2 ERROR: 1 Total: 3 (5.427945 sec)

AdamIsrael commented 10 years ago

Hi Amir,

I'm not familiar with that error. You might check to verify that your installed version of juju-deployer is current. I re-ran bundletester and it's still passing for me. Matt Bruzek will be taking another look shortly, though.

Thanks again for all of your work on this!

mbruzek commented 10 years ago

Hello Amir,

Thanks for the update to the zend-server charm code. This version with less configuration options looks easier for users to deploy. I really like how you are using a dot file to hold the information your scripts need.

Tests

Thank you for writing tests! I ran the tests on my local machine and on HP cloud (an OpenStack cloud). All the tests passed on both environments!

I tested manually as well. Everything came up as expected, I was very pleased with how well zend-server worked. I did not see the error that you listed today.

Review

This version of the charm code was written very well, great job on incorporating the feedback into the hooks.

charmhelpers

The charmhelpers package is supposed to make it easier to write charm code in python. Since your charm is written in bash and does not use charmhelpers, I believe you can remove the charmhelper parts which is all the files in: lib, scripts, and charm-helpers.yaml.

Since we are removing files, you can safely remove the revision file. That file is no longer used by Juju.

copyright

The copyright must be an OSI approved license ( http://opensource.org/licenses ). The copyright in the charm directory only covers the code written to install/configure the charm. The Zend Server code still remains under the license that is bundled with the software. I would recommend the Apache or MIT licenses as they still are permissive but retain authorship rights.

metadata.yaml

The summary still needs to be filled in with a short summary of what Zend Server is. This summary will be displayed in the Juju charms GUI.

That is all I found this round. If you could address the concerns I found here the charm would be ready for inclusion as a recommended charm in the Juju charm store! Thanks again for your patience in the review process.

afroyd commented 10 years ago

Thanks Matt.

Is it possible for you to run the test also on power 8? I will be happy to know it is running there well too. I only have local and Amazon available.

mbruzek commented 10 years ago

Hello Amir,

Thanks for the latest updates to the charm code. The BSD 2 license would have been OK, I just didn't recognize it as that so please forgive my mistake.

I had some problems testing this code on Power 8 hardware and it was not immediately clear why. The Amulet tests default to Ubuntu 'precise' series and Power is only supported on 'trusty' series. After I changed the series in the test code, the tests passed! I created a pull request with the necessary changes here: https://github.com/afroyd/Zend-Server-juju/pull/7

Since all the tests passed and this charm conforms to our policy, zend-server is now a recommended charm and I pushed your code to ~charmers in launchpad. The charm can be deployed by typing juju deploy cs:trusty/zend-server. You can view the charm at: http://manage.jujucharms.com/charms/trusty/zend-server

Since you used github repository for the zend-server charm and the Juju Charm Store (currently) only supports launchpad you will have to take extra steps to update the charm code. Open a merge proposal against the lp:~charmers/charms/trusty/zend-server/trunk branch if you want to update the charm code.

Thanks very much for your submission. This is a great charm and we are very happy to finally have it in the Charm Store!