Bug #1459213 “IBM Spectrum Symphony Master Charm for Review” : Bugs : Juju Charms Collection

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-06-26:

#1

Download full text (5.8 KiB)

# 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 tim...

# 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 <jujusupp@us.ibm.com>” 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 juju@lists.ubuntu.com or contact us in #juju on chat.freenode.net.  Thanks!

Changed in charms:
status:	New → In Progress

Revision history for this message

neha verma (nevermam) wrote on 2015-07-08:

#2

Thanks Matt for reviewing the code.
I will take care of comments given by you and will revert back soon.

Revision history for this message

Prabakaran (prabacha) wrote on 2015-08-11:

#3

Hi Team,

In order to download the platform symphony fixes , user has to be a paid symphony customer then only they should be able to see the fix pack on fix central.

We have not tested this charm on power as correct packages are not available. We are following up with platform symphony product team in order to obtain correct packages. Will keep you posted on the same.

Thanks,

Changed in charms:
status:	In Progress → Fix Committed

sharan (saran110) on 2015-09-28

Changed in charms:
status:	Fix Committed → In Progress

Prabakaran (prabacha) on 2015-09-28

Changed in charms:
status:	In Progress → Fix Committed

Prabakaran (prabacha) on 2015-11-02

description:

updated

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-11-25:

#4

Download full text (5.2 KiB)

# 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 t...

# 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 juju@lists.ubuntu.com or contact us in #juju on chat.freenode.net on IRC.

Changed in charms:
status:	Fix Committed → In Progress

Prabakaran (prabacha) on 2016-05-10

summary:

- IBM Platform Symphony Charm Creation
+ IBM Platform Symphony Master Charm for Review

Prabakaran (prabacha) on 2016-05-10

description:	updated
Changed in charms:
status:	In Progress → Fix Committed

Prabakaran (prabacha) on 2016-06-07

Changed in charms:
status:	Fix Committed → In Progress

Prabakaran (prabacha) on 2016-08-12

summary:	- IBM Platform Symphony Master Charm for Review + IBM Spectrum Symphony Master Charm for Review
description:	updated

Prabakaran (prabacha) on 2016-08-12

Changed in charms:
status:	In Progress → Fix Committed

Revision history for this message

Kevin W Monroe (kwmonroe) wrote on 2016-10-06:

#5

Review has been migrated to:

https://review.jujucharms.com/reviews/15

Juju Charms Collection

IBM Spectrum Symphony Master Charm for Review

Bug Description

Related branches

Other bug subscribers

Remote bug watches