© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
1b1ed1a
(Get the code!)
Greetings Uma,
Thank you for your contribution to the Juju charm store. We appreciate your efforts to bring the IBM Websphere Liberty base to our charm ecosystem, and welcome the contributions. I've taken some time to review your work on this charm, and I have a few notes:
# Charm proof, Readme, and Metadata
By running `charm proof` I noticed that the hook is not implemented. While this is only an I: level message, the relationship should at bare minimum have a hook that sends the port that websphere is going to be running on. This is a necessary component to the Juju relationship model, as the interface requires 2 datapoints: a private address and a port. When deploying websphere, if I were to add a relationship to HAPROXY as a reverse proxy - it would fail due to this missing data point.
When I opened the README to inspect and investigate the deployment routine, I see a significant block of instructions that calls out to "setting up an apache repository". This leads me to believe that there is an expected manual component to the setup and If i'm understanding it would look like this:
- Setup another server which will warehouse the downloaded binaries
- Preload the server with said binaries
- point this charm at that server, along with a path to find the binaries hosted over an HTTP connection
This breaks another core tenant of the Juju Charm store - charms should have sane defaults and be deployable out of the box with minimal effort from the user. I feel that there are other options available here that have not been fully investigated. I'm going to encourage you to investigate other charms that we have in the charm store that are behind EULA/LoginWalls and evaluate their practices encapsulated in charms - such as the MariaDB Charm, which installs a Community edition by default. When the user decides to upgrade to the Enterprise edition, you set the configuration to accept the EULA, and provide an apt string that contains a unique login/password embedded.
https:/
Cory mentioned a UX with regard to combining the config values into a single string, and parsing that code side in the charm if required - I am +1 to that advice and re-state it here. Reducing the overall complexity of the charm config by being opinionated where you can be is a +1 to our core tenants.
# Charm Code
I see you have included some basic standup tests for the websphere_base charm, which is great! I'd like to see the deployment formation flexed bit more including a reverse proxy to exercise the website relationship, and additionally the config options should be altered and validated as well.
Looking through the code, the installation hook raises a few concerns. None of the downloads are being cryptographically signed. This poses a problem as it's subject to a few potential problems here. The charm could be compromised by a man in the middle attack by not delivering over HTTPS. In addition, the lack of CRC validation - we have no idea if the download completed successfully receiving the intended package.
For each file being downloaded, I highly recommend setting it as a configuration variable for 2 things. The source URL of the download, and the SHA1 sum of the intended package, which you can then use against the download.
Looking through the config-changed hook code, I see that there is a fair amount of logic checking if files exist in the $CHARM_DIR/files path. This is a really good pattern to have in here - as we have a concept called 'fatpacking' charms. If you were to add a makefile target that would fetch the appropriate packages, CRC validate them (prior to deployment), I would be OK with accepting the charm with this functionality vs setting up a separate server to only warehouse binaries for the installation. While its not the best user experience, its an acceptable alternative to deploying a third party dependency just for the installation of the Websphere application.
I encourage you to take a look through our documentation on the Juju Docs about the Charm Store Policy, with a keen eye towards what we will be reviewing the charm for: https:/
Thanks a bunch for the submission, the initial charm is a good first cut, and with some modifications this will be a great addition to the charm store. Unfortunately I am unable to accept this submission in its current state. If you feel that you need additional help to clean up the charm according to the review comments here feel free to join us in IRC or reach out over the mailing list and we will be happy to lend a hand in your success charming.
If you have any questions/