)]}'
{"tempest/api/network/test_load_balancer.py":[{"author":{"_account_id":5803,"name":"Attila Fazekas","email":"afazekas@redhat.com","username":"afazekas"},"change_message_id":"6ebf9c6aae178f6d28cd0f26b0d925d05ab46777","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        # Verification of pool delete"},{"line_number":102,"context_line":"        resp, body \u003d self.client.delete_pool(pool[\u0027id\u0027])"},{"line_number":103,"context_line":"        self.assertEqual(\u0027204\u0027, resp[\u0027status\u0027])"},{"line_number":104,"context_line":"        self.client.wait_for_resource_deletion(\u0027pool\u0027, pool[\u0027id\u0027])"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    @test.attr(type\u003d\u0027smoke\u0027)"},{"line_number":107,"context_line":"    def test_show_vip(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAU3%2F%2Fz%2Bg%3D","line":104,"updated":"2014-02-24 17:18:55.000000000","message":"Why do we need to wait for a resource deletion after 204 response ?","commit_id":"d6f35fa51a4506d0ac1ca172b4b2c29187efbd9d"},{"author":{"_account_id":9731,"name":"izikpenso","email":"izikp@radware.com","username":"izikpenso"},"change_message_id":"ef984b36ec7ba4a107bc031e70ebb9cf3f200d2d","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        # Verification of pool delete"},{"line_number":102,"context_line":"        resp, body \u003d self.client.delete_pool(pool[\u0027id\u0027])"},{"line_number":103,"context_line":"        self.assertEqual(\u0027204\u0027, resp[\u0027status\u0027])"},{"line_number":104,"context_line":"        self.client.wait_for_resource_deletion(\u0027pool\u0027, pool[\u0027id\u0027])"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    @test.attr(type\u003d\u0027smoke\u0027)"},{"line_number":107,"context_line":"    def test_show_vip(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAU3%2F%2Fzp0%3D","line":104,"in_reply_to":"AAAAU3%2F%2Fz%2Bg%3D","updated":"2014-02-24 17:44:33.000000000","message":"204 response here doesn\u0027t mean that the back-end was really deleted. Lbaas is async the 204 just says that the rest call completed successfully, Then we should check the status of the resource to verify deletion is completed.","commit_id":"d6f35fa51a4506d0ac1ca172b4b2c29187efbd9d"}],"tempest/config.py":[{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"8d487dddf70cc387a8602c4c8c5a622a5ecaaa9b","unresolved":false,"context_lines":[{"line_number":329,"context_line":"               default\u003d\"\","},{"line_number":330,"context_line":"               help\u003d\"Id of the public router that provides external \""},{"line_number":331,"context_line":"                    \"connectivity\"),"},{"line_number":332,"context_line":"    cfg.IntOpt(\u0027lbaas_entity_delete_timeout\u0027,"},{"line_number":333,"context_line":"               default\u003d10,"},{"line_number":334,"context_line":"               help\u003d\"Timeout in seconds to wait for lbaas entity deletion to \""},{"line_number":335,"context_line":"                    \"complete\"),"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAVX%2F%2F4mQ%3D","line":332,"updated":"2014-02-27 15:53:58.000000000","message":"Why do we need to add another timeout parameter? I don\u0027t see how the lbass delete wait period is any different from any other wait for deletion period.","commit_id":"5ea9939c14e1293a5905238ae050f891e2129098"},{"author":{"_account_id":8446,"name":"Evgeny Fedoruk","email":"evgenyf@radware.com","username":"evgenyfedoruk"},"change_message_id":"e4152b9d0b2c362046f164255a349b0d452a71db","unresolved":false,"context_lines":[{"line_number":329,"context_line":"               default\u003d\"\","},{"line_number":330,"context_line":"               help\u003d\"Id of the public router that provides external \""},{"line_number":331,"context_line":"                    \"connectivity\"),"},{"line_number":332,"context_line":"    cfg.IntOpt(\u0027lbaas_entity_delete_timeout\u0027,"},{"line_number":333,"context_line":"               default\u003d10,"},{"line_number":334,"context_line":"               help\u003d\"Timeout in seconds to wait for lbaas entity deletion to \""},{"line_number":335,"context_line":"                    \"complete\"),"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAVn%2F%2F7Iw%3D","line":332,"in_reply_to":"AAAAVX%2F%2F4mQ%3D","updated":"2014-03-03 11:51:24.000000000","message":"Done.\nSimilar to other projects parameters were added to network project in configuration file: build_timeout and build_interval","commit_id":"5ea9939c14e1293a5905238ae050f891e2129098"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"10d3ad53a42ae24d63a82f47035525f244500626","unresolved":false,"context_lines":[{"line_number":329,"context_line":"               default\u003d\"\","},{"line_number":330,"context_line":"               help\u003d\"Id of the public router that provides external \""},{"line_number":331,"context_line":"                    \"connectivity\"),"},{"line_number":332,"context_line":"    cfg.IntOpt(\u0027build_timeout\u0027,"},{"line_number":333,"context_line":"               default\u003d300,"},{"line_number":334,"context_line":"               help\u003d\"Timeout in seconds to wait for network operation to \""},{"line_number":335,"context_line":"                    \"complete.\"),"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2FiHg%3D","line":332,"updated":"2014-03-05 15:23:14.000000000","message":"Why do we need duplicate options. My comment on the previous revision was not regarding the name of the options, but that you were adding new duplicate timeout options.\n\nYou haven\u0027t given a compelling reason as to why we need an additional set of timeout options, just use the ones in the compute section.","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"fc3933bbea4a1ef05ad3e33c323350274fe8d7b6","unresolved":false,"context_lines":[{"line_number":329,"context_line":"               default\u003d\"\","},{"line_number":330,"context_line":"               help\u003d\"Id of the public router that provides external \""},{"line_number":331,"context_line":"                    \"connectivity\"),"},{"line_number":332,"context_line":"    cfg.IntOpt(\u0027build_timeout\u0027,"},{"line_number":333,"context_line":"               default\u003d300,"},{"line_number":334,"context_line":"               help\u003d\"Timeout in seconds to wait for network operation to \""},{"line_number":335,"context_line":"                    \"complete.\"),"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2Fg%2Fg%3D","line":332,"in_reply_to":"AAAAVn%2F%2FhxY%3D","updated":"2014-03-05 17:05:19.000000000","message":"No that\u0027s fine I guess, especially if the network resources have different operational times. But there needs to be a reason in the commit message.(which I said in my previous review) Something like: \"new network specific timeout options are added to give finer grain control over network resource wait periods\" Just saying that you\u0027re adding new options without given a reason isn\u0027t ok.","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"},{"author":{"_account_id":8446,"name":"Evgeny Fedoruk","email":"evgenyf@radware.com","username":"evgenyfedoruk"},"change_message_id":"6cc2bb19ac45435d52b77f12a08418687fc8ed70","unresolved":false,"context_lines":[{"line_number":329,"context_line":"               default\u003d\"\","},{"line_number":330,"context_line":"               help\u003d\"Id of the public router that provides external \""},{"line_number":331,"context_line":"                    \"connectivity\"),"},{"line_number":332,"context_line":"    cfg.IntOpt(\u0027build_timeout\u0027,"},{"line_number":333,"context_line":"               default\u003d300,"},{"line_number":334,"context_line":"               help\u003d\"Timeout in seconds to wait for network operation to \""},{"line_number":335,"context_line":"                    \"complete.\"),"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2FhxY%3D","line":332,"in_reply_to":"AAAAVn%2F%2FiHg%3D","updated":"2014-03-05 16:35:53.000000000","message":"I added new options for network section for being able to control those values just for network. Shouldn\u0027t we give this kind of flexibility in options? Compute, orchestration and volume have their own options. Values might be different for each one. \nIf this reason is not justified, I will use compute options.","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"}],"tempest/services/network/network_client_base.py":[{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"10d3ad53a42ae24d63a82f47035525f244500626","unresolved":false,"context_lines":[{"line_number":199,"context_line":"    def wait_for_resource_deletion(self, resource_type, id):"},{"line_number":200,"context_line":"        \"\"\"Waits for a resource to be deleted.\"\"\""},{"line_number":201,"context_line":"        start_time \u003d int(time.time())"},{"line_number":202,"context_line":"        while True:"},{"line_number":203,"context_line":"            if self.is_resource_deleted(resource_type, id):"},{"line_number":204,"context_line":"                return"},{"line_number":205,"context_line":"            if int(time.time()) - start_time \u003e\u003d self.build_timeout:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2FiDU%3D","line":202,"updated":"2014-03-05 15:23:14.000000000","message":"I\u0027m not sure why you need to duplicate all of this code, you\u0027ve just overloaded the method but you still have super() available. If you want to do it this way, why not just set self.is_resource_deleted to a private method which is resource specific and call the super(). So it\u0027d be something like:\n\ndef wait_for_resource_deletion(self, resource_type, id):\n    if resource_type \u003d\u003d XXX:\n        self.is_resource_deleted \u003d self._is_XXX_deleted\n    super(NetworkClientBase, self).wait_for_resource_deletion(id)\n\nI can think of some other ways to do this too, probably without having to use an if, else block for everything too. (maybe a class or method that generates the resource specific deletion check method) \n\nBut, I\u0027m still unclear on why you would go with this approach instead of splitting the network client into resource specific subclasses. (which would have to be dependent change in front of this) That just seems like the cleaner solution here.","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"fc3933bbea4a1ef05ad3e33c323350274fe8d7b6","unresolved":false,"context_lines":[{"line_number":199,"context_line":"    def wait_for_resource_deletion(self, resource_type, id):"},{"line_number":200,"context_line":"        \"\"\"Waits for a resource to be deleted.\"\"\""},{"line_number":201,"context_line":"        start_time \u003d int(time.time())"},{"line_number":202,"context_line":"        while True:"},{"line_number":203,"context_line":"            if self.is_resource_deleted(resource_type, id):"},{"line_number":204,"context_line":"                return"},{"line_number":205,"context_line":"            if int(time.time()) - start_time \u003e\u003d self.build_timeout:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2FhBg%3D","line":202,"in_reply_to":"AAAAVn%2F%2FhJY%3D","updated":"2014-03-05 17:05:19.000000000","message":"Oh my mistake, the commit message said you overloaded the function so I just assumed you put this in the network client without actually checking the file. This belongs in the client anyway, so just put this in the network client and then you don\u0027t have to duplicate the wait code.\n\nHonestly I\u0027d rather see this done the right way, but I\u0027m fine with doing this approach if it\u0027s only temporary.","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"},{"author":{"_account_id":8446,"name":"Evgeny Fedoruk","email":"evgenyf@radware.com","username":"evgenyfedoruk"},"change_message_id":"6cc2bb19ac45435d52b77f12a08418687fc8ed70","unresolved":false,"context_lines":[{"line_number":199,"context_line":"    def wait_for_resource_deletion(self, resource_type, id):"},{"line_number":200,"context_line":"        \"\"\"Waits for a resource to be deleted.\"\"\""},{"line_number":201,"context_line":"        start_time \u003d int(time.time())"},{"line_number":202,"context_line":"        while True:"},{"line_number":203,"context_line":"            if self.is_resource_deleted(resource_type, id):"},{"line_number":204,"context_line":"                return"},{"line_number":205,"context_line":"            if int(time.time()) - start_time \u003e\u003d self.build_timeout:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAVn%2F%2FhJY%3D","line":202,"in_reply_to":"AAAAVn%2F%2FiDU%3D","updated":"2014-03-05 16:35:53.000000000","message":"NetworkClientBase does not currently inherit from common RestClient so I can\u0027t use super().\nI agree, proper solution is to split client for resources. It seems to me as a change that will take time to implement and pass review circles, meanwhile this change is critical for getting 3rd party testing work properly.\nI opened a bug for the issue (#1288326)","commit_id":"0072eb3926bd867354abe057280e1c4e5072240b"}]}