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

Revision history for this message
Jay Pipes (jaypipes) wrote :

> To me this branch was long overdue :)

:) Yes, I know.

> At a first _glance_, I couldn't find anything wrong with it...only two small
> niggley bits:
>
> - maybe if tests don't find swift they should say 'skipped' rather than 'ok',
> but I don't know if nosetest can do that easily. The effect is the same but it
> just draws attention to the user

Yes, I thought about this. I had originally used unittest2's @skipIf decorator, but I had proposed an earlier branch and people didn't want to require dependency on unittest2. nose has some ability to do skipping; I could check into this.

> - the default swift store options are a bit weak: DEFAULT_SWIFT_ACCOUNT =
> 'glance' and DEFAULT_SWIFT_CONTAINER = 'glance' do not seem very unique to me
> :)

Well, the account and container are more general-purpose than anything to do with authenticating (user and key are for that purpose). What do you think I should set account and container to?

> Other than that, I'd like to give this branch a whirl on a live swift
> deployment we got here before giving a full 'approve' from my side :)

Can't wait for feedback! Thanks, Armando!

-jay

« Back to merge proposal