Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-08 Thread Matthew Booth
On 07/08/14 18:54, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 17:46 +0100, Matthew Booth wrote:
 In any case, the operative point is that CONF.attribute must
 always be
 evaluated inside run-time code, never at module load time.

 ...unless you call register_opts() safely, which is what I'm
 proposing.
 
 No, calling register_opts() at a different point only fixes the import
 issue you originally complained about; it does not fix the problem that
 the configuration option is evaluated at the wrong time.  The example
 code you included in your original email evaluates the configuration
 option at module load time, BEFORE the configuration has been loaded,
 which means that the argument default will be the default of the
 configuration option, rather than the configured value of the
 configuration option.  Configuration options must be evaluated at
 RUN-TIME, after configuration is loaded; they must not be evaluated at
 LOAD-TIME, which is what your original code does.

Ah, thanks, Kevin. The pertinent information is that the config has not
been loaded at module import time, and you'll therefore always get a
default.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-08 Thread Matthew Booth
On 07/08/14 19:02, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 17:41 +0100, Matthew Booth wrote:
 ... or arg is an object which defines __nonzero__(), or defines
 __getattr__() and then explodes because of the unexpected lookup of a
 __nonzero__ attribute. Or it's False (no quotes when printed by the
 debugger), but has a unicode type and therefore evaluates to True[1].
 
 If you're passing such exotic objects as parameters that could
 potentially be drawn from configuration instead, maybe that code needs
 to be refactored a bit :)
 
 However, if you want to compare a value with None and write 'foo is
 None' it will always do exactly what you expect, regardless what you
 pass to it. I think it's also nicer to the reviewer and the
 maintainer,
 who then don't need to go looking for context to check if anything
 invalid might be passed in.
 
 In the vast majority of cases, however, we use a value that evaluates to
 False to indicate use the default, where default may be drawn from
 configuration.  Yes, there are cases where we must treat, say, 0 as
 distinct from None, but when we don't need to, we should keep the code
 as simple as possible.  After all, I doubt anyone would seriously
 suggest that we must always use something like the _unset sentinel,
 even when None has no special meaning…

I've found a few bugs in Openstack by checking implicit boolean tests
while reviewing code. Here's a recent one:

https://review.openstack.org/#/c/109006/1/nova/db/sqlalchemy/api.py

Note that the caller has accidentally passed read_deleted=False, a
pretty easy mistake to make, and the bare object test has hidden that
error and silently replaced it with a default.

Also note that while PEP8 stops short of decreeing against bare object
tests, it does recommend against it. See the section 'Programming
Recommendations':

http://legacy.python.org/dev/peps/pep-0008/

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-08 Thread Matthew Booth
On 08/08/14 11:04, Matthew Booth wrote:
 On 07/08/14 18:54, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 17:46 +0100, Matthew Booth wrote:
 In any case, the operative point is that CONF.attribute must
 always be
 evaluated inside run-time code, never at module load time.

 ...unless you call register_opts() safely, which is what I'm
 proposing.

 No, calling register_opts() at a different point only fixes the import
 issue you originally complained about; it does not fix the problem that
 the configuration option is evaluated at the wrong time.  The example
 code you included in your original email evaluates the configuration
 option at module load time, BEFORE the configuration has been loaded,
 which means that the argument default will be the default of the
 configuration option, rather than the configured value of the
 configuration option.  Configuration options must be evaluated at
 RUN-TIME, after configuration is loaded; they must not be evaluated at
 LOAD-TIME, which is what your original code does.
 
 Ah, thanks, Kevin. The pertinent information is that the config has not
 been loaded at module import time, and you'll therefore always get a
 default.

Ironically, the specific instance which prompted this investigation[1]
is in a driver. As drivers are imported dynamically after the config has
been loaded, using a config variable in import context will actually
work as intended. Relying on that behaviour seems pretty nasty to me,
though. We could probably do with a guideline on this.

I did a quick scan of Nova and found 12 instances which aren't in a
driver which appear to be broken due to this at first glance, resulting
in 11 config variables whose specified values appear to be ignored. Bug
here:

