Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peer Review Requested #1

Open
lazypower opened this issue Jul 22, 2014 · 1 comment
Open

Peer Review Requested #1

lazypower opened this issue Jul 22, 2014 · 1 comment

Comments

@lazypower
Copy link

Not exactly an issue but I like to keep things tidy and together.

Thanks for the submission of the reddit-charm as a WIP. I've taken a look through the source and I have to say I'm fairly impressed with the status of the charm from a new charm author. I'm going to be running this review as if it were a fully formed submission to the charm store - so take all comments as suggestions for development and not hard requirements.

Charm Proof

W: Includes template icon.svg file.
W: no copyright file
W: Includes template README.ex file
W: README.ex includes line 5 of boilerplate README.ex
W: README.ex includes line 11 of boilerplate README.ex
W: README.ex includes line 13 of boi

All charms must include an icon in SVG format for inclusion to the charm store. You can review the howto build a proper Juju icon here

All charms also should be licensed under an OpenSource license.

I see there are some matches on boilerplate in the README, this is a common and as a W: is not a blocker for charm store inclusion.

Code Review

I see that you have plans on integrating the reddit charm with 4 services, this is nice to see from a new charm author that you're not shying away from integrating with several relationships. I look forward to seeing the approaches you take with them.

config.yaml

I see that the default configuration exists here. It would be nice to expose opinionated deployment options for the user here where they can select different deployment profiles, such as developer - which includes the extra packages you've got commented out for dev purposes, and a tuned option which includes all of the caching mechanisms for the application out of the box.

Another nice option here would be to include the REVNO and REPO as a configuration option as I see you are using git based deployment. This would help alleviate the added stress on the charm author when the charm appears 'broken' due to a broken release being in -HEAD, or someone that wants to fork the reddit service and run their own patches.

Wrap up

Aside from the notes above, I have no further knitpicks as the charm is progressing nicely. I look forward to another submission and review of the reddit charm to see the differences in approach you take with the relationships and what kind of deployment options you expose to the user.

Thanks again, and all the best!

@arosales
Copy link

Thanks for the work here, your design looks well thought out, and great to see you using Charm Helpers.

+1 to all of a Chuck's comments. charm proof is available in the charm-tools package in Ubuntu, if you want to run that against your later revisions.

Also, it would be interesting to see what is minimally needed to deploy Reddit and see a working example (albeit not with all the optimal config and relations).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants