Juju Charms Collection

Bug #1459213
Comment #4

Comment 4 for bug 1459213

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-11-25: Re: IBM Platform Symphony Charm Creation

# README.md
This file still contains markdown errors. To resolve this please view the README.md file side by side next to the HTML that is rendered by the Juju charm store: https://jujucharms.com/u/ibmcharmers/ibm-platform-symphony/trusty

The readme does not have enough details to set up the web server to host the platform symphony software. You need to be specific about where to put the new directory (under the web server root), and verify that the URL works before deploying.

I was not able to verify the download from Fix Central based on my IBM account. The readme should tell the Juju users that they have to pay for the platform symphony software before they can download it. If the users already have an IBM account (as I do) and can not download the software, the readme should also tell them what to do if they get an error like I did.

> No applicable IBM support agreement found for one or more of the products you selected.

The readme is not very specific on how to download the key. I could not find it any key on the Passport Advantage Site. I was logged in and could not find anything labeled “key”. This is information the users would need if they were to deploy this software.

The readme should not advise the users to edit the config.yaml file prior to deployment. If someone is attempting to deploy this charm from the Juju GUI. If this can be set by configuration the readme should be written to call `juju set` on those configuration options.

Please remove the verify_cluster configuration option. If verification is a one time operation with results shown to the user this would be better suited to a Juju Action:
https://jujucharms.com/docs/devel/actions
https://jujucharms.com/docs/devel/authors-charm-actions
or a script that users could run using the `juju run` command:
juju run --unit platform-symphony-master/0 'pwd'

# config-changed
Best practices for a bash charms is to use “set -e” that will stop a bash script at the first error, which you have added since the last review. However there are large sections of code using “set +e” to allow many commands return an error code and continue processing. This defeats the purpose of adding “set -e” and is not up to best practices. My last review suggested using “|| true” for only the few commands that would not return a successful error code. Example: `ls nonexistantfile || true` Re-write this charm to be idempotent and use best practices for bash charms. This includes using spaces or tabs not both (still mixed in config-changed).

I was surprised to find that I could install the software without providing the sha1 sum. It looks like the sha1sum verification is skipped if the “sha” config parameter is empty. This does not comply with the policy and best practices and need to be fixed.

After downloading the software I got the error “service is neither a master nor a node” because I named the service something other than “platform-symphony-master”. The config-changed hook should not depend on the service name for the role of the software. If the charm were deployed from the GUI the default service name is “ibm-platform-symphony” and it is not easy for a GUI to change this. The name of the service can be anything and should not be used to determine the role of the software.

Juju has the concept of “leadership” in charms where one charm is elected as “leader” and other charms of the similar type are “followers”. This code that determines the master and node should be re-written to use leadership concepts: https://jujucharms.com/docs/master/authors-charm-leadership

# node-relation-changed
Over 90% of this file is no protected by the “set -e” option. Explore the options of the commands in this file and only protect the calls that are required with “|| true” or similar workarounds.

# tests/10-deploy.py

My last comment suggested running flak8 against the test files, this test has many unused variables and that makes me wonder if the tests are valid. While you are working on the next iteration of this charm please rewrite the sentry section to not hard-code the unit number in the name. This is a new best practice that I just learned and want to share it with you. The existing test does it this way:
    unit_manager_0 = d.sentry.unit['platform-symphony-master/0']
    unit_client_0 = d.sentry.unit['platform-symphony-node/0']
The best practice is to get the list of sentries and the first element (0):
    unit_manager_0 = d.sentry['platform-symphony-master'][0]
    unit_client_0 = d.sentry['platform-symphony-node'][0]

Do it this way because the unit number could not be zero and the original test code would fail. The new code will get the first unit no matter what the number is.

# Summary

All the files in the charm root have exectuable permissions, even the icon, and README.md file. This is not recommended and unsafe. One file that should be executable is `tests/10-deploy.py`.

