)]}'
{"rally/plugins/openstack/services/base.py":[{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"ec6d2e6d7569c9781a7c68a17b119df3a97b8821","unresolved":false,"context_lines":[{"line_number":20,"context_line":"class Service(object):"},{"line_number":21,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone):"},{"line_number":24,"context_line":"        self.clients \u003d clients"},{"line_number":25,"context_line":"        if atomic_inst:"},{"line_number":26,"context_line":"            if not isinstance(atomic_inst, atomic.ActionTimerMixin):"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_7acd7ab6","line":23,"updated":"2016-04-25 15:32:01.000000000","message":"Where is version arguments?","commit_id":"3d73529b1b14618da3b5c2db0965a9741a342448"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"4221e6c295a07e7241fdd50ef68a2397d88fd696","unresolved":false,"context_lines":[{"line_number":20,"context_line":"class Service(object):"},{"line_number":21,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone):"},{"line_number":24,"context_line":"        self.clients \u003d clients"},{"line_number":25,"context_line":"        if atomic_inst:"},{"line_number":26,"context_line":"            if not isinstance(atomic_inst, atomic.ActionTimerMixin):"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_5a04fe7c","line":23,"in_reply_to":"1a122d0e_7acd7ab6","updated":"2016-04-25 15:37:57.000000000","message":"Also, we need to store owner here. It is required to generate_random_name.","commit_id":"3d73529b1b14618da3b5c2db0965a9741a342448"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"ec6d2e6d7569c9781a7c68a17b119df3a97b8821","unresolved":false,"context_lines":[{"line_number":21,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone):"},{"line_number":24,"context_line":"        self.clients \u003d clients"},{"line_number":25,"context_line":"        if atomic_inst:"},{"line_number":26,"context_line":"            if not isinstance(atomic_inst, atomic.ActionTimerMixin):"},{"line_number":27,"context_line":"                raise TypeError(\"atomic_inst must have ActionTimerMixin \""}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_9a8216ff","line":24,"updated":"2016-04-25 15:32:01.000000000","message":"Each service implementation need to do something like below?\n\n   if version \u003d\u003d \"2\":\n       self.impl \u003d SomeServiceV2\n   elif version \u003d\u003d \"3\":\n       self.impl \u003d SomeServiceV3\n\nLet\u0027s simplify it a bit using mapping\n\n   class Service(object):\n\n        _VERSION_MAPPING \u003d {}\n\n        def __init__(self, clients, version, atomic_inst\u003dNone):\n            # do the same as in your class:)\n            if version and version in self._VERSION_MAPPING:\n                self._impl \u003d self._VERSION_MAPPING[version]\n            else:\n                self._impl \u003d self.discover_version()\n            if not self._impl:\n                raise RallyException(\"There is no implementation...blah blah blah\")\n\n        def discover_version(self):\n             # reserved for those services which support it\n             return None\n    \n    class Identity(Service):\n        \n        _VERSION_MAPPING \u003d {\"2\": KeystoneV2, \"3\": KeystoneV3}\n        \n        def discover_version(self):\n            # do something","commit_id":"3d73529b1b14618da3b5c2db0965a9741a342448"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":28,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone):"},{"line_number":29,"context_line":"        self.clients \u003d clients"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        # NOTE(amaretskiy): atomic_inst is actually an `owner\u0027 object, which"},{"line_number":32,"context_line":"        #                   is also a subclass of RandomNameGeneratorMixin."},{"line_number":33,"context_line":"        #                   We need this resources owner for random names"},{"line_number":34,"context_line":"        #                   generation - see network wrapper as example"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_36091b85","line":31,"updated":"2016-04-27 23:28:26.000000000","message":"First of all I do not understand what means \"owner\" in this case. \n\nIt is not owner, it is just a instance that is used to store \"atomic_actions\" information. \n\nAnd I really do not understand where is the problem?","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":53,"context_line":""},{"line_number":54,"context_line":"        if version:"},{"line_number":55,"context_line":"            if version \u003d\u003d \"2\":"},{"line_number":56,"context_line":"                # NOTE(amaretskiy): PROBLEM!"},{"line_number":57,"context_line":"                #   1. KeystoneV2Service is subclass of"},{"line_number":58,"context_line":"                #      IdentityService, so we have infinity"},{"line_number":59,"context_line":"                #      recursion here!!!"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_568777b4","line":56,"updated":"2016-04-27 23:28:26.000000000","message":"All comments above are very simple to address, just put on 53 line this: \n\n  if  type(self) \u003d\u003d IdentityService: \n     return\n\nIf you are creating directly KeystoneV2Service it won\u0027t run this: \n\n  1) There won\u0027t be recursion\n\n  2) There won\u0027t be problems with overriding KeystoneV2Service, you\u0027ll be able safely to call super(KeystoneV2Service, self).__init__(...)\n  \n3) I don\u0027t think that multple references are problem here (really it\u0027s not a problem, there are many places such in code). Plus in case of creating childrnes of IdentityService you won\u0027t have \"self.impl\" at all","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def project_create(self, name, **kwargs):"},{"line_number":77,"context_line":"        # NOTE(amaretskiy): How does self.impl will use clients?"},{"line_number":78,"context_line":"        #   I. If it uses self.impl.clients - this duplicates self.clients"},{"line_number":79,"context_line":"        #      by reference and looks like overcomplicated code"},{"line_number":80,"context_line":"        #   II. If we decide to pass clients as argument - this would look"},{"line_number":81,"context_line":"        #      overcomplicated too"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_d1d8b155","line":78,"updated":"2016-04-27 23:28:26.000000000","message":"Sorry, but this doesn\u0027t seems like a valid comment: \n\n\n  For example, you don\u0027t need to use clients in  IdentityService.project_create but if you need to use it in any case for some reason it will look very simple: \n\n  IdentityService.project_create(self, name, **kwargs): \n       self.clients._do_some_stuff()  # do some stuff with clients\n \n  ---\n  \n  If you are thinking about KeystoneV2Servivce.project_create you will use in the same way self.clients, which is very nice! from UX perspective.\n\n  @Atomic.ActionTimer(\"\")  \n  KeystoneV2Service.project_create(self, name, **kwargs): \n        self.clients.do_some_v2_specific_stuff()\n\n\n\nSo I complete don\u0027t understand what is the problem here, it looks really cool and it works well","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":87,"context_line":"# NOTE(amaretskiy): Since this is an IdentityService subclass,"},{"line_number":88,"context_line":"#                   it is potentially possible to have this:"},{"line_number":89,"context_line":"#"},{"line_number":90,"context_line":"#                     svc_v2 \u003d KeystoneV2Service(clients, version\u003d\"3\")"},{"line_number":91,"context_line":"#                     svc_v2.version \u003d\u003d \"3\"  # True"},{"line_number":92,"context_line":"class KeystoneV2Service(IdentityService):"},{"line_number":93,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_71cf1d7f","line":90,"updated":"2016-04-27 23:28:26.000000000","message":"It\u0027s is python, you can write raise Exception(\"Something\") and that code won\u0027t work as well. \n\nIn case of Services you should override __init__() method and that is okay. \n\n  def __init__(clients, atomic_inst\u003datomic_inst): \n    super(....)\n\nWhy it\u0027s problem?","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"f7ae5052e0b51ea60fb58ebbf12361ba879c95e2","unresolved":false,"context_lines":[{"line_number":110,"context_line":"#                   problems mentioned above:"},{"line_number":111,"context_line":"#"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"@plugin.configure(name\u003d\"base_service\")"},{"line_number":114,"context_line":"class ServicePlugin(plugin.Plugin):"},{"line_number":115,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_6f2d9f45","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":38},"updated":"2016-04-26 17:14:46.000000000","message":"1) why we need to mark base class as a plugin? It is not plugin, it is just a parent class for further plugins. Please, stop doing it:)\n\n2) Services were designed as alternative for utils and wrappers (for help methods). Not sure that they shoulbe be pluggable","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"e587ed8b7ef6d12b8b029c35f791cbc727ea9b1d","unresolved":false,"context_lines":[{"line_number":110,"context_line":"#                   problems mentioned above:"},{"line_number":111,"context_line":"#"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"@plugin.configure(name\u003d\"base_service\")"},{"line_number":114,"context_line":"class ServicePlugin(plugin.Plugin):"},{"line_number":115,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_b0c1cae6","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":38},"in_reply_to":"1a122d0e_6f2d9f45","updated":"2016-04-27 07:56:46.000000000","message":"No problem. This can be not plugin-based","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":110,"context_line":"#                   problems mentioned above:"},{"line_number":111,"context_line":"#"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"@plugin.configure(name\u003d\"base_service\")"},{"line_number":114,"context_line":"class ServicePlugin(plugin.Plugin):"},{"line_number":115,"context_line":"    \"\"\"Base class for all OpenStack services implementations.\"\"\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_91c2b951","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":38},"in_reply_to":"1a122d0e_b0c1cae6","updated":"2016-04-27 23:28:26.000000000","message":"Services should not be a plugins. \n\nThey are not users related things (you don\u0027t use them in input task)","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":116,"context_line":""},{"line_number":117,"context_line":"    VERSION \u003d None"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self, clients, version\u003dNone, owner\u003dNone, **kwargs):"},{"line_number":120,"context_line":"        self._clients \u003d clients"},{"line_number":121,"context_line":"        self._version \u003d version"},{"line_number":122,"context_line":"        self._owner \u003d owner"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_11ad099b","line":119,"updated":"2016-04-27 23:28:26.000000000","message":"what means owner? this is absolutelly unclear word please do not use it","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def __init__(self, clients, version\u003dNone, owner\u003dNone, **kwargs):"},{"line_number":120,"context_line":"        self._clients \u003d clients"},{"line_number":121,"context_line":"        self._version \u003d version"},{"line_number":122,"context_line":"        self._owner \u003d owner"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"        if self._owner and isinstance(self._owner, atomic.ActionTimerMixin):"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_d1943138","line":121,"updated":"2016-04-27 23:28:26.000000000","message":"why we need to store version at all? it\u0027s just used for intialzation.","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":119,"context_line":"    def __init__(self, clients, version\u003dNone, owner\u003dNone, **kwargs):"},{"line_number":120,"context_line":"        self._clients \u003d clients"},{"line_number":121,"context_line":"        self._version \u003d version"},{"line_number":122,"context_line":"        self._owner \u003d owner"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"        if self._owner and isinstance(self._owner, atomic.ActionTimerMixin):"},{"line_number":125,"context_line":"            # NOTE(amaretskiy): There is a modification by reference, which is"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_71cb9d53","line":122,"updated":"2016-04-27 23:28:26.000000000","message":"it is not owner, it may be a just instanst of atomic_action, because it is used only to make this class look like an atomic action subclass.","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"f7ae5052e0b51ea60fb58ebbf12361ba879c95e2","unresolved":false,"context_lines":[{"line_number":126,"context_line":"            #     not good. Maybe we could use explicit modification in future"},{"line_number":127,"context_line":"            self._atomic_actions \u003d self._owner._atomic_actions"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            self._atomic_actions \u003d collections.OrderedDict()"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    @property"},{"line_number":132,"context_line":"    def clients(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_2fc60768","line":129,"updated":"2016-04-26 17:14:46.000000000","message":"It doesn\u0027t solve the issue with initialization. Service class is not just a base class for different API version, it should be a thing that solves compatibility between different API versions. That is why it should have self.impl.\n\nImo, we can use some check here. For example something like:\n\n    if self.VERSION is None:\n        # assume that it is initialization for base class\n        self._impl \u003d self.discover(version)\n    else:\n        # assume that it is initialization of partial API class\n        pass\n\nIn this case, \"version\" argument of __init__ methods should be only available for base class. For APIs it should be restricted.","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"e587ed8b7ef6d12b8b029c35f791cbc727ea9b1d","unresolved":false,"context_lines":[{"line_number":126,"context_line":"            #     not good. Maybe we could use explicit modification in future"},{"line_number":127,"context_line":"            self._atomic_actions \u003d self._owner._atomic_actions"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            self._atomic_actions \u003d collections.OrderedDict()"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    @property"},{"line_number":132,"context_line":"    def clients(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_90a76e4f","line":129,"in_reply_to":"1a122d0e_2fc60768","updated":"2016-04-27 07:56:46.000000000","message":"\"self._impl \u003d \u003cimplementation_instance\u003e\" looks like a problem in case when self._impl.__class__ is a subclass of self.__class__\n\n\nFor better demonstration - just try to create whole base class and try to use it somehow","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"f7ae5052e0b51ea60fb58ebbf12361ba879c95e2","unresolved":false,"context_lines":[{"line_number":134,"context_line":""},{"line_number":135,"context_line":"    @property"},{"line_number":136,"context_line":"    def version(self):"},{"line_number":137,"context_line":"        return self._version"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _generate_random_name(self):"},{"line_number":140,"context_line":"        if self._owner and isinstance(self._owner,"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_0f718b44","line":137,"updated":"2016-04-26 17:14:46.000000000","message":"self.VERSION or self.__version","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"e587ed8b7ef6d12b8b029c35f791cbc727ea9b1d","unresolved":false,"context_lines":[{"line_number":134,"context_line":""},{"line_number":135,"context_line":"    @property"},{"line_number":136,"context_line":"    def version(self):"},{"line_number":137,"context_line":"        return self._version"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _generate_random_name(self):"},{"line_number":140,"context_line":"        if self._owner and isinstance(self._owner,"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_708d72d2","line":137,"in_reply_to":"1a122d0e_0f718b44","updated":"2016-04-27 07:56:46.000000000","message":"Here is actually a mistake - in my variant this method can be removed at all (we already have self.VERSION)","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":136,"context_line":"    def version(self):"},{"line_number":137,"context_line":"        return self._version"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _generate_random_name(self):"},{"line_number":140,"context_line":"        if self._owner and isinstance(self._owner,"},{"line_number":141,"context_line":"                                      utils.RandomNameGeneratorMixin):"},{"line_number":142,"context_line":"            return self._owner.generate_random_name()"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_b17415fe","line":139,"updated":"2016-04-27 23:28:26.000000000","message":"I don\u0027t like this at all. \n\nFirst of all","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"1bc931d8dc1583d82a26ec87cc39d3f8de6ab237","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                        \"have RandomNameGeneratorMixin in its bases\")"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    @classmethod"},{"line_number":147,"context_line":"    def discover(cls, version, clients, owner, **kwargs):"},{"line_number":148,"context_line":"        \"\"\"Discover service implementation."},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"        :param version: str ot numeric version identifier"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_b19c156b","line":147,"updated":"2016-04-27 23:28:26.000000000","message":"We don\u0027t need this method.","commit_id":"2731fc6ee25701fcf9c2261ff5fb5eca706266af"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"9b76d1a1dc4128e6e8c9c154cc224c3bfbab28eb","unresolved":false,"context_lines":[{"line_number":52,"context_line":"            # assume that it is specific API class initialization"},{"line_number":53,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":54,"context_line":"            if not isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":55,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":56,"context_line":"            else:"},{"line_number":57,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":58,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a122d0e_143dedc3","line":55,"updated":"2016-04-28 10:48:50.000000000","message":"AttributeError. `parent\u0027 is a class, not instance, so parent does not have _atomic_actions yet","commit_id":"5a861e6ef2d93c033bb22d286669bcb4e9438008"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"7a6c4592e46d8daec57687f35b48c339f251d7f9","unresolved":false,"context_lines":[{"line_number":52,"context_line":"            # assume that it is specific API class initialization"},{"line_number":53,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":54,"context_line":"            if not isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":55,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":56,"context_line":"            else:"},{"line_number":57,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":58,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a122d0e_579e4763","line":55,"in_reply_to":"1a122d0e_143dedc3","updated":"2016-04-28 11:48:52.000000000","message":"It looks like it should be an instance. Should fix docstring and L46","commit_id":"5a861e6ef2d93c033bb22d286669bcb4e9438008"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"9b76d1a1dc4128e6e8c9c154cc224c3bfbab28eb","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        return self._osclient()"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    def generate_random_name(self):"},{"line_number":99,"context_line":"        return self._parent.generate_random_name()"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a122d0e_944f3d6a","line":99,"updated":"2016-04-28 10:48:50.000000000","message":"TypeError. `parent\u0027 is a class","commit_id":"5a861e6ef2d93c033bb22d286669bcb4e9438008"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return func"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"def proxy_to_impl(method_name):"},{"line_number":30,"context_line":"    \"\"\"Create a method to proxy call to implementation of specific version"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    New method do not change neither position nor keyword arguments, just"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_163471f9","line":29,"updated":"2016-05-02 22:55:44.000000000","message":"Why you create proxy method which do nothing is the mystery for me","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return func"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"def proxy_to_impl(method_name):"},{"line_number":30,"context_line":"    \"\"\"Create a method to proxy call to implementation of specific version"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    New method do not change neither position nor keyword arguments, just"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_b8b91f7c","line":29,"in_reply_to":"1a122d0e_163471f9","updated":"2016-05-03 09:48:42.000000000","message":"If you look at Keystone implementation, you can find that there are a lot of methods like:\n\n    def do_something(self): self._impl.do_something()\n\nThis decorator was design to reduce number of lines for such 2 lines methods.\n\nIn new patch-set it was removed since it hides the interface of base class","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":54,"context_line":"            # assume that it is implementation of specific version of API which"},{"line_number":55,"context_line":"            # should cover all public methods of Service base class."},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            # find base service class"},{"line_number":58,"context_line":"            mro \u003d cls.mro()"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"            base \u003d [c for c in mro if getattr(c, \"_CLIENT_NAME\", None)]"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_b18799b5","line":57,"updated":"2016-05-02 22:55:44.000000000","message":"Why we are looking for it?","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":54,"context_line":"            # assume that it is implementation of specific version of API which"},{"line_number":55,"context_line":"            # should cover all public methods of Service base class."},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"            # find base service class"},{"line_number":58,"context_line":"            mro \u003d cls.mro()"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"            base \u003d [c for c in mro if getattr(c, \"_CLIENT_NAME\", None)]"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_0391c852","line":57,"in_reply_to":"1a122d0e_b18799b5","updated":"2016-05-03 09:48:42.000000000","message":"please, read L45-46. \nService class was designed to replace wrappers and utils. In case of wrappers, we have several API versions for one service. Base service class should provide a common interface for all possible version implementation. Since logic of methods for such interface looks like \"make a call to version implementation, process the results if needed\", we need to ensure that all version implementations have designed all public methods of base class.","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":150,"context_line":"    \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_23f1716b","line":153,"updated":"2016-05-02 22:55:44.000000000","message":"As I understand we will not get more then one client in self.clients I can understand the case when there will be more information then for one service. Because this will be strange if instance of service will store information about not related client, but there should be only one that match the version. Thus we don\u0027t need this variable to get the name and the version","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":150,"context_line":"    \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_e333ec60","line":153,"in_reply_to":"1a122d0e_23f1716b","updated":"2016-05-03 09:48:42.000000000","message":"As I described the purpose of service classes at L57, service classes will replace all utils. In this case, some of help methods, can use not only one client, so __init__ method accepts aggregation of all clients - the instance of  osclients.Clients. And to determine the main client, _CLIENT_NAME is used.","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_36240db1","line":155,"updated":"2016-05-02 22:55:44.000000000","message":"I don\u0027t see where we pass atomic_inst to service instance init. Why it was removed?","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_83e038ce","line":155,"in_reply_to":"1a122d0e_36240db1","updated":"2016-05-03 09:48:42.000000000","message":"Unfortunately, our spec did not cover all cases, so some changes were applied while implementation. `atomic_inst` is a property of parent/caller class(for example Scenario). Since we need generate_random_name method, which is also related to parent class, we can reduce number of input arguments and transmit them separately via instance of parent class.","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"},{"line_number":159,"context_line":"        :param parent: an instance of class(for example Scenario, Context)"},{"line_number":160,"context_line":"            which inherits from rally.common.utils.RandomNameGeneratorMixin"},{"line_number":161,"context_line":"            and, optional, from rally.task.atomic.ActionTimerMixin"},{"line_number":162,"context_line":"        :param kwargs: an optional arguments. Accepts:"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_e3013908","line":159,"updated":"2016-05-02 22:55:44.000000000","message":"Sorry but I don\u0027t understand why we need this `parent` What problem raised and how it will solve it?","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"},{"line_number":159,"context_line":"        :param parent: an instance of class(for example Scenario, Context)"},{"line_number":160,"context_line":"            which inherits from rally.common.utils.RandomNameGeneratorMixin"},{"line_number":161,"context_line":"            and, optional, from rally.task.atomic.ActionTimerMixin"},{"line_number":162,"context_line":"        :param kwargs: an optional arguments. Accepts:"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_c3387040","line":159,"in_reply_to":"1a122d0e_e3013908","updated":"2016-05-03 09:48:42.000000000","message":"see my comment for L155.","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"b15d210031ef169a15d690043fc9b1a0f11fc262","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"    @commonmethod"},{"line_number":242,"context_line":"    def generate_random_name(self):"},{"line_number":243,"context_line":"        if not self._parent:"},{"line_number":244,"context_line":"            raise exceptions.RallyException("},{"line_number":245,"context_line":"                \"You cannot use `generate_random_name` method without parent \""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_993c32c1","line":242,"updated":"2016-05-02 22:55:44.000000000","message":"Why we have generate_random_name magic in base of service for what its needed, here its look redundant","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c9e8147f60fb3a364cb0d798a69161592030f3c7","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"    @commonmethod"},{"line_number":242,"context_line":"    def generate_random_name(self):"},{"line_number":243,"context_line":"        if not self._parent:"},{"line_number":244,"context_line":"            raise exceptions.RallyException("},{"line_number":245,"context_line":"                \"You cannot use `generate_random_name` method without parent \""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a122d0e_2353246e","line":242,"in_reply_to":"1a122d0e_993c32c1","updated":"2016-05-03 09:48:42.000000000","message":"First of all, why is it magic for you?\n\nAs I said in previous comments, service classes will be used instead of utils. If you look there, we generate names for a lot of resources, so service classes should be able to do the same. Since service class can be initialized without parent class, we need check that AtrributeError will be not thrown. To reduce duplication of code, this check was moved to base class.","commit_id":"07d95794f847563850636d5e78116005b227b906"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"31d85c7dbe8238d4902d6386753210c6550771ca","unresolved":false,"context_lines":[{"line_number":173,"context_line":""},{"line_number":174,"context_line":"        if parent and not isinstance(parent, utils.RandomNameGeneratorMixin):"},{"line_number":175,"context_line":"            raise TypeError(\"Object in `parent` argument is not instance of \""},{"line_number":176,"context_line":"                            \"`RandomNameGeneratorMixin`.\")"},{"line_number":177,"context_line":"        self._parent \u003d parent"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":"        if self.VERSION:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a122d0e_0c4587b2","line":176,"updated":"2016-05-02 21:01:55.000000000","message":"This seems like an odd requirement and an odd place for it, too. I don\u0027t see an easy and straightforward way to get around it right now, but if you have to resubmit this patchset please add a TODO to find a more elegant way to deal with random name generation in Service classes.","commit_id":"0b2adb983587fbcb5351b9740421c64446fbdee4"},{"author":{"_account_id":11748,"name":"Chris St. Pierre","email":"cstpierr@cisco.com","username":"stpierre"},"change_message_id":"31d85c7dbe8238d4902d6386753210c6550771ca","unresolved":false,"context_lines":[{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"},{"line_number":189,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        if kwargs:"},{"line_number":192,"context_line":"            raise TypeError(\"Restricted argument(s) in kwargs for %(cls)s \""},{"line_number":193,"context_line":"                            \"detected: %(args)s\" %"},{"line_number":194,"context_line":"                            {\"cls\": self.__class__.__name__,"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a122d0e_4c7c9feb","line":191,"updated":"2016-05-02 21:01:55.000000000","message":"Why don\u0027t we just do:\n\n    def __init__(clients, parent\u003dNone, compatibility_mode\u003dFalse, version\u003dNone)\n\n?","commit_id":"0b2adb983587fbcb5351b9740421c64446fbdee4"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"73cf0b74ad9057fabc2db13b8825ab4169916d0f","unresolved":false,"context_lines":[{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"},{"line_number":189,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        if kwargs:"},{"line_number":192,"context_line":"            raise TypeError(\"Restricted argument(s) in kwargs for %(cls)s \""},{"line_number":193,"context_line":"                            \"detected: %(args)s\" %"},{"line_number":194,"context_line":"                            {\"cls\": self.__class__.__name__,"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a122d0e_787ca7eb","line":191,"in_reply_to":"1a122d0e_4c7c9feb","updated":"2016-05-03 09:08:39.000000000","message":"because compatibility_mode relates only to versioned implementation while version relates to base class.","commit_id":"0b2adb983587fbcb5351b9740421c64446fbdee4"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":40,"context_line":"    def __new__(cls, name, parents, dct):"},{"line_number":41,"context_line":"        # restrict usage of more than 1 positional argument to avoid issues"},{"line_number":42,"context_line":"        # like https://review.openstack.org/#/c/309470/"},{"line_number":43,"context_line":"        positional_wrapper \u003d positional.positional.method(1)"},{"line_number":44,"context_line":"        for field in dct:"},{"line_number":45,"context_line":"            if not field.startswith(\"_\") and callable(dct[field]):"},{"line_number":46,"context_line":"                dct[field] \u003d positional_wrapper(dct[field])"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_b856e35b","line":43,"updated":"2016-05-05 06:20:05.000000000","message":"Can you provide a comment here, how exactly this works, so newbies won\u0027t need to go and read how positional works. \n\nas well instead a link to the code review it\u0027s better to provide just sample","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":40,"context_line":"    def __new__(cls, name, parents, dct):"},{"line_number":41,"context_line":"        # restrict usage of more than 1 positional argument to avoid issues"},{"line_number":42,"context_line":"        # like https://review.openstack.org/#/c/309470/"},{"line_number":43,"context_line":"        positional_wrapper \u003d positional.positional.method(1)"},{"line_number":44,"context_line":"        for field in dct:"},{"line_number":45,"context_line":"            if not field.startswith(\"_\") and callable(dct[field]):"},{"line_number":46,"context_line":"                dct[field] \u003d positional_wrapper(dct[field])"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_0e044815","line":43,"in_reply_to":"1a122d0e_b856e35b","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":50,"context_line":"        super(ServiceMeta, cls).__init__(name, bases, namespaces)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        if namespaces.get(\"VERSION\"):"},{"line_number":53,"context_line":"            # assume that it is implementation of specific version of API which"},{"line_number":54,"context_line":"            # should cover all public methods of Service base class."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"            # find base service class"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_78956be2","line":53,"updated":"2016-05-05 06:20:05.000000000","message":"I don\u0027t like idea of putting version check here is a good idea.\n\nThis will work if you have \"compute_service\" that can have different versions, however if it is \"netowrk_service\" we have 2 different projects neutron and nova network that are using complete different clients and complete different API, more over those services can have different versions.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":56,"context_line":"            # find base service class"},{"line_number":57,"context_line":"            mro \u003d cls.mro()[1:]"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            base \u003d [c for c in mro if getattr(c, \"_CLIENT_NAME\", None)]"},{"line_number":60,"context_line":"            if not base:"},{"line_number":61,"context_line":"                raise TypeError("},{"line_number":62,"context_line":"                    \"%s has wrong implementation. NOTE: Implementation of \""}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_7863ab1a","line":59,"updated":"2016-05-05 06:20:05.000000000","message":"the same question as above, what if client_name is different in different implementations of common API?","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        import collections"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        Project \u003d collections.namedtuple(\"Project\", [\"id\", \"name\"])"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        class Identity(Service):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_d8747fd0","line":95,"updated":"2016-05-05 06:20:05.000000000","message":"where this is used?","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        import collections"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        Project \u003d collections.namedtuple(\"Project\", [\"id\", \"name\"])"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"        class Identity(Service):"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_025394e2","line":95,"in_reply_to":"1a122d0e_d8747fd0","updated":"2016-05-07 19:15:33.000000000","message":"It is used just to provide a single object(without extra fields) as as a result of execution of different version implementations.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_983cc7e2","line":155,"updated":"2016-05-05 06:20:05.000000000","message":"\"parent\" hurts my brain, it\u0027s better to call it like Alex called owner, or it is even better to pass name_generator \u0026 atomic_inst here. \n\nI am not sure about **kwargs, imho we should be strict here as hell and have very specific arguments.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":152,"context_line":"    VERSION \u003d None"},{"line_number":153,"context_line":"    _CLIENT_NAME \u003d None"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def __init__(self, clients, parent\u003dNone, **kwargs):"},{"line_number":156,"context_line":"        \"\"\"Initialize service class"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_892b1230","line":155,"in_reply_to":"1a122d0e_983cc7e2","updated":"2016-05-07 19:15:33.000000000","message":"\u003e\"parent\" hurts my brain, it\u0027s better to call it like Alex called owner, or it is even better to pass name_generator \u0026 atomic_inst here. \n\nfixed\n\n\u003eI am not sure about **kwargs, imho we should be strict here as hell and have very specific arguments.\n\nIt is strict enough. See L191. But it looks like we have only one optional argument now(see my answer to comment at L163), so we can abandon of kwargs usage.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":160,"context_line":"            which inherits from rally.common.utils.RandomNameGeneratorMixin"},{"line_number":161,"context_line":"            and, optional, from rally.task.atomic.ActionTimerMixin"},{"line_number":162,"context_line":"        :param kwargs: an optional arguments. Accepts:"},{"line_number":163,"context_line":"            - version: a version of API;"},{"line_number":164,"context_line":"                Optional parameter for specific service base class. If None -"},{"line_number":165,"context_line":"                default version will be chosen is such case."},{"line_number":166,"context_line":"                Restricted for specific API implementation"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_1b2ef18b","line":163,"updated":"2016-05-05 06:20:05.000000000","message":"Seems like we need to pass 2 things, service_name and service_version or just service\u003d{\"name\": , \"version\"}, otherwise network service will look very ugly","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":160,"context_line":"            which inherits from rally.common.utils.RandomNameGeneratorMixin"},{"line_number":161,"context_line":"            and, optional, from rally.task.atomic.ActionTimerMixin"},{"line_number":162,"context_line":"        :param kwargs: an optional arguments. Accepts:"},{"line_number":163,"context_line":"            - version: a version of API;"},{"line_number":164,"context_line":"                Optional parameter for specific service base class. If None -"},{"line_number":165,"context_line":"                default version will be chosen is such case."},{"line_number":166,"context_line":"                Restricted for specific API implementation"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_692cee4a","line":163,"in_reply_to":"1a122d0e_1b2ef18b","updated":"2016-05-07 19:15:33.000000000","message":"Seems like we d not need this argument at all. If user wants to initialize a specific version, he can initialize needed service implementation(i.e NeutronService, KeystoneV3Service..)","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        if self.VERSION:"},{"line_number":180,"context_line":"            # assume that it is one of versioned implementation of the service"},{"line_number":181,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_3b000dab","line":182,"updated":"2016-05-05 06:20:05.000000000","message":"This creates a recursion in my brain, and it is really not clear why we need to run atomic_actions if there is some magic self.VERSION","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        if self.VERSION:"},{"line_number":180,"context_line":"            # assume that it is one of versioned implementation of the service"},{"line_number":181,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_09d9e221","line":182,"in_reply_to":"1a122d0e_3b000dab","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            # assume that it is one of versioned implementation of the service"},{"line_number":181,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":186,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_db1d9910","line":183,"updated":"2016-05-05 06:20:05.000000000","message":"let\u0027s just not use \"__\" it always creates more pain than it helps","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            # assume that it is one of versioned implementation of the service"},{"line_number":181,"context_line":"            self._compatibility_mode \u003d kwargs.pop(\"compatibility_mode\", False)"},{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":186,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_698fce23","line":183,"in_reply_to":"1a122d0e_db1d9910","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":186,"context_line":"        else:"},{"line_number":187,"context_line":"            # assume that it is a specific service base class initialization"},{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_5bbfc9cf","line":185,"updated":"2016-05-05 06:20:05.000000000","message":"actually it will be better to create atoimic.ActionTimer()._atomic_actions here","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":182,"context_line":"            if parent and isinstance(parent, atomic.ActionTimerMixin):"},{"line_number":183,"context_line":"                self.__atomic_actions \u003d parent._atomic_actions"},{"line_number":184,"context_line":"            else:"},{"line_number":185,"context_line":"                self.__atomic_actions \u003d collections.OrderedDict()"},{"line_number":186,"context_line":"        else:"},{"line_number":187,"context_line":"            # assume that it is a specific service base class initialization"},{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_e9d1fefc","line":185,"in_reply_to":"1a122d0e_5bbfc9cf","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"},{"line_number":189,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        if kwargs:"},{"line_number":192,"context_line":"            raise TypeError(\"Restricted argument(s) in kwargs for %(cls)s \""},{"line_number":193,"context_line":"                            \"detected: %(args)s\" %"},{"line_number":194,"context_line":"                            {\"cls\": self.__class__.__name__,"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_7ba1e5a8","line":191,"updated":"2016-05-05 06:20:05.000000000","message":"requires some comments... not sure why we are allowing to pass kwargs if they are restricted in any case seems like a bad design.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":188,"context_line":"            self.VERSION \u003d self._process_version(kwargs.pop(\"version\", None))"},{"line_number":189,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        if kwargs:"},{"line_number":192,"context_line":"            raise TypeError(\"Restricted argument(s) in kwargs for %(cls)s \""},{"line_number":193,"context_line":"                            \"detected: %(args)s\" %"},{"line_number":194,"context_line":"                            {\"cls\": self.__class__.__name__,"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_6904ee7d","line":191,"in_reply_to":"1a122d0e_7ba1e5a8","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    @property"},{"line_number":198,"context_line":"    def _atomic_actions(self):"},{"line_number":199,"context_line":"        # share one object atomic_actions across versioned implementaion and"},{"line_number":200,"context_line":"        # service base class"},{"line_number":201,"context_line":"        if hasattr(self, \"_impl\"):"},{"line_number":202,"context_line":"            return self._impl._atomic_actions"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_5b8d0930","line":199,"updated":"2016-05-05 06:20:05.000000000","message":"Actually atomic actions should not be used in base class (in most cases) because we don\u0027t supported nested atomic actions yet.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    @property"},{"line_number":198,"context_line":"    def _atomic_actions(self):"},{"line_number":199,"context_line":"        # share one object atomic_actions across versioned implementaion and"},{"line_number":200,"context_line":"        # service base class"},{"line_number":201,"context_line":"        if hasattr(self, \"_impl\"):"},{"line_number":202,"context_line":"            return self._impl._atomic_actions"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_09f0c28f","line":199,"in_reply_to":"1a122d0e_5b8d0930","updated":"2016-05-07 19:15:33.000000000","message":"It can be displayed not as nested thing now.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":197,"context_line":"    @property"},{"line_number":198,"context_line":"    def _atomic_actions(self):"},{"line_number":199,"context_line":"        # share one object atomic_actions across versioned implementaion and"},{"line_number":200,"context_line":"        # service base class"},{"line_number":201,"context_line":"        if hasattr(self, \"_impl\"):"},{"line_number":202,"context_line":"            return self._impl._atomic_actions"},{"line_number":203,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_fb7cd521","line":200,"updated":"2016-05-05 06:20:05.000000000","message":"Let\u0027s call it: \n\ncommon service class\n\nbecause \"base\" is unclear for me","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":197,"context_line":"    @property"},{"line_number":198,"context_line":"    def _atomic_actions(self):"},{"line_number":199,"context_line":"        # share one object atomic_actions across versioned implementaion and"},{"line_number":200,"context_line":"        # service base class"},{"line_number":201,"context_line":"        if hasattr(self, \"_impl\"):"},{"line_number":202,"context_line":"            return self._impl._atomic_actions"},{"line_number":203,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_898832e9","line":200,"in_reply_to":"1a122d0e_fb7cd521","updated":"2016-05-07 19:15:33.000000000","message":"Done","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":203,"context_line":"        else:"},{"line_number":204,"context_line":"            return self.__atomic_actions"},{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    def _process_version(self, version):"},{"line_number":207,"context_line":"        # Some services implement specific way to obtain default version, we"},{"line_number":208,"context_line":"        # need to provide way to do it."},{"line_number":209,"context_line":"        if version is None:"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_7bee058c","line":206,"updated":"2016-05-05 06:20:05.000000000","message":"Is this specific for each client?","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":203,"context_line":"        else:"},{"line_number":204,"context_line":"            return self.__atomic_actions"},{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    def _process_version(self, version):"},{"line_number":207,"context_line":"        # Some services implement specific way to obtain default version, we"},{"line_number":208,"context_line":"        # need to provide way to do it."},{"line_number":209,"context_line":"        if version is None:"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_c96a3a1a","line":206,"in_reply_to":"1a122d0e_7bee058c","updated":"2016-05-07 19:15:33.000000000","message":"it is specific for only one client - keystone:D","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"e83a7b8788ce7d41d343eb2f5a404d67f44660da","unresolved":false,"context_lines":[{"line_number":203,"context_line":"        else:"},{"line_number":204,"context_line":"            return self.__atomic_actions"},{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    def _process_version(self, version):"},{"line_number":207,"context_line":"        # Some services implement specific way to obtain default version, we"},{"line_number":208,"context_line":"        # need to provide way to do it."},{"line_number":209,"context_line":"        if version is None:"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_dd0833dc","line":206,"in_reply_to":"dab17558_c96a3a1a","updated":"2016-05-08 00:12:29.000000000","message":"It is specific for now for only one client.. \n\nSomeday we want to make this mechanism generic (when we introduce non openstack projects)","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":213,"context_line":"                raise ValueError("},{"line_number":214,"context_line":"                    \"Failed to process version for %s service, since \""},{"line_number":215,"context_line":"                    \"\u0027version\u0027 argument is not specified and there is no \""},{"line_number":216,"context_line":"                    \"default one.\" % self.__class__.__name__)"},{"line_number":217,"context_line":"        return str(version)"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    def _discover_impl(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_3baacd71","line":216,"updated":"2016-05-05 06:20:05.000000000","message":"why not just % self? or type(self) ?","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":213,"context_line":"                raise ValueError("},{"line_number":214,"context_line":"                    \"Failed to process version for %s service, since \""},{"line_number":215,"context_line":"                    \"\u0027version\u0027 argument is not specified and there is no \""},{"line_number":216,"context_line":"                    \"default one.\" % self.__class__.__name__)"},{"line_number":217,"context_line":"        return str(version)"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    def _discover_impl(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_895f9271","line":216,"in_reply_to":"1a122d0e_3baacd71","updated":"2016-05-07 19:15:33.000000000","message":"Because it is not user-friendly:\n\nIn [1]: class Service(object): pass\n\nIn [2]: \"%s\" % Service()\nOut[2]: \u0027\u003c__main__.Service object at 0x7fb9ba499310\u003e\u0027\n\nIn [3]: \"%s\" % type(Service())\nOut[3]: \"\u003cclass \u0027__main__.Service\u0027\u003e\"\n\nIn [4]: \"%s\" % Service().__class__.__name__\nOut[4]: \u0027Service\u0027","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"e83a7b8788ce7d41d343eb2f5a404d67f44660da","unresolved":false,"context_lines":[{"line_number":213,"context_line":"                raise ValueError("},{"line_number":214,"context_line":"                    \"Failed to process version for %s service, since \""},{"line_number":215,"context_line":"                    \"\u0027version\u0027 argument is not specified and there is no \""},{"line_number":216,"context_line":"                    \"default one.\" % self.__class__.__name__)"},{"line_number":217,"context_line":"        return str(version)"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    def _discover_impl(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_1d13db0e","line":216,"in_reply_to":"dab17558_895f9271","updated":"2016-05-08 00:12:29.000000000","message":"type(Service) is most user-friendly. \n\nYou know exactly where is the class that raises exception and you can go there and fix it.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    @property"},{"line_number":231,"context_line":"    def _osclient(self):"},{"line_number":232,"context_line":"        \"\"\"Returns rally.osclients.OSClient instance for current service.\"\"\""},{"line_number":233,"context_line":"        return getattr(self._clients, self._CLIENT_NAME)"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    @property"},{"line_number":236,"context_line":"    @commonmethod"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_1bd551e4","line":233,"updated":"2016-05-05 06:20:05.000000000","message":"so I am not sure how this works in case of nova/neutron and when you pass just version.\n\nBtw why this is property and not just regular method, it looks like a method.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    @property"},{"line_number":236,"context_line":"    @commonmethod"},{"line_number":237,"context_line":"    def client(self):"},{"line_number":238,"context_line":"        \"\"\"Returns initialized client for current service.\"\"\""},{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_fbdb35eb","line":237,"updated":"2016-05-05 06:20:05.000000000","message":"Could you show sample where this approach is better, than just using \ndirectly self.clients \n\nFor example in heat scenarios: \n\nI would like to do the next: \n\n   self.clients(\"heat\").stacks.create(.....)\n   list_of_servers \u003d self.clients(\"nova\").servers.list() \n   # make sure that all servers are created \n\nhow this is going to look in your case? \n\n   self.client.stacks.create(....)\n   list_of_servers \u003d self.clients(\"nova\").servers.list() \n\nso we have 2 different approach of initializations of python clients.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    @property"},{"line_number":236,"context_line":"    @commonmethod"},{"line_number":237,"context_line":"    def client(self):"},{"line_number":238,"context_line":"        \"\"\"Returns initialized client for current service.\"\"\""},{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_89a4b257","line":237,"in_reply_to":"1a122d0e_fbdb35eb","updated":"2016-05-07 19:15:33.000000000","message":"in heat scenarios, it will look like:\n\n    from rally.plugins.openstack.services import orchestration\n\n    class HeatSceanrios(...):\n        def __init__(self, *args, **kwargs):\n            super(HeatScenarios, self).__init__(*args, **kwargs)\n            self.heat \u003d orchestration.Orchestration(\n                self._clients, \n                self._atomic_inst, \n                self.generate_random_name)\n\n        def create_something(self):\n            res \u003d self.clients(\"nova\").servers.get(\"asdasd\")\n            self.heat.do_something(res)\n\nin heat service implementation:\n\n    class Orchestration(base.Service):\n        def do_something(self, res\u003dNone):\n            return self._impl.do_something(res)\n\n    class HeatV1Service(Orchestration):\n        def do_something(self, res\u003dNone):\n            if res is None:\n                # obtain something default\n                res \u003d self.clients.nova().servers.get(\"default\")\n            return self.client.something.do(res)\n\nIn most cases, only one client will be used per Service, so `client` property is a simlink to simplify usage of it.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"e83a7b8788ce7d41d343eb2f5a404d67f44660da","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    @property"},{"line_number":236,"context_line":"    @commonmethod"},{"line_number":237,"context_line":"    def client(self):"},{"line_number":238,"context_line":"        \"\"\"Returns initialized client for current service.\"\"\""},{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_fd2c174c","line":237,"in_reply_to":"dab17558_89a4b257","updated":"2016-05-08 00:12:29.000000000","message":"Please do not make *any* assumptions that may work in many current cases (if they already don\u0027t work in all cases) especially at this level, it may become a huge blocker in future that will require refactoring of all code, which is very bad. \n\nUsing 2 or more clients should be something specific, and adding another abstraction, doesn\u0027t look like simplify things (it adds another layer of abstraction that newbies need to learn) and doesn\u0027t provide actually any big value.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"0bea22bb78ff0e724b8c2d4e3aa77e879842cab7","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"    @commonmethod"},{"line_number":242,"context_line":"    def generate_random_name(self):"},{"line_number":243,"context_line":"        if not self._parent:"},{"line_number":244,"context_line":"            raise exceptions.RallyException("},{"line_number":245,"context_line":"                \"You cannot use `generate_random_name` method without parent \""}],"source_content_type":"text/x-python","patch_set":14,"id":"1a122d0e_5b306956","line":242,"updated":"2016-05-05 06:20:05.000000000","message":"Instead of creating common method that can fail in runtime let\u0027s just implement this once in base class and forget,\n\ne.g. \n\n   self.generate_random_name \u003d name_generator","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"dc2510899ec676ceb285f97fd044bbc292961fb7","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"    @commonmethod"},{"line_number":242,"context_line":"    def generate_random_name(self):"},{"line_number":243,"context_line":"        if not self._parent:"},{"line_number":244,"context_line":"            raise exceptions.RallyException("},{"line_number":245,"context_line":"                \"You cannot use `generate_random_name` method without parent \""}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_29ab061f","line":242,"in_reply_to":"1a122d0e_5b306956","updated":"2016-05-07 19:15:33.000000000","message":"Service class can be initialized without name_generator(for example we initialize it in cleanup resources), so we can make `name_generator` required argument. To prevent AttribureError, let\u0027s keep this method.","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"e83a7b8788ce7d41d343eb2f5a404d67f44660da","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        return self._osclient()"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"    @commonmethod"},{"line_number":242,"context_line":"    def generate_random_name(self):"},{"line_number":243,"context_line":"        if not self._parent:"},{"line_number":244,"context_line":"            raise exceptions.RallyException("},{"line_number":245,"context_line":"                \"You cannot use `generate_random_name` method without parent \""}],"source_content_type":"text/x-python","patch_set":14,"id":"dab17558_1d2abb2c","line":242,"in_reply_to":"dab17558_29ab061f","updated":"2016-05-08 00:12:29.000000000","message":"1) IMHO developers shouldn\u0027t not be able to initialize these class without generate_random_name method (even in case of cleanup classes) \n\n2) It\u0027s better to catch this on module load time, than in runtime (because runtime can be your production cloud)","commit_id":"63c9c979726201e51a177dc15df970c7e2cdd4c1"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        #"},{"line_number":48,"context_line":"        #   .. code-block:: python"},{"line_number":49,"context_line":"        #"},{"line_number":50,"context_line":"        #       class IdentityService(Service):"},{"line_number":51,"context_line":"        #           def add_role(self, role_id, user_id, project_id):"},{"line_number":52,"context_line":"        #               self._impl(role_id, user_id, project_id)"},{"line_number":53,"context_line":"        #"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_fdba57e0","line":50,"updated":"2016-05-08 00:54:22.000000000","message":"it will be more interesting to see how to resolve situation when we are implementi\u003dng method that looks different in different version (project/tenant)","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        #"},{"line_number":48,"context_line":"        #   .. code-block:: python"},{"line_number":49,"context_line":"        #"},{"line_number":50,"context_line":"        #       class IdentityService(Service):"},{"line_number":51,"context_line":"        #           def add_role(self, role_id, user_id, project_id):"},{"line_number":52,"context_line":"        #               self._impl(role_id, user_id, project_id)"},{"line_number":53,"context_line":"        #"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_044b5fb1","line":50,"in_reply_to":"dab17558_fdba57e0","updated":"2016-05-09 17:47:31.000000000","message":"It doesn\u0027t solve such case. For example we can do it via specific decorator:\n\n   class KeystoneV2:\n      def __init__(compatibility_mode):\n           if compatibility_mode:\n                for name, method in iteritems(self.compatibility_names):\n                     setattr(self, name, method)\n\n      @compatibility_name(\"create_project\"):\n      def create_tenant(...): pass\n\nBut it will make all the logic more complicated.\n\nor there is another solution(which was rejected while discussion):\n\n    class Identity():\n        def create_project():\n            if self.version \u003d\u003d \"2\":\n                tenant \u003d self._impl.create_tenant()\n                # crop keystone v2 specific fields and convert to unified term \"project\"\n                return self.convert_tenant2project(tenant)\n            elif self.version \u003d\u003d \"3\":\n                project \u003d self._impl.create_project()\n                # crop keystone v3 specific fields\n                return self.crop_project(project)\n\nIn this case version implementation will be simple, we will able to name methods by real names there. Also it will simplify _discover_impl method, since common class will know all his version implementation(it can be hardcoded via some mapping), but it will be too hardcoded.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":177,"context_line":"                pass"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":"    \"\"\""},{"line_number":180,"context_line":"    SERVICE_TYPE \u003d None"},{"line_number":181,"context_line":"    SERVICE_NAME \u003d None"},{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_fd889724","line":180,"updated":"2016-05-08 00:54:22.000000000","message":"We already see many times, that this approach works quite bad. \n\nThis arguments should be set by decorator, so there won\u0027t be typos and we will have way to put doc strings on these values and explain what these arguments are doing.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":181,"context_line":"    SERVICE_NAME \u003d None"},{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_bdc9ef7a","line":184,"updated":"2016-05-08 00:54:22.000000000","message":"name_generation should be required parameter. \n\notherwise we will create a lot of runtime checks and bugs that can be catch only during runtime (which is bad in case of python)","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"4569878e831efb974c5021794cf9df9cd8ce87aa","unresolved":false,"context_lines":[{"line_number":181,"context_line":"    SERVICE_NAME \u003d None"},{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_bc92639d","line":184,"in_reply_to":"dab17558_a7cb75a4","updated":"2016-05-09 21:01:09.000000000","message":"This is not true.\n\nCleanup needs to have actually instance of name generator (so it will be able to check that resource belongs to the Rally and it can delete it)","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":181,"context_line":"    SERVICE_NAME \u003d None"},{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_a7cb75a4","line":184,"in_reply_to":"dab17558_bdc9ef7a","updated":"2016-05-09 17:47:31.000000000","message":"In this case it will reduce the number of place where service can be used(if I need just list resources in compatible way, do I need this name generator?). For example, Cleanup context doesn\u0027t have name_generator, since it doesn\u0027t create resources, just list and delete. We can transmit name_generator\u003dNone there, but someone can look at our code, copy-paste it and get un-friendly \"TypeError: \u0027NoneType\u0027 object is not callable\".","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_9dc4eb5f","line":185,"updated":"2016-05-08 00:54:22.000000000","message":"from reading the docstrings I can\u0027t understand why we need? Why can\u0027t I just intilize directly CommonService instance if I need comptibility_mode","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"2b978f0f1d90d7a3f7bbb3d07f794158c26ae1e4","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_8758e833","line":185,"in_reply_to":"dab17558_3c62f3ad","updated":"2016-05-09 21:26:44.000000000","message":"\u003eIf I initalize specific version I would like to get specific version. \n\nspecific version can be initialized directly or via common class. In first case you expect to see raw object, in second case, you expect to see an object which are equal for whatever version is used. Since we decided to store that each version implementation should take care about converting raw object to unified form by itself, we need to inform it about our choice(what kind of object we expect). This variable is a such identifier. But in case of common class it is redundant.\n\nAs an alternative, we can use inspect to obtain the caller and based on it decide what kind of object should be returned.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"4569878e831efb974c5021794cf9df9cd8ce87aa","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_3c62f3ad","line":185,"in_reply_to":"dab17558_4730c99e","updated":"2016-05-09 21:01:09.000000000","message":"HM it\u0027s not clear for me....\n\nIf I initialize common service I would like to get just common methods.\nIf I initalize specific version I would like to get specific version. \n\nWhat is the third option? make specific version return just common methods? Why we need this at all?","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":182,"context_line":"    VERSION \u003d None"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def __init__(self, clients, atomic_inst\u003dNone, name_generator\u003dNone,"},{"line_number":185,"context_line":"                 compatibility_mode\u003dNone):"},{"line_number":186,"context_line":"        \"\"\"Initialize service class"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_4730c99e","line":185,"in_reply_to":"dab17558_9dc4eb5f","updated":"2016-05-09 17:47:31.000000000","message":"Version implementations should not be hardcoded for usage only in compatibility mode, so we support for them two modes: compatible with other versions or returning version specific object with all its fields.\n\nIn case of common class, it should always return one type of object - unified for all versions. So there is no need to allow argument compatibility_mode there.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        else:"},{"line_number":210,"context_line":"            # assume that it is a specific service common class initialization"},{"line_number":211,"context_line":"            if compatibility_mode is not None:"},{"line_number":212,"context_line":"                raise ValueError(\"Setting \u0027compatibility_mode\u0027 is restricted \""},{"line_number":213,"context_line":"                                 \"for service common class\")"},{"line_number":214,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":215,"context_line":"            self.SERVICE_NAME \u003d self._impl.SERVICE_NAME"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_1daf5b95","line":212,"updated":"2016-05-08 00:54:22.000000000","message":"Why we need this compability mode, and adding one more potential unexpected runtime errors?","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        else:"},{"line_number":210,"context_line":"            # assume that it is a specific service common class initialization"},{"line_number":211,"context_line":"            if compatibility_mode is not None:"},{"line_number":212,"context_line":"                raise ValueError(\"Setting \u0027compatibility_mode\u0027 is restricted \""},{"line_number":213,"context_line":"                                 \"for service common class\")"},{"line_number":214,"context_line":"            self._impl \u003d self._discover_impl()"},{"line_number":215,"context_line":"            self.SERVICE_NAME \u003d self._impl.SERVICE_NAME"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_a7ded53c","line":212,"in_reply_to":"dab17558_1daf5b95","updated":"2016-05-09 17:47:31.000000000","message":"1) see the comment above\n2) we can just ignore this argument in case of common class or print warning.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":216,"context_line":"            self.VERSION \u003d self._impl.VERSION"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"    @classmethod"},{"line_number":219,"context_line":"    def _get_version(cls, clients):"},{"line_number":220,"context_line":"        \"\"\"Obtain default version."},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        :param clients: OSClient\u0027s instance"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_5d9f2350","line":219,"updated":"2016-05-08 00:54:22.000000000","message":"Why we need this method? \nWe already have chose_version() that obtains default version, wrapping it one more time is bad design. \n\nIf you need to rise the exception every time when you are using I would strongly recommend to put this code in OSclients.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"2b978f0f1d90d7a3f7bbb3d07f794158c26ae1e4","unresolved":false,"context_lines":[{"line_number":216,"context_line":"            self.VERSION \u003d self._impl.VERSION"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"    @classmethod"},{"line_number":219,"context_line":"    def _get_version(cls, clients):"},{"line_number":220,"context_line":"        \"\"\"Obtain default version."},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        :param clients: OSClient\u0027s instance"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_67cea4b5","line":219,"in_reply_to":"dab17558_5d9f2350","updated":"2016-05-09 21:26:44.000000000","message":"Btw, choose_version of OSClient is designed to choose version for initialization of client. So it is a pre-step.\nIn case of keystone, it doesn\u0027t require a strict version for initialization and this pre-step is redundant there. But we need chosen version here.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":216,"context_line":"            self.VERSION \u003d self._impl.VERSION"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"    @classmethod"},{"line_number":219,"context_line":"    def _get_version(cls, clients):"},{"line_number":220,"context_line":"        \"\"\"Obtain default version."},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        :param clients: OSClient\u0027s instance"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_27c66548","line":219,"in_reply_to":"dab17558_5d9f2350","updated":"2016-05-09 17:47:31.000000000","message":"For such questions, I post an example of implementation.\nsee L36 https://review.openstack.org/#/c/310758/12/rally/plugins/openstack/services/identity.py\n\nSo for most clients we do not need initialized client to choose version, so choose_version can be used for validation without any client initialization. BUT there is one client which do not have cross deployment default version. It is keystoneclient. You need to initialize client to obtain default version for client. \n\nI do not want to make choose_version require initialized client only for keystoneclient","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":228,"context_line":"                \"api_versions context to specify it.\" % cls.SERVICE_NAME)"},{"line_number":229,"context_line":"        return str(version)"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"    def _discover_impl(self):"},{"line_number":232,"context_line":"        implementations \u003d ["},{"line_number":233,"context_line":"            cls for cls in discover.itersubclasses(self.__class__)"},{"line_number":234,"context_line":"            if cls.SERVICE_NAME and cls.VERSION]"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_fdb6d7da","line":231,"updated":"2016-05-08 00:54:22.000000000","message":"This code looks over complicated, and doesn\u0027t have any docstrings that explains to me why it\u0027s so hard to discover right implementation.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":228,"context_line":"                \"api_versions context to specify it.\" % cls.SERVICE_NAME)"},{"line_number":229,"context_line":"        return str(version)"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"    def _discover_impl(self):"},{"line_number":232,"context_line":"        implementations \u003d ["},{"line_number":233,"context_line":"            cls for cls in discover.itersubclasses(self.__class__)"},{"line_number":234,"context_line":"            if cls.SERVICE_NAME and cls.VERSION]"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_8702b16d","line":231,"in_reply_to":"dab17558_fdb6d7da","updated":"2016-05-09 17:47:31.000000000","message":"I can add docstrings and inline comments, but I do not know the way how this method can be simplified. Any suggestions? \n\nIf inline comments posted below will be enough, please let me know.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            cls for cls in discover.itersubclasses(self.__class__)"},{"line_number":234,"context_line":"            if cls.SERVICE_NAME and cls.VERSION]"},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"        service_names \u003d set(map(lambda o: o.SERVICE_NAME, implementations))"},{"line_number":237,"context_line":""},{"line_number":238,"context_line":"        enabled_services \u003d None"},{"line_number":239,"context_line":"        if len(service_names) \u003e 1:"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_677a2df3","line":236,"range":{"start_line":236,"start_character":8,"end_line":236,"end_character":75},"updated":"2016-05-09 17:47:31.000000000","message":"Common class can support different implementation of service, which can be different not only by version, but by project too. For example, we have NetworkService which is implemented via Neutron(service_name\u003d\"neutron\", supported_version\u003d[\"2.0\"], service_type\u003d\"network\") and NovaNetwork(service_name\u003d\"nova-network\", supported_version\u003d[\"2\"], service_type\u003d\"network\"). In such cases, we can\u0027t choose implementation using only the default version of service. We should check available services in the cloud and make a decision after that.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":237,"context_line":""},{"line_number":238,"context_line":"        enabled_services \u003d None"},{"line_number":239,"context_line":"        if len(service_names) \u003e 1:"},{"line_number":240,"context_line":"            enabled_services \u003d list(self._clients.services().values())"},{"line_number":241,"context_line":"            if consts.Service.NOVA_NET in service_names:"},{"line_number":242,"context_line":"                for service in self._clients.nova().services.list():"},{"line_number":243,"context_line":"                    if (service.binary \u003d\u003d consts.Service.NOVA_NET and"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_a7e9b582","line":240,"updated":"2016-05-09 17:47:31.000000000","message":"we have a complex service, so let\u0027s check enabled services.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":238,"context_line":"        enabled_services \u003d None"},{"line_number":239,"context_line":"        if len(service_names) \u003e 1:"},{"line_number":240,"context_line":"            enabled_services \u003d list(self._clients.services().values())"},{"line_number":241,"context_line":"            if consts.Service.NOVA_NET in service_names:"},{"line_number":242,"context_line":"                for service in self._clients.nova().services.list():"},{"line_number":243,"context_line":"                    if (service.binary \u003d\u003d consts.Service.NOVA_NET and"},{"line_number":244,"context_line":"                            service.status \u003d\u003d \"enabled\"):"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_47c0c9e0","line":241,"updated":"2016-05-09 17:47:31.000000000","message":"Nova-network is not listed in keystone catalog and we can not assume that it is enabled if neutron is missed. Since such discovery needs external calls, let\u0027s make it only if needed.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":249,"context_line":"            if enabled_services:"},{"line_number":250,"context_line":"                if cls.SERVICE_NAME not in enabled_services:"},{"line_number":251,"context_line":"                    continue"},{"line_number":252,"context_line":"            if cls.SERVICE_NAME not in cache:"},{"line_number":253,"context_line":"                cache[cls.SERVICE_NAME] \u003d cls._get_version(self._clients)"},{"line_number":254,"context_line":"            if cls.VERSION \u003d\u003d cache[cls.SERVICE_NAME]:"},{"line_number":255,"context_line":"                return cls(self._clients, self._atomic_actions,"},{"line_number":256,"context_line":"                           self._name_generator, compatibility_mode\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_e7261dcd","line":253,"range":{"start_line":252,"start_character":12,"end_line":253,"end_character":73},"updated":"2016-05-09 17:47:31.000000000","message":"_get_version method can be service specific(for example, keystone requires initialized client to do obtain version), in this case we do not want to obtain version several times for different API versions, so let\u0027s cache it.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"2b978f0f1d90d7a3f7bbb3d07f794158c26ae1e4","unresolved":false,"context_lines":[{"line_number":249,"context_line":"            if enabled_services:"},{"line_number":250,"context_line":"                if cls.SERVICE_NAME not in enabled_services:"},{"line_number":251,"context_line":"                    continue"},{"line_number":252,"context_line":"            if cls.SERVICE_NAME not in cache:"},{"line_number":253,"context_line":"                cache[cls.SERVICE_NAME] \u003d cls._get_version(self._clients)"},{"line_number":254,"context_line":"            if cls.VERSION \u003d\u003d cache[cls.SERVICE_NAME]:"},{"line_number":255,"context_line":"                return cls(self._clients, self._atomic_actions,"},{"line_number":256,"context_line":"                           self._name_generator, compatibility_mode\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_67806447","line":253,"range":{"start_line":252,"start_character":12,"end_line":253,"end_character":73},"in_reply_to":"dab17558_5c2d3745","updated":"2016-05-09 21:26:44.000000000","message":"agree about caching.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"4569878e831efb974c5021794cf9df9cd8ce87aa","unresolved":false,"context_lines":[{"line_number":249,"context_line":"            if enabled_services:"},{"line_number":250,"context_line":"                if cls.SERVICE_NAME not in enabled_services:"},{"line_number":251,"context_line":"                    continue"},{"line_number":252,"context_line":"            if cls.SERVICE_NAME not in cache:"},{"line_number":253,"context_line":"                cache[cls.SERVICE_NAME] \u003d cls._get_version(self._clients)"},{"line_number":254,"context_line":"            if cls.VERSION \u003d\u003d cache[cls.SERVICE_NAME]:"},{"line_number":255,"context_line":"                return cls(self._clients, self._atomic_actions,"},{"line_number":256,"context_line":"                           self._name_generator, compatibility_mode\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_5c2d3745","line":253,"range":{"start_line":252,"start_character":12,"end_line":253,"end_character":73},"in_reply_to":"dab17558_e7261dcd","updated":"2016-05-09 21:01:09.000000000","message":"Andrey, \n\nLet\u0027s make better OSclients instead of adding hacks. If client is initialized once, it will be already cached by it self, so no need for extra level of caching here.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    @property"},{"line_number":263,"context_line":"    @commonmethod"},{"line_number":264,"context_line":"    def client(self):"},{"line_number":265,"context_line":"        \"\"\"Returns initialized client for current service.\"\"\""},{"line_number":266,"context_line":"        return self._get_osclient(self._clients)()"},{"line_number":267,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_1d9d9b59","line":264,"updated":"2016-05-08 00:54:22.000000000","message":"I still don\u0027t like this code, and your comment that in most cases this will work doesn\u0027t change my opinion. \n\nExplicitly of what client and what version you are using is thing that we must have, otherwise we will create unreadable code. \n\nMore over, having the same way to work with openstack clients everywhere is way more valuable and improtant that saving few symbols and instead of writing: \n\n   self.clients(\"nova\", \"2\")  # here I know exactly what I am using\n\n   vs\n\n   self.client # here I don\u0027t know anything \n\n as well as I mention previously we won\u0027t be able to use self.client in case if many projects are required. \n\nBy the way we already have very good abstraction OSClients that hides a lot of work related to initialization and doesn\u0027t hide such crucial infomration about what exactly version and project we are using.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    @property"},{"line_number":263,"context_line":"    @commonmethod"},{"line_number":264,"context_line":"    def client(self):"},{"line_number":265,"context_line":"        \"\"\"Returns initialized client for current service.\"\"\""},{"line_number":266,"context_line":"        return self._get_osclient(self._clients)()"},{"line_number":267,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_87163153","line":264,"in_reply_to":"dab17558_1d9d9b59","updated":"2016-05-09 17:47:31.000000000","message":"Ok, let\u0027s remove this method.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":266,"context_line":"        return self._get_osclient(self._clients)()"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    @classmethod"},{"line_number":269,"context_line":"    def _get_osclient(cls, clients):"},{"line_number":270,"context_line":"        # NOTE(andreykurilin): This method can look redundant, but it is not."},{"line_number":271,"context_line":"        #   Some specific service(for example nova-network) can have different"},{"line_number":272,"context_line":"        #   logic for obtaining client. It should be designed like:"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_3d833f06","line":269,"updated":"2016-05-08 00:54:22.000000000","message":"Just remove this method,\n\n  1) It hides that we have just bad design in OSclients that requires such ugly construction to run get_validated_verstion() \n\n  2) It adds more levels in stacks and creates a lot of unclears of newbies what way they should use clients, which in feature will require refactoring of whole scenarios, context and cleanups one more time. \n\n\n\nIK would suggest, just to have way to obtain clients class, for example:\n\n\n   self.clients.get_cls(cls.SERVICE_NAME).get_validate_version \n\n\nor even better extend just base clients class with this method: \n\n   self.clients.get_validate_version(cls.SERVICE_NAME)","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":266,"context_line":"        return self._get_osclient(self._clients)()"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    @classmethod"},{"line_number":269,"context_line":"    def _get_osclient(cls, clients):"},{"line_number":270,"context_line":"        # NOTE(andreykurilin): This method can look redundant, but it is not."},{"line_number":271,"context_line":"        #   Some specific service(for example nova-network) can have different"},{"line_number":272,"context_line":"        #   logic for obtaining client. It should be designed like:"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_a2a9a3e1","line":269,"in_reply_to":"dab17558_3d833f06","updated":"2016-05-09 17:47:31.000000000","message":"Sorry, it is not a small change and I have no time to rewrite logic of Clients now.\n\nAs we need _get_version for only one client - keystoneclient, as we need _get_osclient for another one client - nova-network.","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"7265a270ea84573d11a2bfc155b1db8c01bdb550","unresolved":false,"context_lines":[{"line_number":266,"context_line":"        return self._get_osclient(self._clients)()"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    @classmethod"},{"line_number":269,"context_line":"    def _get_osclient(cls, clients):"},{"line_number":270,"context_line":"        # NOTE(andreykurilin): This method can look redundant, but it is not."},{"line_number":271,"context_line":"        #   Some specific service(for example nova-network) can have different"},{"line_number":272,"context_line":"        #   logic for obtaining client. It should be designed like:"}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_98eb30b0","line":269,"in_reply_to":"dab17558_a2a9a3e1","updated":"2016-05-09 18:51:41.000000000","message":"ok, if we write NovaNetwork as code below, we can remove this method.\n\n    class NovaNetwork(Network):\n        SERVICE_TYPE \u003d \"compute\"","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":6172,"name":"Boris Pavlovic","email":"boris@pavlovic.me","username":"boris-42"},"change_message_id":"d38586875b3cfe32bc1d17bb390e94bd1a0b8cb2","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        return getattr(clients, cls.SERVICE_NAME)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    @commonmethod"},{"line_number":286,"context_line":"    def generate_random_name(self):"},{"line_number":287,"context_line":"        if not self._name_generator:"},{"line_number":288,"context_line":"            raise exceptions.RallyException("},{"line_number":289,"context_line":"                \"You cannot use `generate_random_name` method, until you \""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_ddbdd3d8","line":286,"updated":"2016-05-08 00:54:22.000000000","message":"See previous patch comments and comments in init class, we can remove this method as well","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"78cdb22befede5e3d476a4666b5af42242837ea3","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        return getattr(clients, cls.SERVICE_NAME)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    @commonmethod"},{"line_number":286,"context_line":"    def generate_random_name(self):"},{"line_number":287,"context_line":"        if not self._name_generator:"},{"line_number":288,"context_line":"            raise exceptions.RallyException("},{"line_number":289,"context_line":"                \"You cannot use `generate_random_name` method, until you \""}],"source_content_type":"text/x-python","patch_set":16,"id":"dab17558_e2c0cb7e","line":286,"in_reply_to":"dab17558_ddbdd3d8","updated":"2016-05-09 17:47:31.000000000","message":"I left an answer for you at init method:)","commit_id":"549d0c1aedac333e02b900a66f70311d073e7591"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"3e38e7aa308db6bcbf9dbfb08302cac4a0b5168c","unresolved":false,"context_lines":[{"line_number":169,"context_line":""},{"line_number":170,"context_line":"@six.add_metaclass(ServiceMeta)"},{"line_number":171,"context_line":"class Service(meta.MetaMixin):"},{"line_number":172,"context_line":"    \"\"\"Base class for help classed for OpenStack Services."},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    A simple example of Identity service implementation:"},{"line_number":175,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"dab17558_0f7cf4b3","line":172,"updated":"2016-05-16 12:07:26.000000000","message":"s/classed/classes/","commit_id":"0b04dbec09fa839562f54235ecf15158e45845e1"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"3e38e7aa308db6bcbf9dbfb08302cac4a0b5168c","unresolved":false,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        class Identity(Service):"},{"line_number":191,"context_line":"            def create_project(self, project_name, domain_name\u003d\"Default\"):"},{"line_number":192,"context_line":"                return self._impl(project_name, domain_name\u003ddomain_name)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":""},{"line_number":195,"context_line":"        @common_implementation(KeystoneServiceV2)"}],"source_content_type":"text/x-python","patch_set":19,"id":"dab17558_e37548cb","line":192,"updated":"2016-05-16 12:07:26.000000000","message":"s/self._impl(/self._impl.create_project(/","commit_id":"0b04dbec09fa839562f54235ecf15158e45845e1"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"3e38e7aa308db6bcbf9dbfb08302cac4a0b5168c","unresolved":false,"context_lines":[{"line_number":261,"context_line":"        if impl_cls:"},{"line_number":262,"context_line":"            self._impl \u003d impl_cls(self._clients, self._name_generator,"},{"line_number":263,"context_line":"                                  self._atomic_actions)"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    @classmethod"},{"line_number":266,"context_line":"    def is_applicable(cls, clients):"},{"line_number":267,"context_line":"        \"\"\"Check that implementation can be used in cloud.\"\"\""}],"source_content_type":"text/x-python","patch_set":19,"id":"dab17558_5bf7d858","line":264,"updated":"2016-05-16 12:07:26.000000000","message":"How service version can be obtained?","commit_id":"0b04dbec09fa839562f54235ecf15158e45845e1"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":44,"context_line":"    return wrapper"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"def common_implementation(original_impl):"},{"line_number":48,"context_line":"    \"\"\"Set class which should be unified to common interface"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    :param original_impl: implementation of specific service API"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_ee94d78b","line":47,"updated":"2016-08-11 13:14:36.000000000","message":"How about to make this name shorter? - just call it common","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"0a6cb35b2439c53d087441684b5c24c4ef008953","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    return wrapper"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class ServiceMeta(type):"},{"line_number":61,"context_line":"    \"\"\"Alternative implementation of abstract classes for Services."},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    Common class of specific Service should not be hardcoded for any version of"}],"source_content_type":"text/x-python","patch_set":21,"id":"1aa78d24_88456f44","line":60,"updated":"2016-07-12 09:25:23.000000000","message":"Suggestion: meta programming overcomplicates the code, I can assist to help get rid this meta but having same features","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    return wrapper"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class ServiceMeta(type):"},{"line_number":61,"context_line":"    \"\"\"Alternative implementation of abstract classes for Services."},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    Common class of specific Service should not be hardcoded for any version of"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_603953a4","line":60,"in_reply_to":"1aa78d24_88456f44","updated":"2016-08-11 13:14:36.000000000","message":"Personally I like this ServiceMeta class","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"@six.add_metaclass(ServiceMeta)"},{"line_number":101,"context_line":"class Service(meta.MetaMixin):"},{"line_number":102,"context_line":"    \"\"\"Base class for help classes for OpenStack Services."},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"    A simple example of Identity service implementation:"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_8ff630f7","line":101,"updated":"2016-08-11 13:14:36.000000000","message":"Why we don\u0027t inherit it for AtomicMixin and RandomNameGeneratorMixin? This will free us from the function generate_random_name()","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"71770ecc17e60a71aadd1e76580ebbe47cc0ea18","unresolved":false,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"@six.add_metaclass(ServiceMeta)"},{"line_number":101,"context_line":"class Service(meta.MetaMixin):"},{"line_number":102,"context_line":"    \"\"\"Base class for help classes for OpenStack Services."},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"    A simple example of Identity service implementation:"}],"source_content_type":"text/x-python","patch_set":21,"id":"fa6399be_647eecaf","line":101,"in_reply_to":"9ad45d7e_8ff630f7","updated":"2016-10-18 14:01:41.000000000","message":"It was a suggestion from boris-42.\n\n* RandomNameGeneratorMixin : we should not create resources based on nothing. It will create a pain while identifying such resources. So users should transmit an instance of RandomNameGeneratorMixin(i.e scenario ) manually","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":191,"context_line":"                # assume it is implementation of specific API. It does not need"},{"line_number":192,"context_line":"                # impl argument"},{"line_number":193,"context_line":"                self.version \u003d self._meta_get(\"version\")"},{"line_number":194,"context_line":"        else:"},{"line_number":195,"context_line":"            impl_cls \u003d utils.discover_impl(self)"},{"line_number":196,"context_line":"            self._impl \u003d impl_cls(self._clients, self._name_generator,"},{"line_number":197,"context_line":"                                  self._atomic_actions)"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_8e50bb0c","line":194,"updated":"2016-08-11 13:14:36.000000000","message":"I don\u0027t understand user case of this at all. we will enter to else block only if we will not have common and service decorators. When does it allowed?","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"6e4316ec34ec0e248ef48890b66bbb1302dad172","unresolved":false,"context_lines":[{"line_number":192,"context_line":"                # impl argument"},{"line_number":193,"context_line":"                self.version \u003d self._meta_get(\"version\")"},{"line_number":194,"context_line":"        else:"},{"line_number":195,"context_line":"            impl_cls \u003d utils.discover_impl(self)"},{"line_number":196,"context_line":"            self._impl \u003d impl_cls(self._clients, self._name_generator,"},{"line_number":197,"context_line":"                                  self._atomic_actions)"},{"line_number":198,"context_line":"            self.version \u003d self._impl.version"}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_0682aec8","line":195,"updated":"2016-06-27 13:42:04.000000000","message":"discover_impl is used only in this place, and it expects a Service instance as argument, so maybe it worth to move it to this class?","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c240d7c7bf6590025d9158bfc57f306782da3b34","unresolved":false,"context_lines":[{"line_number":192,"context_line":"                # impl argument"},{"line_number":193,"context_line":"                self.version \u003d self._meta_get(\"version\")"},{"line_number":194,"context_line":"        else:"},{"line_number":195,"context_line":"            impl_cls \u003d utils.discover_impl(self)"},{"line_number":196,"context_line":"            self._impl \u003d impl_cls(self._clients, self._name_generator,"},{"line_number":197,"context_line":"                                  self._atomic_actions)"},{"line_number":198,"context_line":"            self.version \u003d self._impl.version"}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_424bd8b0","line":195,"in_reply_to":"3aaa91ec_0682aec8","updated":"2016-06-28 06:35:42.000000000","message":"I moved it to separate place to make code of Service class as small as possible. I prefer to leave it there, since it is used only in 1/3 cases","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    @classmethod"},{"line_number":201,"context_line":"    def is_applicable(cls, clients):"},{"line_number":202,"context_line":"        \"\"\"Check that implementation can be used in cloud.\"\"\""},{"line_number":203,"context_line":""},{"line_number":204,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse):"},{"line_number":205,"context_line":"            impl \u003d cls._meta_get(\"impl\", cls)"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_cff828c6","line":202,"updated":"2016-08-11 13:14:36.000000000","message":"looks like this is validation stuff","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"}],"rally/plugins/openstack/services/utils.py":[{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":20,"context_line":"from rally import exceptions"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def discover_impl(service_obj):"},{"line_number":24,"context_line":"    \"\"\"Discover implementation for service"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    One OpenStack service can have different implementations. Each of such"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_ce5753c8","line":23,"updated":"2016-08-11 13:14:36.000000000","message":"This function contains a lot of OpenStack related things and they will be added into service base. This is not good.","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"71770ecc17e60a71aadd1e76580ebbe47cc0ea18","unresolved":false,"context_lines":[{"line_number":20,"context_line":"from rally import exceptions"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def discover_impl(service_obj):"},{"line_number":24,"context_line":"    \"\"\"Discover implementation for service"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    One OpenStack service can have different implementations. Each of such"}],"source_content_type":"text/x-python","patch_set":21,"id":"fa6399be_4c99141f","line":23,"in_reply_to":"9ad45d7e_ce5753c8","updated":"2016-10-18 14:01:41.000000000","message":"Done","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"6e4316ec34ec0e248ef48890b66bbb1302dad172","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    and based on expected version."},{"line_number":30,"context_line":"    \"\"\""},{"line_number":31,"context_line":"    # find all classes with unified implementation"},{"line_number":32,"context_line":"    impls \u003d dict(["},{"line_number":33,"context_line":"        (cls, cls._meta_get(\"impl\"))"},{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_a698fa1c","line":32,"updated":"2016-06-27 13:42:04.000000000","message":"\"dict comprehension\" is a bit more compact","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"71770ecc17e60a71aadd1e76580ebbe47cc0ea18","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    and based on expected version."},{"line_number":30,"context_line":"    \"\"\""},{"line_number":31,"context_line":"    # find all classes with unified implementation"},{"line_number":32,"context_line":"    impls \u003d dict(["},{"line_number":33,"context_line":"        (cls, cls._meta_get(\"impl\"))"},{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"}],"source_content_type":"text/x-python","patch_set":21,"id":"fa6399be_2c94e0e6","line":32,"in_reply_to":"3aaa91ec_a698fa1c","updated":"2016-10-18 14:01:41.000000000","message":"Done","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    impls \u003d dict(["},{"line_number":33,"context_line":"        (cls, cls._meta_get(\"impl\"))"},{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    service_names \u003d set([o._meta_get(\"name\") for o in impls.values()])"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_eee5f708","line":35,"updated":"2016-08-11 13:14:36.000000000","message":"We iterate through classes with @common, but why do we discover only common classes?","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"71770ecc17e60a71aadd1e76580ebbe47cc0ea18","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    impls \u003d dict(["},{"line_number":33,"context_line":"        (cls, cls._meta_get(\"impl\"))"},{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    service_names \u003d set([o._meta_get(\"name\") for o in impls.values()])"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"fa6399be_c4e7b8aa","line":35,"in_reply_to":"9ad45d7e_eee5f708","updated":"2016-10-18 14:01:41.000000000","message":"because we are interested only in compatibility layers. If Service class do not have class which transforms returned data to unified form, it should not be used in base Service class.","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"6e4316ec34ec0e248ef48890b66bbb1302dad172","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    service_names \u003d set([o._meta_get(\"name\") for o in impls.values()])"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    enabled_services \u003d None"},{"line_number":40,"context_line":"    # let\u0027s make additional calls to cloud only when we need to make a"}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_06b58e7d","line":37,"updated":"2016-06-27 13:42:04.000000000","message":"\"set comprehension\" is more compact\n\n service_names \u003d {o._meta_get(\"name\") for o in impls.values()}","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"71770ecc17e60a71aadd1e76580ebbe47cc0ea18","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        for cls in discover.itersubclasses(service_obj.__class__)"},{"line_number":35,"context_line":"        if cls._meta_is_inited(raise_exc\u003dFalse) and cls._meta_get(\"impl\")])"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    service_names \u003d set([o._meta_get(\"name\") for o in impls.values()])"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    enabled_services \u003d None"},{"line_number":40,"context_line":"    # let\u0027s make additional calls to cloud only when we need to make a"}],"source_content_type":"text/x-python","patch_set":21,"id":"fa6399be_acaf102a","line":37,"in_reply_to":"3aaa91ec_06b58e7d","updated":"2016-10-18 14:01:41.000000000","message":"Done","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        if (enabled_services is not None and"},{"line_number":55,"context_line":"                impl._meta_get(\"name\") not in enabled_services):"},{"line_number":56,"context_line":"            continue"},{"line_number":57,"context_line":"        if impl.is_applicable(service_obj._clients):"},{"line_number":58,"context_line":"            return cls"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    raise exceptions.RallyException(\"There is no proper implementation for %s.\""}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_0e716b0f","line":57,"updated":"2016-08-11 13:14:36.000000000","message":"here we check that written by hand version(in osclient:default_versions, api_context) is equal to assigned in @service(version)","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":10475,"name":"Alexander Maretskiy","email":"amaretskiy@mirantis.com","username":"maretskiy"},"change_message_id":"23a8d94005123f0430b7b0ef951612ab1068bc8e","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"class DevNullDict(dict):"},{"line_number":65,"context_line":"    \"\"\"Do not keep anything.\"\"\""},{"line_number":66,"context_line":"    def __setitem__(self, key, value):"},{"line_number":67,"context_line":"        pass"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_d5b32ee8","line":66,"updated":"2016-06-27 13:03:37.000000000","message":"This does not prevent setting values via __init__() and update()","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"284fcc9a83ca87368e80ae8e885d4a9fac9df289","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"class DevNullDict(dict):"},{"line_number":65,"context_line":"    \"\"\"Do not keep anything.\"\"\""},{"line_number":66,"context_line":"    def __setitem__(self, key, value):"},{"line_number":67,"context_line":"        pass"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"3aaa91ec_bb02d370","line":66,"in_reply_to":"3aaa91ec_d5b32ee8","updated":"2016-06-27 13:19:34.000000000","message":"agree. since it is not critical(we use DevNullDict only in one place), I\u0027ll update this class in separate patch or in new patch-set if more comments appear","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"2de5c8d284e5b0c33116427a3776637329934941","unresolved":false,"context_lines":[{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            raise TypeError(message)"},{"line_number":136,"context_line":""},{"line_number":137,"context_line":"        if kwargs.pop(\"no_atomic\", False):"},{"line_number":138,"context_line":"            instance \u003d ServiceWithoutAtomic(instance)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"        return func(instance, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":21,"id":"9ad45d7e_4efc033e","line":137,"updated":"2016-08-11 13:14:36.000000000","message":"I don\u0027t understand the purpose of this argument and thus DevNullDict and ServiceWithoutAtomic classes","commit_id":"dda26c9f91e5bb43a1001a94eff5579a89bfcd4b"}],"rally/task/service.py":[{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"f21acecffbf2278d0d11ef80fbf2fed5100c73e0","unresolved":false,"context_lines":[{"line_number":142,"context_line":""},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"class ServiceMeta(type):"},{"line_number":145,"context_line":"    \"\"\"Alternative implementation of abstract classes for Services."},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    Common class of specific Service should not be hardcoded for any version of"},{"line_number":148,"context_line":"    API. We expect that all public methods of specific common class are"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5da102_6e089792","line":145,"updated":"2016-11-07 06:51:44.000000000","message":"Now its not an alternative\u003d)","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"f21acecffbf2278d0d11ef80fbf2fed5100c73e0","unresolved":false,"context_lines":[{"line_number":145,"context_line":"    \"\"\"Alternative implementation of abstract classes for Services."},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    Common class of specific Service should not be hardcoded for any version of"},{"line_number":148,"context_line":"    API. We expect that all public methods of specific common class are"},{"line_number":149,"context_line":"    overridden in all versioned implementation."},{"line_number":150,"context_line":"    \"\"\""},{"line_number":151,"context_line":"    def __new__(mcs, name, parents, dct):"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5da102_cef7e38a","line":148,"updated":"2016-11-07 06:51:44.000000000","message":"\"class are overridden\" - is it normal sentence?","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"f21acecffbf2278d0d11ef80fbf2fed5100c73e0","unresolved":false,"context_lines":[{"line_number":208,"context_line":""},{"line_number":209,"context_line":"    \"\"\""},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    def __init__(self, clients, name_generator\u003dNone, atomic_inst\u003dNone):"},{"line_number":212,"context_line":"        \"\"\"Initialize service class"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5da102_4c4911d0","line":211,"updated":"2016-11-07 06:51:44.000000000","message":"what is the reason to have default value None, but to raise an Exception always if we don\u0027t set it at line 235?\nIs it normal that we expect to set it after instantiation like:\n  Service().name_generator \u003d foo\nOtherwise I don\u0027t understand check at 234 line","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"},{"author":{"_account_id":9545,"name":"Andriy Kurilin","email":"andr.kurilin@gmail.com","username":"akurilin"},"change_message_id":"c35baecca2fd712fde451002c906e66c0994199f","unresolved":false,"context_lines":[{"line_number":208,"context_line":""},{"line_number":209,"context_line":"    \"\"\""},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    def __init__(self, clients, name_generator\u003dNone, atomic_inst\u003dNone):"},{"line_number":212,"context_line":"        \"\"\"Initialize service class"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"        :param clients: an instance of rally.osclients.Clients"}],"source_content_type":"text/x-python","patch_set":23,"id":"9a629dbe_83588158","line":211,"in_reply_to":"ba5da102_4c4911d0","updated":"2016-11-07 12:07:32.000000000","message":"you can initialize Service class without name generator to access list methods. in this case name_generator is redundant","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"f21acecffbf2278d0d11ef80fbf2fed5100c73e0","unresolved":false,"context_lines":[{"line_number":256,"context_line":""},{"line_number":257,"context_line":"            # this method is equal in UnifiedKeystoneV2 and UnifiedKeystoneV3."},{"line_number":258,"context_line":"            # Since there is no other implementation except Keystone, there"},{"line_number":259,"context_line":"            # are no needs to copy-paste it."},{"line_number":260,"context_line":"            @classmethod"},{"line_number":261,"context_line":"            def _is_applicable(cls, clients):"},{"line_number":262,"context_line":"                cloud_version \u003d clients.keystone().version.split(\".\")[0][1:]"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5da102_2c6a1d1b","line":259,"updated":"2016-11-07 06:51:44.000000000","message":"Example with Network service was better for understanding. Just a note","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"},{"author":{"_account_id":12395,"name":"Roman Vasylets","email":"pomeo92@gmail.com","username":"rvasilets"},"change_message_id":"f21acecffbf2278d0d11ef80fbf2fed5100c73e0","unresolved":false,"context_lines":[{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        service_names \u003d {o._meta_get(\"name\") for o in impls.values()}"},{"line_number":340,"context_line":""},{"line_number":341,"context_line":"        enabled_services \u003d None"},{"line_number":342,"context_line":"        # let\u0027s make additional calls to cloud only when we need to make a"},{"line_number":343,"context_line":"        # decision based on available services"},{"line_number":344,"context_line":"        if len(service_names) \u003e 1:"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5da102_103e43b8","line":341,"updated":"2016-11-07 06:51:44.000000000","message":"enabled_services \u003d []\nand we are able to rewrite 348 is statement into more compact:\nif impl._meta_get(\"name\") not in enabled_services","commit_id":"b2a186d2d970df24a6939148814b1fbe3fa7b415"}]}