Juju Charms Collection

Bug #1446999
Comment #5

Comment 5 for bug 1446999

Revision history for this message

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

Hello Uma,

Welcome to the Juju community! WebSphere is a large complex product and I appreciate the effort it took to bring WebSphere Network Deployment into a charm!

When reviewing a charm I generally look for Best Practices and Charm Store Policy
https://jujucharms.com/docs/devel/authors-charm-best-practice
https://jujucharms.com/docs/devel/authors-charm-policy

Here what I found on my review:

# config.yaml
The configuration values look to have defaults and the correct values. The was_nd_url, and im_install_path description could both have better descriptions with examples of valid values or just more description of what needs to be in these configuration values. For example I did not know what path the url was expecting. Looking at the code it appears that names are appended to the end of a $was_repository, so the README or config.yaml could point this out as an example.

# copyright
The copyright doesn't need to be Canonical, it just needs to be an open source license. We will accept “Canonical Ltd” but this could be IBM.

# icon.svg
The icon looks fine thanks for updating that one!

# metadata.yaml
The metadata suffers from other reviews that I have done, the maintainer field must be “Juju Support Team <email address hidden>” or something similar. I see the metadata defines a “website” relation but see no hooks for this relation! The website relation is very easy to implement you only need to set the hostname=$private-address and port=$ in a website-relation-joined hook.

# README.md
The Overview section is very brief and could contain more detail about what WebSphere Network Deployment is and why someone would use it.

The “Creating the repositories” section was confusing. Like other IBM charms this one requires a separate download and configuration but the steps were very vague and it was not clear to me that these steps must be done on another server or my own system. Please note that markdown interprets greater than (>) and less than (<) signs so "http://<server-name>/debs/" gets incorrectly rendered to “http:///debs/” in the HTML result. This section needs more detail and description of exactly what the user needs to do, with links to trial versions if possible.

The “Usage” section could again have more detail. Upload to where, and what directory? Could you give examples of directory names or file names. Since this is a manual process this is prone to more errors so the instructions must be as clear as possible. I did not find links on where to find the IBM Installation Manager. Each charm must be self contained and if you need IBM IM for this charm the README should indicate where to get it.

The “Configuration” section should have more description or examples of the configuration options as was suggested in the config.yaml review.

The “Contact Information” section should have links to the WebSphere forums or bug trackers, or Information Centers where users can get more information about WebSphere Network Deployment software.

# hooks
Many of the hooks mix spaces and tabs which is not a best practice for Bash charms. Please choose one and remain consistent throughout all the hooks.

# install
As Cory pointed out in comment #2 the was_url and im_file_name are immutable and can not be changed after install. The configuration should not be immutable (best practices). There is also no crypographic verification of the downloads to the charm. Please refer to the best practices and charm store policy links at the top of this review.

# config-changed
Again this file has a mix of spaces and tabs which makes it hard to look at in some editors. All hooks run as “root” user already there is no need to use “sudo” (found 6 times). Also using the bash best practice of “set -e” will cause the hook to stop on any command returning a non zero result. So checking for the non zero case is not necessary. The code should still attempt to handle error cases when appropriate.

I really like that you have log messages in the hook, which will help for debugging and understanding what is going on from the unit log file.

The config-changed hook is the only one run when configuration changes. If you don't read all the configuration options in this file the configuration is immutable and the user does not know which configuraiton options they can change or not.

# website-relation-joined
I did not see a website-relation-joined hook in this code. Comment #3 suggested that you fixed this, but I don't see it in the current code. You will need this hook if you want to work with reverse proxies or other Juju charms that consume (require) the website relation

Thank you for submitting this charm for review. Please remember this is an iterative process and we all want high quality charms in the charm store. Look over the comments in this review and read the best practices and charm store policy links above. I will change the status to “In Progress”, please change the status to “Fix Committed” when you are ready for another review.