There are some big changes needed in this submission before it can be recommended. I am going to put this bug status back to “In Progress”. When you have resolved all the review comments, please put the bug back in “Fix Committed” status. If you have any questions please email the list <email address hidden> or contact us in #juju on chat.freenode.net on IRC.

# README.md
This file still contains markdown errors.  To resolve this please view the README.md file side by side next to the HTML that is rendered by the Juju charm store:  https://jujucharms.com/u/ibmcharmers/ibm-platform-symphony/trusty

The readme does not have enough details to set up the web server to host the platform symphony software.  You need to be specific about where to put the new directory (under the web server root), and verify that the URL works before deploying.

I was not able to verify the download from Fix Central based on my IBM account.   The readme should tell the Juju users that they have to pay for the platform symphony software before they can download it.  If the users already have an IBM account (as I do) and can not download the software, the readme should also tell them what to do if they get an error like I did.

> No applicable IBM support agreement found for one or more of the products you selected.

The readme is not very specific on how to download the key.  I could not find it any key on the Passport Advantage Site.  I was logged in and could not find anything labeled “key”.  This is information the users would need if they were to deploy this software.

The readme should not advise the users to edit the config.yaml file prior to deployment.  If someone is attempting to deploy this charm from the Juju GUI.  If this can be set by configuration the readme should be written to call `juju set` on those configuration options.

# config-changed
Best practices for a bash charms is to use “set -e” that will stop a bash script at the first error, which you have added since the last review.  However there are large sections of code using “set +e” to allow many commands return an error code and continue processing.  This defeats the purpose of adding “set -e” and is not up to best practices.  My last review suggested using “|| true” for only the few commands that would not return a successful error code.  Example:  `ls nonexistantfile || true`  Re-write this charm to be idempotent and use best practices for bash charms.  This includes using spaces or tabs not both (still mixed in config-changed).

I was surprised to find that I could install the software without providing the sha1 sum.  It looks like the sha1sum verification is skipped if the “sha” config parameter is empty.  This does not comply with the policy and best practices and need to be fixed.

After downloading the software I got the error “service is neither a master nor a node” because I named the service something other than “platform-symphony-master”.  The config-changed hook should not depend on the service name for the role of the software.  If the charm were deployed from the GUI the default service name is “ibm-platform-symphony” and it is not easy for a GUI to change this.  The name of the service can be anything and should not be used to determine the role of the software.

Juju has the concept of “leadership” in charms where one charm is elected as “leader” and other charms of the similar type are “followers”.  This code that determines the master and node should be re-written to use leadership concepts: https://jujucharms.com/docs/master/authors-charm-leadership

# node-relation-changed
Over 90% of this file is no protected by the “set -e” option.  Explore the options of the commands in this file and only protect the calls that are required with “|| true” or similar workarounds.

# tests/10-deploy.py

My last comment suggested running flak8 against the test files, this test has many unused variables and that makes me wonder if the tests are valid.  While you are working on the next iteration of this charm please rewrite the sentry section to not hard-code the unit number in the name.  This is a new best practice that I just learned and want to share it with you. The existing test does it this way:  
    unit_manager_0 = d.sentry.unit['platform-symphony-master/0']
    unit_client_0 = d.sentry.unit['platform-symphony-node/0']
The best practice is to get the list of sentries and the first element (0):  
    unit_manager_0 = d.sentry['platform-symphony-master'][0]
    unit_client_0 = d.sentry['platform-symphony-node'][0]

Do it this way because the unit number could not be zero and the original test code would fail.  The new code will get the first unit no matter what the number is.

# Summary

All the files in the charm root have exectuable permissions, even the icon, and README.md file.  This is not recommended and unsafe.  One file that should be executable is `tests/10-deploy.py`.

There are some big changes needed in this submission before it can be recommended.  I am going to put this bug status back to “In Progress”.  When you have resolved all the review comments, please put the bug back in “Fix Committed” status. If you have any questions please email the list juju@lists.ubuntu.com or contact us in #juju on chat.freenode.net on IRC.