https://bugs.launchpad.net/nova/+bug/1354403

Matt

[1]
https://review.openstack.org/#/c/104145/14/nova/virt/vmwareapi/vmware_images.py
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Doug Hellmann

On Aug 7, 2014, at 12:39 PM, Kevin L. Mitchell kevin.mitch...@rackspace.com 
wrote:

 On Thu, 2014-08-07 at 17:27 +0100, Matthew Booth wrote:
 On 07/08/14 16:27, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config.
 
 Actually, I disagree.  The real problem here is the definition of
 bar_func().  The default value of the parameter arg will likely always
 be the default value of foo_opt, rather than the configured value,
 because CONF.foo_opt will be evaluated at module load time.  The way
 bar_func() should be defined would be:
 
def bar_func(arg=None):
if not arg:
arg = CONF.foo_opt
…
 
 That ensures that arg will be the configured value, and should also
 solve the import conflict.
 
 That's different behaviour, because you can no longer pass arg=None. The
 fix isn't to change the behaviour of the code.
 
 Well, the point is that the code as written is incorrect.  And if 'None'
 is an input you want to allow, then use an incantation like:
 
_unset = object()
 
def bar_func(arg=_unset):
if arg is _unset:
arg = CONF.foo_opt
…
 
 In any case, the operative point is that CONF.attribute must always be
 evaluated inside run-time code, never at module load time.

It would be even better to take the extra step of registering the option at 
runtime at the point it is about to be used by calling register_opt() inside 
bar_func() instead of when bar is imported. That avoids import order concerns, 
and re-enforces the idea that options should be declared local to the code that 
uses them and their values should be passed to other code, rather than having 2 
modules tightly bound together through a global configuration value.

Doug


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Brant Knudson
On Thu, Aug 7, 2014 at 12:54 PM, Kevin L. Mitchell 
kevin.mitch...@rackspace.com wrote:

 On Thu, 2014-08-07 at 17:46 +0100, Matthew Booth wrote:
   In any case, the operative point is that CONF.attribute must
  always be
   evaluated inside run-time code, never at module load time.
 
  ...unless you call register_opts() safely, which is what I'm
  proposing.

 No, calling register_opts() at a different point only fixes the import
 issue you originally complained about; it does not fix the problem that
 the configuration option is evaluated at the wrong time.  The example
 code you included in your original email evaluates the configuration
 option at module load time, BEFORE the configuration has been loaded,
 which means that the argument default will be the default of the
 configuration option, rather than the configured value of the
 configuration option.  Configuration options must be evaluated at
 RUN-TIME, after configuration is loaded; they must not be evaluated at
 LOAD-TIME, which is what your original code does.
 --
 Kevin L. Mitchell kevin.mitch...@rackspace.com
 Rackspace


We had this problem in Keystone[1]. There were some config parameters
passed to a function decorator (it was the cache timeout time). You'd
change the value in the config file and it would have no effect... the
default was still used. Luckily the cache decorator also took a function so
it was an easy fix, just pass `lambda: CONF.foo`. The mistaken code was
made possible because the config options were registered at import time.
Keystone now registers its config options at run-time so using CONF.foo at
import-time fails with an error that the option isn't registered.

[1] https://bugs.launchpad.net/keystone/+bug/1265670

- Brant
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


[openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matthew Booth
I'm sure this is well known, but I recently encountered this problem for
the second time.

---
foo:
import oslo.config as cfg

import bar

CONF = cfg.CONF
CONF.register_opts('foo_opt')

---
bar:
import oslo.config as cfg

CONF = cfg.CONF

def bar_func(arg=CONF.foo_opt):
  pass
---

importing foo results in an error in bar because CONF.foo_opt doesn't
exist. This is because bar is imported before CONF.register_opts.
CONF.import_opt() fails in the same way because it just imports foo and
hits the exact same problem when foo imports bar.

A (the?) solution is to register_opts() in foo before importing any
modules which might also use oslo.config. This also allows import_opt()
to work in bar, which you should do to remove any dependency on import
order:

---
foo:
import oslo.config as cfg

CONF = cfg.CONF
CONF.register_opts('foo_opt')

import bar

---
bar:
import oslo.config as cfg

CONF = cfg.CONF
CONF.import_opt('foo_opt', 'foo')

def bar_func(arg=CONF.foo_opt):
  pass
---

Even if it's old news it's worth a refresher because it was a bit of a
headscratcher.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains (a proposal to update the style guidelines for imports)

2014-08-07 Thread Matthew Booth
On 07/08/14 12:15, Matthew Booth wrote:
 I'm sure this is well known, but I recently encountered this problem for
 the second time.
 
 ---
 foo:
 import oslo.config as cfg
 
 import bar
 
 CONF = cfg.CONF
 CONF.register_opts('foo_opt')
 
 ---
 bar:
 import oslo.config as cfg
 
 CONF = cfg.CONF
 
 def bar_func(arg=CONF.foo_opt):
   pass
 ---
 
 importing foo results in an error in bar because CONF.foo_opt doesn't
 exist. This is because bar is imported before CONF.register_opts.
 CONF.import_opt() fails in the same way because it just imports foo and
 hits the exact same problem when foo imports bar.
 
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config. This also allows import_opt()
 to work in bar, which you should do to remove any dependency on import
 order:
 
 ---
 foo:
 import oslo.config as cfg
 
 CONF = cfg.CONF
 CONF.register_opts('foo_opt')
 
 import bar
 
 ---
 bar:
 import oslo.config as cfg
 
 CONF = cfg.CONF
 CONF.import_opt('foo_opt', 'foo')
 
 def bar_func(arg=CONF.foo_opt):
   pass
 ---
 
 Even if it's old news it's worth a refresher because it was a bit of a
 headscratcher.

This became pertinent, because it's now blocking:
https://review.openstack.org/#/c/104145/. It has a -1 for not following
the style guidelines in the ordering of imports.

I think we should update our style guidelines to recommend that any
module which calls register_opts() or import_opt() do so *before*
importing another module which might also call one of those functions.
If this is done consistently, it means that any module can import_opt()
from another module and be certain that it won't be re-imported itself
before register_opts() has been called.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config.

Actually, I disagree.  The real problem here is the definition of
bar_func().  The default value of the parameter arg will likely always
be the default value of foo_opt, rather than the configured value,
because CONF.foo_opt will be evaluated at module load time.  The way
bar_func() should be defined would be:

def bar_func(arg=None):
if not arg:
arg = CONF.foo_opt
…

That ensures that arg will be the configured value, and should also
solve the import conflict.
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matt Riedemann



On 8/7/2014 10:27 AM, Kevin L. Mitchell wrote:

On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:

A (the?) solution is to register_opts() in foo before importing any
modules which might also use oslo.config.


Actually, I disagree.  The real problem here is the definition of
bar_func().  The default value of the parameter arg will likely always
be the default value of foo_opt, rather than the configured value,
because CONF.foo_opt will be evaluated at module load time.  The way
bar_func() should be defined would be:

 def bar_func(arg=None):
 if not arg:
 arg = CONF.foo_opt
 …

That ensures that arg will be the configured value, and should also
solve the import conflict.



Surely you mean:

if arg is not None:

right?! I'm pretty sure there is a hacking check for that now too...

:)

--

Thanks,

Matt Riedemann


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 10:55 -0500, Matt Riedemann wrote:
 
 On 8/7/2014 10:27 AM, Kevin L. Mitchell wrote:
  On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
  A (the?) solution is to register_opts() in foo before importing any
  modules which might also use oslo.config.
 
  Actually, I disagree.  The real problem here is the definition of
  bar_func().  The default value of the parameter arg will likely always
  be the default value of foo_opt, rather than the configured value,
  because CONF.foo_opt will be evaluated at module load time.  The way
  bar_func() should be defined would be:
 
   def bar_func(arg=None):
   if not arg:
   arg = CONF.foo_opt
   …
 
  That ensures that arg will be the configured value, and should also
  solve the import conflict.
 
 
 Surely you mean:
 
 if arg is not None:
 
 right?! I'm pretty sure there is a hacking check for that now too...

No, I meant if not arg, which would be true if arg is None—or 0, or
empty string, or False.  If those alternate false values are potential
values of arg, then clearly an if arg is None would be the correct
incantation.  However, an if arg is not None would never be
appropriate for this logic :)  (And the hacking check is against if arg
== None or if arg != None…)
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matthew Booth
On 07/08/14 16:27, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config.
 
 Actually, I disagree.  The real problem here is the definition of
 bar_func().  The default value of the parameter arg will likely always
 be the default value of foo_opt, rather than the configured value,
 because CONF.foo_opt will be evaluated at module load time.  The way
 bar_func() should be defined would be:
 
 def bar_func(arg=None):
 if not arg:
 arg = CONF.foo_opt
 …
 
 That ensures that arg will be the configured value, and should also
 solve the import conflict.

That's different behaviour, because you can no longer pass arg=None. The
fix isn't to change the behaviour of the code.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Dan Smith
 That's different behaviour, because you can no longer pass arg=None. The
 fix isn't to change the behaviour of the code.

So use a sentinel...

--Dan



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 17:27 +0100, Matthew Booth wrote:
 On 07/08/14 16:27, Kevin L. Mitchell wrote:
  On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
  A (the?) solution is to register_opts() in foo before importing any
  modules which might also use oslo.config.
  
  Actually, I disagree.  The real problem here is the definition of
  bar_func().  The default value of the parameter arg will likely always
  be the default value of foo_opt, rather than the configured value,
  because CONF.foo_opt will be evaluated at module load time.  The way
  bar_func() should be defined would be:
  
  def bar_func(arg=None):
  if not arg:
  arg = CONF.foo_opt
  …
  
  That ensures that arg will be the configured value, and should also
  solve the import conflict.
 
 That's different behaviour, because you can no longer pass arg=None. The
 fix isn't to change the behaviour of the code.

Well, the point is that the code as written is incorrect.  And if 'None'
is an input you want to allow, then use an incantation like:

_unset = object()

def bar_func(arg=_unset):
if arg is _unset:
arg = CONF.foo_opt
…

In any case, the operative point is that CONF.attribute must always be
evaluated inside run-time code, never at module load time.
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matthew Booth
On 07/08/14 17:11, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 10:55 -0500, Matt Riedemann wrote:

 On 8/7/2014 10:27 AM, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config.

 Actually, I disagree.  The real problem here is the definition of
 bar_func().  The default value of the parameter arg will likely always
 be the default value of foo_opt, rather than the configured value,
 because CONF.foo_opt will be evaluated at module load time.  The way
 bar_func() should be defined would be:

  def bar_func(arg=None):
  if not arg:
  arg = CONF.foo_opt
  …

 That ensures that arg will be the configured value, and should also
 solve the import conflict.


 Surely you mean:

 if arg is not None:

 right?! I'm pretty sure there is a hacking check for that now too...
 
 No, I meant if not arg, which would be true if arg is None—or 0, or
 empty string, or False.  If those alternate false values are potential
 values of arg, then clearly an if arg is None would be the correct
 incantation.  However, an if arg is not None would never be
 appropriate for this logic :)  (And the hacking check is against if arg
 == None or if arg != None…)

... or arg is an object which defines __nonzero__(), or defines
__getattr__() and then explodes because of the unexpected lookup of a
__nonzero__ attribute. Or it's False (no quotes when printed by the
debugger), but has a unicode type and therefore evaluates to True[1].

However, if you want to compare a value with None and write 'foo is
None' it will always do exactly what you expect, regardless what you
pass to it. I think it's also nicer to the reviewer and the maintainer,
who then don't need to go looking for context to check if anything
invalid might be passed in.

Matt

[1] I actually hit this. I still don't understand it.
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matthew Booth
On 07/08/14 17:34, Dan Smith wrote:
 That's different behaviour, because you can no longer pass arg=None. The
 fix isn't to change the behaviour of the code.
 
 So use a sentinel...

That would also be a change to the behaviour of the code, because you
can no longer pass in the sentinel.

