Code review comment for lp:~rconradharris/nova/backup_schedule_extension

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

> > Having `import tempo` in the `__init__` means that it won't be imported
> unless the Backup_schedule
> > class is actually created.
>
> Right, but we're putting Tempo in pip-requires and in the Ubuntu package
> right? So this should be considered a core extension (I'll attempt to not get
> started on the hilarity of that statement to me).

Indeed, which is why I think the term "extension" has been a little bit abused in Nova ;)

> If it were an 'optional
> extension' it would be packaged separately to manage dependencies.

Indeed.

> > Since API extensions are eager-loaded--everything in the 'contrib' directory
> is imported in search
> > of an Extension class--I think it makes sense not to have 3rd party imports
> in the module-scope.
>
> This extension is in the base source code checkout, so if I checkout the Nova
> source, and do ./run_tests.sh it's going to require tempo, right? If so it's a
> requirement of the project unless we're up front about it and make the tests
> skip if tempo isn't installed.

++

> > If we did, that would mean that users would be required to have every
> dependency of every
> > (optional) extension. I don't think that scales.
>
> No argument here. I really don't want to have tempo in pip-requires or the
> Ubuntu package if I'm not going to use it. Since it's there (or going to be
> there?), shouldn't we just show the imports up front?
>
> My main argument though is that imports should always be up top in my opinion,
> even if you have to do something like:
>
> try:
> import tempo
> except ImportError:
> LOG.warn("Tempo not installed, backup extension won't work.")

Agree fully.
-jay

« Back to merge proposal