Comment 1 for bug 1459213

# platform-symphony

Greetings Neha

Thank you for the submission of the platform-symphony charm! It seems like an interesting product for grid computing. That would be a perfect addition to the charm store. Thanks for all the work that went into this charm, this product is a lot of code!

Please note that this charm is already available in the Juju Charm Store for anyone to deploy by going to the url: https://jujucharms.com/u/ibmcharmers/platform-symphony/trusty
By creating a bug you have indicated that you wish to have this code reviewed and put in the recommended section of the charm store.

Just for your reference I am reviewing the charm using the Charm Review Process document:
https://jujucharms.com/docs/stable/charm-review-process

And looking for Juju Charm best practices:
https://jujucharms.com/docs/stable/authors-charm-best-practice

# charm proof
There was only one warning from the `charm proof` tool:
W: Metadata missing required field "tags"
The file requires a “tags” entry with a value for categorization. Please see the following URL for more detail: https://jujucharms.com/docs/stable/authors-charm-metadata

# metadata.yaml
Again the maintainer needs to be in the format “Firstname Lastname <emailaddress>”. This has come up on many other IBM reviews, please use something like “IBM Juju Support Team <email address hidden>” for that field.

# README.md
Following the steps in the readme I was unable to download the file as described. The error message I got was:
```
Some selected fixes encountered errors for this order.
sym-7.1-build335711
No applicable IBM support agreement found for one or more of the products you selected.
```
Since users could get this error (as I did) the readme should include steps on what do in this case. I was able to find the download for this charm on the joint ftp server, but users will not have that backup.

The markdown readme did not convert to HTML correctly in many areas. Please check out the markdown guide found here: https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet
Several paragraphs were not terminated properly and continued to the next line. To end a line/paragraph, add two spaces after the last character. You had several formatting errors so I branched your code and suggested my own version.

The configuration section is not just a pointer to the config.yaml. The readme is rendered on the web page and may be the only contact that a user has to this charm. The configuration section needs to describe the configuration options in good detail so the user knows what values are valid, why and when they would want to change these values. This section needs more work.

# hooks
This is another IBM charm where the hook code has a mixture of spaces and tabs. This is something that is not a best practice for BASH charms and shows up with different indentation on my editor. Some if blocks are on the same indentation as the if and else statements. Please standardize on one indentation format and use indentation properly after if then else.

Another general statement for the hooks is make sure they are idempotent. What I mean by this is make sure the hooks can run multiple times as Juju may call the config-changed or node-relation-changed many times.

# config-changed
The code does not use the set -e option at the top of the config-changed hook. It is policy for bash charms to use set -e, so scripts with failures do not run after an error has occurred. If you are having problems with commands returning non zero values a bash trick is to pipe the command output to true. For example it looks like your command `egosh user logon -u Admin -x Admin` could possibly fail and return non zero. You might try running this command `egosh user logon -u Admin -x Admin || true` or some other way if you need to check the response code.

# node-relation-changed
This hook has indentation issues as well. Also please notice that line 28-29 is NOT idempotent. Those echo commands ALWAYS append to the /etc/hosts file. Old values are not removed in the broken relation. There could be many relation changed events with many clients connecting and disconnecting (breaking). After significant time that file will contain many different addresses. You will have to come up with a different way to get the relations ip addresses and update the hosts file.

# tests
It is awesome that there are tests! This will help automatically test this charm for quality and support on different platforms. It is best practice to run any code against industry lint tools. For python this is flake8 or pep8. Which produce a lot of error messages when run on `10-deploy` you will want to address these problems to ensure the quality of the tests.

# Summary
There were similar findings in this and other IBM charms. The mixture of spaces and tabs is not a best practice for bash charms and very confusing to read and follow. The readme needs to be fixed for proper markdown and formatting changes. I created a branch of code: lp:~mbruzek/charms/trusty/platform-symphony/readme that should address many of the readme formatting issues. Please continue to revise the readme with more details about the configuration and links to the IBM Knowledge Center.

Due to the issues found here, putting the bug in the “In Progress” status. The hooks have some idempotent issues that would need to be fixed before the next review. Please address all the comments made here and when you are ready for the next review put the bug back to “Fix Committed” status and someone else may review it.

Thanks again for the submission. It is great to have grid computing charms like this available in the charm store https://jujucharms.com/u/ibmcharmers/platform-symphony/trusty

If you have any questions please email the list <email address hidden> or contact us in #juju on chat.freenode.net. Thanks!