CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Blacklist keystonemiddleware 4.19 #923

Closed knikolla closed 6 years ago

knikolla commented 6 years ago

The 4.19 release of keystonemiddleware contains a commit[0] which starts making use of the oslo.cache library and specifically some internal module which interface with memcache. However python-memcache is an optional backend of the library, and therefore not installed by default. Additionally, when keystonemiddleware is not configured for caching, those modules are not needed. So python-memcache can't be a hard dependency of keystonemiddleware, but the code treats it as such. This is tracked at bug [1].

[2] merged which enables lazy loading for oslo.cache, but that won't be included until the next release, so we need to blacklist 4.19.

  1. https://github.com/openstack/keystonemiddleware/commit/9d8e2836fe7fca186e0380d8a532540ff5cc5215
  2. https://bugs.launchpad.net/keystonemiddleware/+bug/1737115
  3. https://github.com/openstack/keystonemiddleware/commit/35fa0e1da1b96a40aa2aa6dd2be378e4155a22ba
zenhack commented 6 years ago

Would you add some comments in setup.py explaining all this?

Thanks for tracking this down

naved001 commented 6 years ago

it's in the commit message, isn't that enough?

zenhack commented 6 years ago

Quoting Naved Ansari (2017-12-14 16:23:59)

it's in the commit message, isn't that enough?

General rule of thumb: commit messages are for people trying to understand the history, while comments are for people to understand the current state of the code.

So in this case, someone viewing the code might ask "Why are we blacklisting 4.19?" The answer doesn't need to involve when we did that.

Additionally, if we change something else about that line, the relevant commit might get tricky to track down, since git-blame won't give you the right commit anymore.

naved001 commented 6 years ago

@zenhack okay, added a short comment.

zenhack commented 6 years ago

Good enough for me, merging.