Please reach out to the community in #juju on chat.freenode.net if you have questions or need some help. You can also email the <email address hidden> mailing list or email me directly.

Hello Uma,

Welcome to the Juju community!  WebSphere is a large complex product and I appreciate the effort it took to bring WebSphere Network Deployment into a charm!

When reviewing a charm I generally look for Best Practices and Charm Store Policy
https://jujucharms.com/docs/devel/authors-charm-best-practice
https://jujucharms.com/docs/devel/authors-charm-policy

Here what I found on my review:

# config.yaml
The configuration values look to have defaults and the correct values.  The was_nd_url, and im_install_path description could both have better descriptions with examples of valid values or just more description of what needs to be in these configuration values.  For example I did not know what path the url was expecting.  Looking at the code it appears that names are appended to the end of a $was_repository, so the README or config.yaml could point this out as an example.

# copyright
The copyright doesn't need to be Canonical, it just needs to be an open source license.  We will accept “Canonical Ltd” but this could be IBM.

# icon.svg
The icon looks fine thanks for updating that one!

# metadata.yaml
The metadata suffers from other reviews that I have done, the maintainer field must be “Juju Support Team <jujusupp@us.ibm.com>” or something similar.  I see the metadata defines a “website” relation but see no hooks for this relation!  The website relation is very easy to implement you only need to set the hostname=$private-address and port=$ in a website-relation-joined hook.

# README.md
The Overview section is very brief and could contain more detail about what WebSphere Network Deployment is and why someone would use it.

The “Creating the repositories” section was confusing.  Like other IBM charms this one requires a separate download and configuration but the steps were very vague and it was not clear to me that these steps must be done on another server or  my own system.  Please note that markdown interprets greater than (>) and less than (<) signs so "http://<server-name>/debs/" gets incorrectly rendered to “http:///debs/” in the HTML result.  This section needs more detail and description of exactly what the user needs to do, with links to trial versions if possible.

The “Usage” section could again have more detail.  Upload to where, and what directory?  Could you give examples of directory names or file names.  Since this is a manual process this is prone to more errors so the instructions must be as clear as possible.  I did not find links on where to find the IBM Installation Manager.  Each charm must be self contained and if you need IBM IM for this charm the README should indicate where to get it.

The “Configuration” section should have more description or examples of the configuration options as was suggested in the config.yaml review.

# hooks
Many of the hooks mix spaces and tabs which is not a best practice for Bash charms.  Please choose one and remain consistent throughout all the hooks.

# install 
As Cory pointed out in comment #2 the was_url and im_file_name are immutable and can not be changed after install.  The configuration should not be immutable (best practices).  There is also no crypographic verification of the downloads to the charm.  Please refer to the best practices and charm store policy links at the top of this review.

# config-changed
Again this file has a mix of spaces and tabs which makes it hard to look at in some editors.  All hooks run as “root” user already there is no need to use “sudo” (found 6 times).  Also using the bash best practice of “set -e” will cause the hook to stop on any command returning a non zero result.  So checking for the non zero case is not necessary.  The code should still attempt to handle error cases when appropriate.

I really like that you have log messages in the hook, which will help for debugging and understanding what is going on from the unit log file.

The config-changed hook is the only one run when configuration changes.  If you don't read all the configuration options in this file the configuration is immutable and the user does not know which configuraiton options they can change or not.

# website-relation-joined
I did not see a website-relation-joined hook in this code.  Comment #3 suggested that you fixed this, but I don't see it in the current code. You will need this hook if you want to work with reverse proxies or other Juju charms that consume (require) the website relation

Thank you for submitting this charm for review.  Please remember this is an iterative process and we all want high quality charms in the charm store.  Look over the comments in this review and read the best practices and charm store policy links above.  I will change the status to “In Progress”, please change the status to “Fix Committed” when you are ready for another review.

Please reach out to the community in #juju on chat.freenode.net if you have questions or need some help.  You can also email the juju@lists.ubuntu.com mailing list or email me directly.