Bug #1007419 “Django-ranbow charm needs review” : Bugs : Juju Charms Collection

Revision history for this message

Adam Gandelman (gandelman-a) wrote on 2012-06-12:

#1

Hi Robert-

Thanks for submitting. This is very cool stuff.

A couple of things that popped out at me on first review:

- I'd prefer the 'package-installation-directory' to default to somewhere other than /home/server-packages. Traditionally, /home should be reserved for users' homes and not a packaging dumps. Perhaps /var/lib/juju/rainbow/server-packages or something similar?

- While I'm totally cool with the 'purge-instance' and 're-install-mysql-server' related parts of the install hook, other people are more conservative with regard to instance re-use and would probably prefer users destroy and redeploy units rather than use the install hook as a sort of container scrubber.

- rainbow-relation-changed uses open-port to open up port 80, though the vhost config for the application is customizable in the corresponding stripe charm. If a user configures a stripe charm to setup a vhost on something other than port 80, shouldn't the rainbow charm be able to detect this and open up the correct port?

I'm a little unsure about the charms reliance on corresponding stripe charm. I'll open that review @
https://bugs.launchpad.net/charms/+bug/1007422

Revision history for this message

Adam Gandelman (gandelman-a) wrote on 2012-06-12:

#2

So, I'm a little bit worried about the use of an essentially no-op service that only exists to pass relation data to another unit that will be doing all the work. It seems a bit wasteful, as we'll need to spin up 2x the resources (read: $$$) for a single application, and half of that will be doing *nothing* after its been related. I'd love to see the functionality of this charm merged into the main django-rainbow charm in some way. I understand that's a bit difficult to do and still support many applications on the same rainbow server. Without Juju support for passing in arbitrary and variable-length config, it certainly gets tricky.

One idea that comes to mind would (AFAICS) support the current functionality of the pair, but with a single django-rainbow charm:

- Move all of the config options of the stripe charm into the config.yaml of django-rainbow, all defaulting to null.
- The current 'rainbow-relation-changed' hook becomes part of the config-changed hook of django-rainbow. Instead of calling 'relation-get', call 'config-get' instead.

Users can then deploy django-rainbow with or without the config options set. After its been deployed, they can set the corresponding application config settings and update the services config. This will trigger a config-changed event and setup the corresponding application. At any point, config-get will show the last installed application. I can picture a user maintaining a repository of config snippets for each application they might want to install. This would make it easier to manage config changes for installed applications later on. As it is, you'd have to trigger relation events from config-changed on the stripe side if a user wanted to (say) change something in the vhost config for an app.

My two cents, at least. Leaving in queue to get some input from others.

A side note, I believe its unnecessary to have users copy the charm, and update the metadata with custom name. Instead, you can 'juju deploy --repository=. local:stripe stripe-mycoolapp' and have the charm look to its enviromment for its unique name ($JUJU_UNIT_NAME)

So, I'm a little bit worried about the use of an essentially no-op service that only exists to pass relation data to another unit that will be doing all the work.  It seems a bit wasteful, as we'll need to spin up 2x the resources (read: $$$) for a single application, and half of that will be doing *nothing* after its been related.   I'd love to see the functionality of this charm merged into the main django-rainbow charm in some way.   I understand that's a bit difficult to do and still support many applications on the same rainbow server.   Without Juju support for passing in arbitrary and variable-length config, it certainly gets tricky.

One idea that comes to mind would (AFAICS) support the current functionality of the pair, but with a single django-rainbow charm:

- Move all of the config options of the stripe charm into the config.yaml of django-rainbow,  all defaulting to null.  
- The current 'rainbow-relation-changed' hook becomes part of the config-changed hook of django-rainbow.  Instead of calling 'relation-get', call 'config-get' instead.

Users can then deploy django-rainbow with or without the config options set.  After its been deployed, they can set the corresponding application config settings and update the services config.  This will trigger a config-changed event and setup the corresponding application.  At any point,  config-get will show the last installed application.  I can picture a user maintaining a repository of config snippets for each application they might want to install.   This would make it easier to manage config changes for installed applications later on.  As it is, you'd have to trigger relation events from config-changed on the stripe side if a user wanted to (say) change something in the vhost config for an app.

My two cents, at least.  Leaving in queue to get some input from others.

A side note, I believe its unnecessary to have users copy the charm, and update the metadata with custom name.   Instead, you can 'juju deploy --repository=. local:stripe stripe-mycoolapp' and have the charm look to its enviromment for its unique name ($JUJU_UNIT_NAME)

Revision history for this message

Robert Steckroth (robertsteckroth) wrote on 2012-06-12: Re: [Bug 1007419] Re: Django-ranbow charm needs review

#3

Hey, thanks for the input fellas. I am under the weather as of now,
but am looking forward to
parsing through your input. Man I hope I get some good rest tonight.

On Mon, Jun 11, 2012 at 8:53 PM, Adam Gandelman
<email address hidden> wrote:
> So, I'm a little bit worried about the use of an essentially no-op
> service that only exists to pass relation data to another unit that will
> be doing all the work. It seems a bit wasteful, as we'll need to spin
> up 2x the resources (read: $$$) for a single application, and half of
> that will be doing *nothing* after its been related. I'd love to see
> the functionality of this charm merged into the main django-rainbow
> charm in some way. I understand that's a bit difficult to do and still
> support many applications on the same rainbow server. Without Juju
> support for passing in arbitrary and variable-length config, it
> certainly gets tricky.
>
> One idea that comes to mind would (AFAICS) support the current
> functionality of the pair, but with a single django-rainbow charm:
>
> - Move all of the config options of the stripe charm into the config.yaml of django-rainbow, all defaulting to null.
> - The current 'rainbow-relation-changed' hook becomes part of the config-changed hook of django-rainbow. Instead of calling 'relation-get', call 'config-get' instead.
>
> Users can then deploy django-rainbow with or without the config options
> set. After its been deployed, they can set the corresponding
> application config settings and update the services config. This will
> trigger a config-changed event and setup the corresponding application.
> At any point, config-get will show the last installed application. I
> can picture a user maintaining a repository of config snippets for each
> application they might want to install. This would make it easier to
> manage config changes for installed applications later on. As it is,
> you'd have to trigger relation events from config-changed on the stripe
> side if a user wanted to (say) change something in the vhost config for
> an app.
>
> My two cents, at least. Leaving in queue to get some input from others.
>
> A side note, I believe its unnecessary to have users copy the charm, and
> update the metadata with custom name. Instead, you can 'juju deploy
> --repository=. local:stripe stripe-mycoolapp' and have the charm look to
> its enviromment for its unique name ($JUJU_UNIT_NAME)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1007419
>
> Title:
> Django-ranbow charm needs review
>
> Status in Juju Charms:
> New
>
> Bug description:
> Very basic but a good start to a Django project charm. Needs review
> for public offering.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1007419/+subscriptions

--
Bust0ut, Surgemcgee: Systems Engineer ---
PBDefence.com
BudTVNetwork.com
RadioWeedShow.com
"Bringing entertainment to Unix"

Hey, thanks for the input fellas. I am under the weather as of now,
but am looking forward to
parsing through your input. Man I hope I get some good rest tonight.

