Code review comment for lp:~jaypipes/glance/bug713126

Revision history for this message
Armando Migliaccio (armando-migliaccio) wrote :

> On Sun, Mar 6, 2011 at 1:14 PM, Armando Migliaccio
> <email address hidden> wrote:
> > 1) It looks that the variable 'account' is still not used in the 'add'
> method
> > 2) In the glance.conf.sample it's specified:
> >
> > +# Address where the Swift authentication service lives
> > +# The auth address should be in the form:
> > +# <DOMAIN>[:<PORT>]/<VERSION>/<ACCOUNT>
> > +swift_store_auth_address = 127.0.0.1:8080/v1.0/glance-account
> >
> > so one would understand that swift_store_account is 'glance-account', but
> then the code specifies:
> >
> > +DEFAULT_SWIFT_ACCOUNT = 'glance'
>
> I meant to remove this. The account should be specified in the
> swift_store_auth_address option, and nothing else.
>
> > 3) swift_store_account is a missing option in the glance.conf.sample
>
> I removed this option.
>
> > 4) swift_store_create_container_on_put = False (I'd rather see True as a
> default option)
>
> I didn't feel comfortable setting it to True by default since that
> would change data in Swift (add the container), but I could go either
> way I suppose... this is an easy fix obviously.
>
> > All of these add up to the confusion :)
> >
> > I would like to see glance.conf.sample with a set of sensible defaults so
> that one who does not have a great deal of experience with glance/swift can
> get up and running without troubles. The above mentioned points make me feel
> we are not quite there yet :)
>
> So would I. :) Unfortunately, its not exactly simple to setup Swift
> and get everything working without, frankly, knowing what you are
> doing and knowing how Swift authentication works (some "auth service"
> with a URL like http://localhost:8080/v1.0/account/ authenticates a
> user/key combination for an account and returns a storage management
> URL that subsequent requests use as their base request URL...
>

fair enough!

> Anyway, I've tried my best to make it sensible to configure *if* you
> know how to setup Swift, but I was tentative to try anything more
> complex.
>
> -jay

I think you did great, as I was able to get this up and running fairly quickly. Apart the niggley bit of

> > +DEFAULT_SWIFT_ACCOUNT = 'glance'

lying around in the code, this branch has both thumbs up from my side

review: Approve

« Back to merge proposal