search

openvstorage / framework

The Framework is a set of components and tools which brings the user an interface (GUI / API) to setup, extend and manage an Open vStorage platform.
Other
27 stars 23 forks source link

Configuration.list does not give result when last character is / #1024

Closed kinvaris closed 7 years ago

kinvaris commented 7 years ago

As seen in example:

In [2]: Configuration.list('ovs/arakoon')
Out[2]: <generator object list at 0x7f1406d45960>

In [3]: for i in Configuration.list('ovs/arakoon'):
   ...:     print i
   ...:     
ovsdb

In [4]: for i in Configuration.list('ovs/arakoon/'):
    print i
   ...:     

In [5]:
wimpers commented 7 years ago

@kinvaris , if you need this can you create a PR which removed the trailing slash

I believe the trick is to replace in https://github.com/openvstorage/framework/blob/d5086cf0fd9c36e0708f45ffacd559549dd407b4/ovs/extensions/generic/configuration.py#L215

return Configuration._list(key) with return Configuration._list(key.rstrip("/"))

We might want to the same for dir_exists(key):

Note, might be that ETCD gives a different output compared to Arakoon.

wimpers commented 7 years ago

Might have impact on Configuration.list('/') we sometimes use.

wimpers commented 7 years ago

Also check if input validation is done on the key.

kvanhijf commented 7 years ago

@wimpers : Perhaps QA should do their job and framework should do their job too ;) Framework: Fix bugs rapported by QA (thus opening PRs) QA: Find bugs introduced by Framework

wimpers commented 7 years ago

@kvanhijf A feel a lot of job protection there? A great 'Make engineering great again'-feeling :p

@kinvaris which config mgmt tool is this Arakoon/ETCD or is this happening on both?

kvanhijf commented 7 years ago

@wimpers : Not so much really

  1. It's more that QA knows a lot less of the possible impact a change can have, which in the end causes more time lost because we have to put time in the reviews or 'holding their hand'. So QA loses time trying to fix it, we lose time telling QA how to fix it and reviewing it
  2. From time to time it can be pleasant for the framework to fix some easier things, a nice diversion of breaking our heads over harder matters
khenderick commented 7 years ago

@wimpers, @kinvaris, @kvanhijf; A lot of these things were implemented to keep the exact same behavior as with Etcd which means that you could not do a list on /ovs/arakoon/ as far as I know. An rstrip could resolve this for the arakoon implementation but might introduce issues in the etcd implementation. There's already a ton of awkward code in the configuration wrapper to keep the two compatible (behave identically)

To be honest, I'd rather just remove Etcd support altogether and make a lot of changes next to this one to make our config management at least a bit more straightforward to use, as this keep-things-compatible-with-etcd is just a pain in the ass.

khenderick commented 7 years ago

Fixed by #1031, packaged in openvstorage-2.7.4-rev.4136.8d34d87

JeffreyDevloo commented 7 years ago

Information

Seeing as the 'fix' both mentions the starting and trailing '/', I will test both for both cases. The fix is only applied to ArakoonConfiguration, no changes have been made to EtcdConfiguration.

Steps

from ovs.extensions.generic.configuration import Configuration

# Seeing as it would be the same call, the order should also be identical
normal_case = []
starting_case = []
trailing_case = []
starting_trailing_case = []

for i in Configuration.list('ovs/arakoon'):
    normal_case.append(i)
for i in Configuration.list('/ovs/arakoon'):
    starting_case.append(i)
for i in Configuration.list('ovs/arakoon'):
    trailing_case.append(i)
for i in Configuration.list('/ovs/arakoon/'):
    starting_trailing_case.append(i)

for index, value in enumerate(normal_case):
    if (starting_case[index] and trailing_case[index] and starting_trailing_case[index]) == value:
        print 'All lists match for {0} on position {1}'.format(value, index)
    else:
        raise ValueError('The test has failed, difference in return values')

Output

All lists match for gwgw-abm on position 0
All lists match for gwgw-nsm_0 on position 1
All lists match for gwgwgwg-abm on position 2
All lists match for gwgwgwg-nsm_0 on position 3
All lists match for gwwwg-abm on position 4
All lists match for gwwwg-nsm_0 on position 5
All lists match for hehehe-abm on position 6
All lists match for hehehe-nsm_0 on position 7
All lists match for mybackend-abm on position 8
All lists match for mybackend-nsm_0 on position 9
All lists match for ovsdb on position 10
All lists match for test-abm on position 11
All lists match for test-nsm_0 on position 12
All lists match for voldrv on position 13

Test result

Test passed.

Packages