IBM Spectrum Symphony Master Charm for Review

Bug #1459213 reported by neha verma
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Undecided
Unassigned

Bug Description

Hello Team,

Here is a new charm for IBM Spectrum Symphony Master for review.

To test this charm, a license to IBM Spectrum Symphony is required.

To test the IBM Spectrum Symphony Master, we need Symphony Storage charm and Symphony Node charm as well. As per the design changes, the Platform Symphony charm has been split into three charms:

IBM Spectrum Symphony Storage
IBM Spectrum Symphony Master
IBM Spectrum Symphony Node.

We need all three charms to be deployed to have a functional Symphony Cluster.

And, its source code can be found in the below repository

Repo: https://code.launchpad.net/~ibmcharmers/ibmlayers/layer-ibm-spectrum-symphony-master

The charm has been pushed into charm store as well

branch : cs:~ibmcharmers/ibm-spectrum-symphony-master-o

Thanks

Revision history for this message
Matt Bruzek (mbruzek) wrote :
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...

Read more...

Changed in charms:
status: New → In Progress
Revision history for this message
neha verma (nevermam) wrote :

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 :

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)
Changed in charms:
status: Fix Committed → In Progress
Prabakaran (prabacha)
Changed in charms:
status: In Progress → Fix Committed
Prabakaran (prabacha)
description: updated
Revision history for this message
Matt Bruzek (mbruzek) wrote :
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...

Read more...

Changed in charms:
status: Fix Committed → In Progress
Prabakaran (prabacha)
summary: - IBM Platform Symphony Charm Creation
+ IBM Platform Symphony Master Charm for Review
Prabakaran (prabacha)
description: updated
Changed in charms:
status: In Progress → Fix Committed
Prabakaran (prabacha)
Changed in charms:
status: Fix Committed → In Progress
Prabakaran (prabacha)
summary: - IBM Platform Symphony Master Charm for Review
+ IBM Spectrum Symphony Master Charm for Review
description: updated
Prabakaran (prabacha)
Changed in charms:
status: In Progress → Fix Committed
Revision history for this message
Kevin W Monroe (kwmonroe) wrote :

Review has been migrated to:

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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers