[openstack-dev] [nova] Questions on pep8 F811 hacking check for microversion

Christopher Yeoh cbkyeoh at gmail.com
Wed Jan 28 08:35:37 UTC 2015


On Tue, 06 Jan 2015 07:31:19 -0500
Jay Pipes <jaypipes at gmail.com> wrote:

> On 01/06/2015 06:25 AM, Chen CH Ji wrote:
> > Based on nova-specs api-microversions.rst
> > we support following function definition format, but it violate the
> > hacking rule pep8 F811 because duplicate function definition
> > we should use #noqa for them , but considering microversion may
> > live for long time ,
> > keep adding #noqa may be a little bit ugly, can anyone suggest a
> > good solution for it ? thanks
> >
> >  >   @api_version(min_version='2.1')
> >  >   def _version_specific_func(self, req, arg1):
> >  >      pass
> >  >
> >  >   @api_version(min_version='2.5')
> >  >   def _version_specific_func(self, req, arg1):
> >  >      pass
> 
> Hey Kevin,
> 
> This was actually one of my reservations about the proposed 
> microversioning implementation -- i.e. having functions that are
> named exactly the same, only decorated with the microversioning
> notation. It kinda reminds me of the hell of debugging C++ code that
> uses STL: how does one easily know which method one is in when inside
> a debugger?
> 
> That said, the only other technique we could try to use would be to
> not use a decorator and instead have a top-level dispatch function
> that would inspect the API microversion (only when the API version
> makes a difference to the output or input of that function) and then
> dispatch the call to a helper method that had the version in its name.
> 
> So, for instance, let's say you are calling the controller's GET 
> /$tenant/os-hosts method, which happens to get routed to the 
> nova.api.openstack.compute.contrib.hosts.HostController.index()
> method. If you wanted to modify the result of that method and the API 
> microversion is at 2.5, you might do something like:
> 
>   def index(self, req):
>       req_api_ver = utils.get_max_requested_api_version(req)
>       if req_api_ver == (2, 5):
>           return self.index_2_5(req)
>       return self.index_2_1(req)
> 
>   def index_2_5(self, req):
>       results = self.index_2_1(req)
>       # Replaces 'host' with 'host_name'
>       for result in results:
>           result['host_name'] = result['host']
>           del result['host']
>       return results
> 
>   def index_2_1(self, req):
>       # Would be a rename of the existing index() method on
>       # the controller....
> 

So having to manually add switching code everything we have an API
patch I think is not only longer and more complicated but more error
prone when updating. If we change something at the core in the future it
means changing all the microversioned code rather than just the
switching architecture at the core of wsgi.


> Another option would be to use something like JSON-patch to determine 
> the difference between two output schemas and automatically translate 
> one to another... but that would be a huge effort.
> 
> That's the only other way I can think of besides disabling F811,
> which I really would not recommend, since it's a valuable safeguard
> against duplicate function names (especially duplicated test methods).

So I don't think we need to disable F811 in general - why not just
disable it for any method with the api_version decorator? On those ones
we can do checks on what is passed to api_version which will help
verify that there hasn't been a typo to an api_version decorator.

Chris



More information about the OpenStack-dev mailing list