)]}'
{"kolla_cli/common/inventory.py":[{"author":{"_account_id":17386,"name":"Steve Noyes","email":"steve.noyes@oracle.com","username":"ssn1ssn"},"change_message_id":"eb92aad887f46e68a9eb184dbcce3336e193c20e","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        for servicename, service in ansible_inventory.services.items():"},{"line_number":109,"context_line":"            if servicename not in self._services:"},{"line_number":110,"context_line":"                self._services[servicename] \u003d service"},{"line_number":111,"context_line":"            # If Service newly enabled, take the latest"},{"line_number":112,"context_line":"            elif self._services[servicename].is_supported() !\u003d \\"},{"line_number":113,"context_line":"                    service.is_supported():"},{"line_number":114,"context_line":"                self._services[servicename] \u003d service"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_49c6436c","line":111,"updated":"2018-07-23 12:49:20.000000000","message":"This is a little confusing to me. I think what is going on is that a service is in the inventory, but it is not enabled. So in the new version, it is now enabled and that wasn\u0027t adjusted during the upgrade to the new version. (My first reaction to this change was that why would we need to add a service that is not enabled?)\n\nI think this would be clearer if instead of loading the new service on top of the existing service, you just set the enable bit (on or off). \n\nLikewise below, it might be clearer to just set the group. What do you think?\n\nOr if you decided to keep it this way, you should then add a bunch of comments explaining how this is working for the sake of clarity.\n\nThanks!","commit_id":"03cfb35970ded2c6225f1ba1e9dd3e5686a45b86"},{"author":{"_account_id":16520,"name":"Borne Mace","email":"borne.mace@oracle.com","username":"bmace"},"change_message_id":"a74896a31bc39c66da80e571e5a1d8f18ff597c4","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        for servicename, service in ansible_inventory.services.items():"},{"line_number":109,"context_line":"            if servicename not in self._services:"},{"line_number":110,"context_line":"                self._services[servicename] \u003d service"},{"line_number":111,"context_line":"            # If Service newly enabled, take the latest"},{"line_number":112,"context_line":"            elif self._services[servicename].is_supported() !\u003d \\"},{"line_number":113,"context_line":"                    service.is_supported():"},{"line_number":114,"context_line":"                self._services[servicename].set_supported(True)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_6cdbff79","line":111,"updated":"2018-11-14 17:39:48.000000000","message":"There is an odd assumption here that is_supported() !\u003d is_supported() is an enablement case.  This could also be a service that was previously enabled being disabled.  Shouldn\u0027t this use the new is_supported() value rather than True in the set_supported() call?","commit_id":"59b0b33fea60d4d5676def7540208a21fe0e073b"},{"author":{"_account_id":16520,"name":"Borne Mace","email":"borne.mace@oracle.com","username":"bmace"},"change_message_id":"a74896a31bc39c66da80e571e5a1d8f18ff597c4","unresolved":false,"context_lines":[{"line_number":112,"context_line":"            elif self._services[servicename].is_supported() !\u003d \\"},{"line_number":113,"context_line":"                    service.is_supported():"},{"line_number":114,"context_line":"                self._services[servicename].set_supported(True)"},{"line_number":115,"context_line":"            # If Service group changed, take the latest"},{"line_number":116,"context_line":"            elif self._services[servicename].get_groupnames() !\u003d \\"},{"line_number":117,"context_line":"                    service.get_groupnames():"},{"line_number":118,"context_line":"                self._services[servicename].add_groupname("}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_2c3427ab","line":115,"updated":"2018-11-14 17:39:48.000000000","message":"I haven\u0027t looked at this code super recently, but as long as multiple groups can be associated to a service (and since it is an add and not set group name, I would think that would be true) then the call to add_groupname() is off since get_groupnames() could return a list?  It seems like perhapbs the list of groupnames should be cleared and then added back in individually or some sort of set_groupnames() should be added to the Service object?","commit_id":"59b0b33fea60d4d5676def7540208a21fe0e073b"},{"author":{"_account_id":24147,"name":"Mark Giles","email":"mark.giles@oracle.com","username":"mark-giles"},"change_message_id":"12a7b7f3b14a24bf05b5febaf66c26599fcf66af","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                    service.is_supported():"},{"line_number":114,"context_line":"                self._services[servicename].set_supported(True)"},{"line_number":115,"context_line":"            # If Service group changed, take the latest"},{"line_number":116,"context_line":"            elif self._services[servicename].get_groupnames() !\u003d \\"},{"line_number":117,"context_line":"                    service.get_groupnames():"},{"line_number":118,"context_line":"                self._services[servicename].add_groupname("},{"line_number":119,"context_line":"                    service.get_groupnames())"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f79a3b5_ecb2af05","line":116,"updated":"2018-11-14 17:57:40.000000000","message":"I\u0027m not sure that \u0027elif\u0027 is the correct logic here since it means that groups can only get updated with the supported flag is not changed.  If several attributes change at once I don\u0027t think this logic works.","commit_id":"59b0b33fea60d4d5676def7540208a21fe0e073b"},{"author":{"_account_id":17386,"name":"Steve Noyes","email":"steve.noyes@oracle.com","username":"ssn1ssn"},"change_message_id":"132e0a1e1d870fbe3f4fd6b9f420aea72bb71791","unresolved":false,"context_lines":[{"line_number":117,"context_line":""},{"line_number":118,"context_line":"                # If Service group changed, take the latest"},{"line_number":119,"context_line":"                if self._services[servicename].get_groupnames() !\u003d \\"},{"line_number":120,"context_line":"                        service.get_groupnames():"},{"line_number":121,"context_line":"                    self._services[servicename].set_groupnames("},{"line_number":122,"context_line":"                        service.get_groupnames())"},{"line_number":123,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_bff045dd","line":120,"updated":"2018-11-15 19:16:10.000000000","message":"I think it\u0027s better to do the sort here:\n\nif self._services[servicename].get_groupnames().sort() !\u003d \\\n                        service.get_groupnames().sort()","commit_id":"cf7f5e10c468fae42253932e3bcae80ea3b7eb3b"}],"kolla_cli/common/service.py":[{"author":{"_account_id":17386,"name":"Steve Noyes","email":"steve.noyes@oracle.com","username":"ssn1ssn"},"change_message_id":"b5aefd24c5af1595bb06972d3812c7246d1d45d8","unresolved":false,"context_lines":[{"line_number":44,"context_line":"    def add_groupname(self, groupname):"},{"line_number":45,"context_line":"        if groupname is not None and groupname not in self._groupnames:"},{"line_number":46,"context_line":"            self._groupnames.append(groupname)"},{"line_number":47,"context_line":"            self._groupnames.sort()"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    def remove_groupname(self, groupname):"},{"line_number":50,"context_line":"        if groupname in self._groupnames:"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_bfbda505","line":47,"updated":"2018-11-15 19:03:12.000000000","message":"why are you sorting the groups? Do we sort anywhere else?\n\nIf I remember correctly, we do the sorting only when we are outputting a list. And that is in the presentation layer, not down in here.","commit_id":"cf7f5e10c468fae42253932e3bcae80ea3b7eb3b"},{"author":{"_account_id":17386,"name":"Steve Noyes","email":"steve.noyes@oracle.com","username":"ssn1ssn"},"change_message_id":"132e0a1e1d870fbe3f4fd6b9f420aea72bb71791","unresolved":false,"context_lines":[{"line_number":44,"context_line":"    def add_groupname(self, groupname):"},{"line_number":45,"context_line":"        if groupname is not None and groupname not in self._groupnames:"},{"line_number":46,"context_line":"            self._groupnames.append(groupname)"},{"line_number":47,"context_line":"            self._groupnames.sort()"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    def remove_groupname(self, groupname):"},{"line_number":50,"context_line":"        if groupname in self._groupnames:"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_ff1f9d08","line":47,"in_reply_to":"3f79a3b5_bfbda505","updated":"2018-11-15 19:16:10.000000000","message":"OK I see why now.","commit_id":"cf7f5e10c468fae42253932e3bcae80ea3b7eb3b"}]}