On Mon, Jun 11, 2012 at 8:53 PM, Adam Gandelman
<1007419@bugs.launchpad.net> wrote:
> So, I'm a little bit worried about the use of an essentially no-op
> service that only exists to pass relation data to another unit that will
> be doing all the work.  It seems a bit wasteful, as we'll need to spin
> up 2x the resources (read: $$$) for a single application, and half of
> that will be doing *nothing* after its been related.   I'd love to see
> the functionality of this charm merged into the main django-rainbow
> charm in some way.   I understand that's a bit difficult to do and still
> support many applications on the same rainbow server.   Without Juju
> support for passing in arbitrary and variable-length config, it
> certainly gets tricky.
>
> One idea that comes to mind would (AFAICS) support the current
> functionality of the pair, but with a single django-rainbow charm:
>
> - Move all of the config options of the stripe charm into the config.yaml of django-rainbow,  all defaulting to null.
> - The current 'rainbow-relation-changed' hook becomes part of the config-changed hook of django-rainbow.  Instead of calling 'relation-get', call 'config-get' instead.
>
> Users can then deploy django-rainbow with or without the config options
> set.  After its been deployed, they can set the corresponding
> application config settings and update the services config.  This will
> trigger a config-changed event and setup the corresponding application.
> At any point,  config-get will show the last installed application.  I
> can picture a user maintaining a repository of config snippets for each
> application they might want to install.   This would make it easier to
> manage config changes for installed applications later on.  As it is,
> you'd have to trigger relation events from config-changed on the stripe
> side if a user wanted to (say) change something in the vhost config for
> an app.
>
> My two cents, at least.  Leaving in queue to get some input from others.
>
> A side note, I believe its unnecessary to have users copy the charm, and
> update the metadata with custom name.   Instead, you can 'juju deploy
> --repository=. local:stripe stripe-mycoolapp' and have the charm look to
> its enviromment for its unique name ($JUJU_UNIT_NAME)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1007419
>
> Title:
>  Django-ranbow charm needs review
>
> Status in Juju Charms:
>  New
>
> Bug description:
>  Very basic but a good start to a Django project charm. Needs review
>  for public offering.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1007419/+subscriptions

-- 
Bust0ut, Surgemcgee: Systems Engineer ---
PBDefence.com
BudTVNetwork.com
RadioWeedShow.com
"Bringing entertainment to Unix"

Revision history for this message

Robert Steckroth (robertsteckroth) wrote on 2012-06-12:

#4

Wow, ok feel better now (thought I was gonna need organ transplants), jeesh.
I feel the above recommendations warrant pursuing. It is flattering
to have a professional opinion of my work and I do not take it lightly.
The rainbow charm should stay in an extended queue placement, as I wish
to develop these ideas further. The stripe charm should/will be
discontinued pending
any "real" functionality.

--
Bust0ut, Surgemcgee: Systems Engineer ---
PBDefence.com
BudTVNetwork.com
RadioWeedShow.com
"Bringing entertainment to Unix"

Revision history for this message

Jorge Castro (jorge) wrote on 2012-06-12:

#5

Hi Robert, I've unsubscribed "charmers" from this bug to give you plenty of time to work on this. When you're ready for another review just subscribe "charmers" and we'll go for another round, thanks for your contribution!

Revision history for this message

Clint Byrum (clint-fewbar) wrote on 2012-06-28:

#6

really un-subbing charmers again. Please re-submit when you think the charm is ready for another review.

Revision history for this message

Robert Steckroth (robertsteckroth) wrote on 2012-06-28:

#7

Ok, now you got me worried :0
Is this in regards to the package installation directory's? I pushed a fix
for that and will re-subscribe charmers to it now.
Appreciate your time.

On Thu, Jun 28, 2012 at 5:23 PM, Clint Byrum <email address hidden> wrote:

> really un-subbing charmers again. Please re-submit when you think the
> charm is ready for another review.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1007419
>
> Title:
> Django-ranbow charm needs review
>
> Status in Juju Charms:
> New
>
> Bug description:
> Very basic but a good start to a Django project charm. Needs review
> for public offering.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1007419/+subscriptions
>

--
Bust0ut, Surgemcgee: Systems Engineer ---
PBDefence.com
BudTVNetwork.com
RadioWeedShow.com
"Bringing entertainment to Unix"

Juju Charms Collection

Django-ranbow charm needs review

Bug Description

Other bug subscribers

Remote bug watches