)]}'
{"cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"5f4beca584affa2235e080ef40116e2b853ca885","unresolved":true,"context_lines":[{"line_number":12736,"context_line":"                                               [self.rep_target])"},{"line_number":12737,"context_line":"        self.driver.do_setup(self.ctxt)"},{"line_number":12738,"context_line":""},{"line_number":12739,"context_line":"        if vol_spec[\u0027volume_type\u0027] \u003d\u003d \u0027mm\u0027:"},{"line_number":12740,"context_line":"            vol_type \u003d self.mm_type"},{"line_number":12741,"context_line":"        if vol_spec[\u0027volume_type\u0027] \u003d\u003d \u0027gm\u0027:"},{"line_number":12742,"context_line":"            vol_type \u003d self.gm_type"},{"line_number":12743,"context_line":"        if vol_spec[\u0027volume_type\u0027] \u003d\u003d \u0027gmcv\u0027:"},{"line_number":12744,"context_line":"            vol_type \u003d self.gmcv_default_type"},{"line_number":12745,"context_line":"        if vol_spec[\u0027volume_type\u0027] \u003d\u003d \u0027non-rep\u0027:"},{"line_number":12746,"context_line":"            vol_type \u003d self.non_replica_type"},{"line_number":12747,"context_line":""},{"line_number":12748,"context_line":"        # Create metro mirror replication."},{"line_number":12749,"context_line":"        volume, model_update \u003d self._create_test_volume(vol_type)"}],"source_content_type":"text/x-python","patch_set":4,"id":"6021471b_22d3c3ed","line":12746,"range":{"start_line":12739,"start_character":0,"end_line":12746,"end_character":44},"updated":"2021-08-17 13:29:00.000000000","message":"if you change the ddt data like this:\n\n    @ddt.data(({\u0027volume_type\u0027: \u0027mm_type\u0027}),\n              ({\u0027volume_type\u0027: \u0027gm_type\u0027}),\n              ({\u0027volume_type\u0027: \u0027gmcv_default_type\u0027}),\n              ({\u0027volume_type\u0027: \u0027non_replica_type\u0027})\n              )\n\nthen you can replace all this stuff with\n\n        vol_type \u003d getattr(self, vol_spec[\u0027volume_type\u0027])\n\nwhich I think is much more clear.  Plus, I really dislike logic inside unit tests, because how do you know you\u0027ve got it right?\n\nThis is a minor point, though.  See my comment in storwize_svc_common.py.  You\u0027ll need to revise the below part of the test.","commit_id":"263938c70b3dcf020432c89766f88a54d9c89ca2"}],"cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"aa4a8487a65844acbc5fa99612180a646f3fa049","unresolved":true,"context_lines":[{"line_number":3384,"context_line":"                    del model_update[\u0027metadata\u0027][key]"},{"line_number":3385,"context_line":"        else:"},{"line_number":3386,"context_line":"            for key, value in rep_properties.items():"},{"line_number":3387,"context_line":"                if not rel_info[\u0027sync\u0027] or rel_info.get(value):"},{"line_number":3388,"context_line":"                    model_update[\u0027metadata\u0027][key] \u003d rel_info[value]"},{"line_number":3389,"context_line":"        return model_update"},{"line_number":3390,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"116a523b_5165102c","line":3387,"range":{"start_line":3387,"start_character":16,"end_line":3387,"end_character":63},"updated":"2021-08-02 15:04:29.000000000","message":"I\u0027m not clear how this fixes the problem described in the bug.  What the old code was supposed to be doing is check to see if value is in rel_info, and if so, update the metadata.  So why not just check this:\n\n  if value in rel_info:","commit_id":"0ea313d6027e56d1db0218c8499fb4e046d2d8a7"},{"author":{"_account_id":32036,"name":"katari manoj kumar","email":"katkumar@in.ibm.com","username":"katarimanojkumar"},"change_message_id":"07a4739d5ab80b0a6420a396be285776081544a6","unresolved":true,"context_lines":[{"line_number":3384,"context_line":"                    del model_update[\u0027metadata\u0027][key]"},{"line_number":3385,"context_line":"        else:"},{"line_number":3386,"context_line":"            for key, value in rep_properties.items():"},{"line_number":3387,"context_line":"                if not rel_info[\u0027sync\u0027] or rel_info.get(value):"},{"line_number":3388,"context_line":"                    model_update[\u0027metadata\u0027][key] \u003d rel_info[value]"},{"line_number":3389,"context_line":"        return model_update"},{"line_number":3390,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"43ef995e_a029085a","line":3387,"range":{"start_line":3387,"start_character":16,"end_line":3387,"end_character":63},"in_reply_to":"116a523b_5165102c","updated":"2021-08-03 08:54:37.000000000","message":"\u0027Sync\u0027 is one of the replicatoin parameters which behaves differently compared to others.\npossible values for the parameter in SVF are \u0027outofsyc\u0027, \u0027\u0027 (empty in case of full synched volume).\n\nConsider a case where sync is updated with \u0027outofsync\u0027 initially and next refresh metadata would fetch it as \u0027\u0027(empty). if we restrict empty values of sync atribute. it will never be updated.","commit_id":"0ea313d6027e56d1db0218c8499fb4e046d2d8a7"},{"author":{"_account_id":32171,"name":"Girish Chilukuri","email":"girish.chilukuri@ibm.com","username":"GirishChilukuri"},"change_message_id":"dc61ac0a03d30cab88d92c2d8e721c704a833e8a","unresolved":true,"context_lines":[{"line_number":3384,"context_line":"                    del model_update[\u0027metadata\u0027][key]"},{"line_number":3385,"context_line":"        else:"},{"line_number":3386,"context_line":"            for key, value in rep_properties.items():"},{"line_number":3387,"context_line":"                if not rel_info[\u0027sync\u0027] or rel_info.get(value):"},{"line_number":3388,"context_line":"                    model_update[\u0027metadata\u0027][key] \u003d rel_info[value]"},{"line_number":3389,"context_line":"        return model_update"},{"line_number":3390,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ce363a86_7ba5dbf4","line":3387,"range":{"start_line":3387,"start_character":32,"end_line":3387,"end_character":38},"updated":"2021-08-10 17:22:08.000000000","message":"only \"sync\" attribute\u0027s value will be empty or is it possible that any other attribute\u0027s value can be empty?","commit_id":"263938c70b3dcf020432c89766f88a54d9c89ca2"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"5f4beca584affa2235e080ef40116e2b853ca885","unresolved":true,"context_lines":[{"line_number":3384,"context_line":"                    del model_update[\u0027metadata\u0027][key]"},{"line_number":3385,"context_line":"        else:"},{"line_number":3386,"context_line":"            for key, value in rep_properties.items():"},{"line_number":3387,"context_line":"                if not rel_info[\u0027sync\u0027] or rel_info.get(value):"},{"line_number":3388,"context_line":"                    model_update[\u0027metadata\u0027][key] \u003d rel_info[value]"},{"line_number":3389,"context_line":"        return model_update"},{"line_number":3390,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"d09d1c24_e87c1d42","line":3387,"range":{"start_line":3387,"start_character":32,"end_line":3387,"end_character":38},"in_reply_to":"ce363a86_7ba5dbf4","updated":"2021-08-17 13:29:00.000000000","message":"(1) It would be helpful to have a comment here about how \u0027sync\u0027 behaves so that it\u0027s clear why this is being checked, and also (as indicated in your answer to Girish) that you are intentionally ignoring any other parameters that have an empty string as a value.\n\n(2) More importantly, however, this condition is not correct.  Suppose\n  rel_info[\u0027sync\u0027] \u003d\u003d \u0027\u0027\n\nThen the right side of the disjunction will never be checked in any iteration of the loop, and if the value is not a key in rel_info, you will get a KeyError at line 3388.  And if the value *is* a key but has an empty value, then you\u0027ll be setting it in the metadata, contrary to what you said you\u0027re supposed to be doing.  (This also indicates that your unit test is not very robust, because you did not detect this yourself.)\n\n(3) Further, the problem with line 3387 in the base patch is, as you say, that if rel_info.get(value) returns an empty string, the condition is False.  That\u0027s why I suggested only checking to see\n  if value in rel_info:\n\nbecause the above condition only looks at whether the key (which is confusingly named \u0027value\u0027 here) is there or not.  This way you know there will be no key errors when you access rel_info[value] later, and you can take that into account in checking whatever needs to be checked to get this correct.","commit_id":"263938c70b3dcf020432c89766f88a54d9c89ca2"},{"author":{"_account_id":32036,"name":"katari manoj kumar","email":"katkumar@in.ibm.com","username":"katarimanojkumar"},"change_message_id":"a9766b4d446f4cfb25d09191266e543cb5054370","unresolved":true,"context_lines":[{"line_number":3384,"context_line":"                    del model_update[\u0027metadata\u0027][key]"},{"line_number":3385,"context_line":"        else:"},{"line_number":3386,"context_line":"            for key, value in rep_properties.items():"},{"line_number":3387,"context_line":"                if not rel_info[\u0027sync\u0027] or rel_info.get(value):"},{"line_number":3388,"context_line":"                    model_update[\u0027metadata\u0027][key] \u003d rel_info[value]"},{"line_number":3389,"context_line":"        return model_update"},{"line_number":3390,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"e38c5a8d_ed13676f","line":3387,"range":{"start_line":3387,"start_character":32,"end_line":3387,"end_character":38},"in_reply_to":"d09d1c24_e87c1d42","updated":"2021-08-18 12:11:34.000000000","message":"Thanks Brian for detailed analysis, we agree that condition is wrong.\n\nwe have analyzed all the parameters of lsrcrealtionship in the storage, there can be other attributes which can empty at some point of time.\nFor few attributes like sync, even the empty value indicates successful synchronization.\n\nIt is better to remove the line 3387 completely so that the attributes data from the storage is updated as it is, without any \u0027empty\u0027 checks.\n\nThis would also need change in commit message, release notes, and UT.","commit_id":"263938c70b3dcf020432c89766f88a54d9c89ca2"}]}