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

Revision history for this message
Brian Lamar (blamar) 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). If it were an 'optional extension' it would be packaged separately to manage dependencies.

> 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.")

« Back to merge proposal