These are tricky, case-by-case workarounds to a general problem which
can be solved by simply calling register_opts() in a place where it's
guaranteed to be safe. Is there any reason not to call register_opts()
before importing other modules?

Matt

-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Matthew Booth
On 07/08/14 17:39, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 17:27 +0100, Matthew Booth wrote:
 On 07/08/14 16:27, Kevin L. Mitchell wrote:
 On Thu, 2014-08-07 at 12:15 +0100, Matthew Booth wrote:
 A (the?) solution is to register_opts() in foo before importing any
 modules which might also use oslo.config.

 Actually, I disagree.  The real problem here is the definition of
 bar_func().  The default value of the parameter arg will likely always
 be the default value of foo_opt, rather than the configured value,
 because CONF.foo_opt will be evaluated at module load time.  The way
 bar_func() should be defined would be:

 def bar_func(arg=None):
 if not arg:
 arg = CONF.foo_opt
 …

 That ensures that arg will be the configured value, and should also
 solve the import conflict.

 That's different behaviour, because you can no longer pass arg=None. The
 fix isn't to change the behaviour of the code.
 
 Well, the point is that the code as written is incorrect.  And if 'None'
 is an input you want to allow, then use an incantation like:
 
 _unset = object()
 
 def bar_func(arg=_unset):
 if arg is _unset:
 arg = CONF.foo_opt
 …
 
 In any case, the operative point is that CONF.attribute must always be
 evaluated inside run-time code, never at module load time.

...unless you call register_opts() safely, which is what I'm proposing.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 17:44 +0100, Matthew Booth wrote:
 These are tricky, case-by-case workarounds to a general problem which
 can be solved by simply calling register_opts() in a place where it's
 guaranteed to be safe. 

No, THE CODE IS WRONG.  It is evaluating a configuration value at
_module load time_ rather than at run time.  It is wrong for the same
reason that def foo(arg={}) is wrong.  If you fix the time at which
evaluation occurs, importing is no longer a problem.

 Is there any reason not to call register_opts()
 before importing other modules?

-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 17:46 +0100, Matthew Booth wrote:
  In any case, the operative point is that CONF.attribute must
 always be
  evaluated inside run-time code, never at module load time.
 
 ...unless you call register_opts() safely, which is what I'm
 proposing.

No, calling register_opts() at a different point only fixes the import
issue you originally complained about; it does not fix the problem that
the configuration option is evaluated at the wrong time.  The example
code you included in your original email evaluates the configuration
option at module load time, BEFORE the configuration has been loaded,
which means that the argument default will be the default of the
configuration option, rather than the configured value of the
configuration option.  Configuration options must be evaluated at
RUN-TIME, after configuration is loaded; they must not be evaluated at
LOAD-TIME, which is what your original code does.
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [nova][oslo] oslo.config and import chains

2014-08-07 Thread Kevin L. Mitchell
On Thu, 2014-08-07 at 17:41 +0100, Matthew Booth wrote:
 ... or arg is an object which defines __nonzero__(), or defines
 __getattr__() and then explodes because of the unexpected lookup of a
 __nonzero__ attribute. Or it's False (no quotes when printed by the
 debugger), but has a unicode type and therefore evaluates to True[1].

If you're passing such exotic objects as parameters that could
potentially be drawn from configuration instead, maybe that code needs
to be refactored a bit :)

 However, if you want to compare a value with None and write 'foo is
 None' it will always do exactly what you expect, regardless what you
 pass to it. I think it's also nicer to the reviewer and the
 maintainer,
 who then don't need to go looking for context to check if anything
 invalid might be passed in.

In the vast majority of cases, however, we use a value that evaluates to
False to indicate use the default, where default may be drawn from
configuration.  Yes, there are cases where we must treat, say, 0 as
distinct from None, but when we don't need to, we should keep the code
as simple as possible.  After all, I doubt anyone would seriously
suggest that we must always use something like the _unset sentinel,
even when None has no special meaning…
-- 
Kevin L. Mitchell kevin.mitch...@rackspace.com
Rackspace


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev