)]}'
{"tempest/api/volume/v2/test_volumes_list.py":[{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"d1887b1650dd57bfd4f2ca701d3132026eccc9c9","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from six.moves.urllib import parse as urllib"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from tempest.api.volume import base"},{"line_number":20,"context_line":"from tempest import test"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_ec803bc1","line":17,"updated":"2015-06-25 08:50:52.000000000","message":"This is a bad idea. You can\"t rename urllib.parse into urllib. You should just:\n  from six.moves.urllib import parse\n\nThen:\n  parse.urlparse","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f63ce21e675f949d799d87aca3680e6b6f2ab792","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from six.moves.urllib import parse as urllib"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from tempest.api.volume import base"},{"line_number":20,"context_line":"from tempest import test"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_daa08992","line":17,"in_reply_to":"fa32b979_ec803bc1","updated":"2015-06-25 09:46:02.000000000","message":"I don\u0027t usually do it this way, but when in Rome do as Romans do:\n\n https://github.com/openstack/tempest/blob/master/tempest/services/compute/json/agents_client.py#L17\n https://github.com/openstack/tempest/blob/master/tempest/services/compute/json/images_client.py#L18\n https://github.com/openstack/tempest/blob/master/tempest/services/compute/json/volumes_extensions_client.py#L19\n\nI\u0027ll change it","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"d1887b1650dd57bfd4f2ca701d3132026eccc9c9","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        # Get list method for the type of resource from the client"},{"line_number":108,"context_line":"        method \u003d getattr(self.client, \u0027list_\u0027 + resource)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"        # Include limit in params for list request"},{"line_number":111,"context_line":"        kwargs \u003d dict(kwargs)"},{"line_number":112,"context_line":"        params \u003d kwargs.pop(\u0027params\u0027, {})"},{"line_number":113,"context_line":"        params[\u0027limit\u0027] \u003d limit"},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_ec57db57","line":111,"range":{"start_line":110,"start_character":17,"end_line":111,"end_character":29},"updated":"2015-06-25 08:50:52.000000000","message":"Isn\u0027t this line useless ? kwargs is bound to be a dict right ?","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f63ce21e675f949d799d87aca3680e6b6f2ab792","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        method \u003d getattr(self.client, \u0027list_\u0027 + resource)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"        # Include limit in params for list request"},{"line_number":111,"context_line":"        kwargs \u003d dict(kwargs)"},{"line_number":112,"context_line":"        params \u003d kwargs.pop(\u0027params\u0027, {})"},{"line_number":113,"context_line":"        params[\u0027limit\u0027] \u003d limit"},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_da8649b8","line":111,"in_reply_to":"fa32b979_ec57db57","updated":"2015-06-25 09:46:02.000000000","message":"Yes it is. The idea was to do a shallow copy of kwargs as not to modify the original on the next pop.  But that\u0027s just because I was thinking of kwargs as a dictionary argument passed by the caller instead of what it really is.  So like you said, no need to copy the dictionary.","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"d1887b1650dd57bfd4f2ca701d3132026eccc9c9","unresolved":false,"context_lines":[{"line_number":135,"context_line":"                                    \u0027#received\u0027: len(response[resource])})"},{"line_number":136,"context_line":""},{"line_number":137,"context_line":"                # For each received element"},{"line_number":138,"context_line":"                for i in range(num_expected):"},{"line_number":139,"context_line":"                    element \u003d response[resource][i][\u0027id\u0027]"},{"line_number":140,"context_line":"                    # Check it\u0027s one of expected ids"},{"line_number":141,"context_line":"                    self.assertIn(element,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_6c2d0ba7","line":138,"updated":"2015-06-25 08:50:52.000000000","message":"You should use:\n  for element in response[resource]:\n    element_id \u003d element[\u0027id\u0027]\n\nI meant: you don\u0027t need this \u0027i\u0027 variable.","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f63ce21e675f949d799d87aca3680e6b6f2ab792","unresolved":false,"context_lines":[{"line_number":135,"context_line":"                                    \u0027#received\u0027: len(response[resource])})"},{"line_number":136,"context_line":""},{"line_number":137,"context_line":"                # For each received element"},{"line_number":138,"context_line":"                for i in range(num_expected):"},{"line_number":139,"context_line":"                    element \u003d response[resource][i][\u0027id\u0027]"},{"line_number":140,"context_line":"                    # Check it\u0027s one of expected ids"},{"line_number":141,"context_line":"                    self.assertIn(element,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_9aa3e142","line":138,"in_reply_to":"fa32b979_6c2d0ba7","updated":"2015-06-25 09:46:02.000000000","message":"Yup, I don\u0027t know what I was thinking that day. ;-)","commit_id":"33f29d2ffae1d5206ef60b77bd27ab4f0621c89e"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"f458c48a9c1803aa1545122cb52c77aeb24598d2","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            # If we have to check ids"},{"line_number":129,"context_line":"            if remaining is not None:"},{"line_number":130,"context_line":"                # Confirm we receive expected number of elements"},{"line_number":131,"context_line":"                num_expected \u003d min(len(remaining), limit)"},{"line_number":132,"context_line":"                self.assertEqual(num_expected, len(response[resource]),"},{"line_number":133,"context_line":"                                 \u0027Requested %(#expect)d but got %(#received)d \u0027"},{"line_number":134,"context_line":"                                 % {\u0027#expect\u0027: num_expected,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_375c4a4a","line":131,"updated":"2015-07-09 03:18:40.000000000","message":"Actually that\u0027s not quit true, return element can be limited with configuration value \"osapi_max_limit\".\n\nBut Devstack use its default value which is quite high(1000) and Tempest creating only 3 volumes. So for gate it is perfectly fine but I am wondering if any Cloud having very less value for max limit (less than 3) ? If so then its going to break even other existing tests too.\n\nAnyways as of now we can ignore this point due to this patch scope. I will discuss this more and change volume list tests if needed.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9d2f3fdfbe9742f66ef574218710fd52443aa01c","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            # If we have to check ids"},{"line_number":129,"context_line":"            if remaining is not None:"},{"line_number":130,"context_line":"                # Confirm we receive expected number of elements"},{"line_number":131,"context_line":"                num_expected \u003d min(len(remaining), limit)"},{"line_number":132,"context_line":"                self.assertEqual(num_expected, len(response[resource]),"},{"line_number":133,"context_line":"                                 \u0027Requested %(#expect)d but got %(#received)d \u0027"},{"line_number":134,"context_line":"                                 % {\u0027#expect\u0027: num_expected,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_ba04ac17","line":131,"in_reply_to":"ba3cc151_375c4a4a","updated":"2015-07-09 12:10:57.000000000","message":"I already explained that in general a comment on this patch, and I highly doubt anyone will have a deployment with a value of 3 or lower, people that change default value actually increase it.  ;-)","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"56dc70a46a6064ce2915bf80071d3134cd70a712","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            # If we have to check ids"},{"line_number":129,"context_line":"            if remaining is not None:"},{"line_number":130,"context_line":"                # Confirm we receive expected number of elements"},{"line_number":131,"context_line":"                num_expected \u003d min(len(remaining), limit)"},{"line_number":132,"context_line":"                self.assertEqual(num_expected, len(response[resource]),"},{"line_number":133,"context_line":"                                 \u0027Requested %(#expect)d but got %(#received)d \u0027"},{"line_number":134,"context_line":"                                 % {\u0027#expect\u0027: num_expected,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_7accb98a","line":131,"in_reply_to":"ba3cc151_ba04ac17","updated":"2015-07-10 08:28:07.000000000","message":"Yes, I agree on that. if some cases comes then we can consider that config option.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"f458c48a9c1803aa1545122cb52c77aeb24598d2","unresolved":false,"context_lines":[{"line_number":163,"context_line":""},{"line_number":164,"context_line":"            # Check if we have next and we shouldn\u0027t or the other way around"},{"line_number":165,"context_line":"            if remaining is not None:"},{"line_number":166,"context_line":"                if remaining or (num_expected and len(ids) % limit \u003d\u003d 0):"},{"line_number":167,"context_line":"                    self.assertIsNotNone(next, \u0027Missing link to next page\u0027)"},{"line_number":168,"context_line":"                else:"},{"line_number":169,"context_line":"                    self.assertIsNone(next, \u0027Unexpected link to next page\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_2242caae","line":166,"updated":"2015-07-09 03:18:40.000000000","message":"I did not get second condition (num_expected and len(ids) % limit \u003d\u003d 0).\n\nWhy we cannot just check  (len(ids) \u003d\u003d limit)? or \"remainig\" itself check this condition so we can even remove the second condition.\n\nBasically next link will be present in below 3 conditions-\n\n        1) limit is equal to or less than the total number of items.\n        2) limit and total number of items are greater than CONF.osapi_max_limit.\n        3) limit is not specified and the total number of items is greater than \n            CONF.osapi_max_limit.\n\nSo here we are checking only first condition as we are not considering CONF.osapi_max_limit in our volume list tests.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9d2f3fdfbe9742f66ef574218710fd52443aa01c","unresolved":false,"context_lines":[{"line_number":163,"context_line":""},{"line_number":164,"context_line":"            # Check if we have next and we shouldn\u0027t or the other way around"},{"line_number":165,"context_line":"            if remaining is not None:"},{"line_number":166,"context_line":"                if remaining or (num_expected and len(ids) % limit \u003d\u003d 0):"},{"line_number":167,"context_line":"                    self.assertIsNotNone(next, \u0027Missing link to next page\u0027)"},{"line_number":168,"context_line":"                else:"},{"line_number":169,"context_line":"                    self.assertIsNone(next, \u0027Unexpected link to next page\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_75b875c8","line":166,"in_reply_to":"ba3cc151_2242caae","updated":"2015-07-09 12:10:57.000000000","message":"That check is to cover case 1, because items is not total number of items but returned items.  So when we reach the last pagination if we have returned as many elements as limit we still need to give a next link, even if that next page will be empty.  \n\nAnd using len(ids) \u003d\u003d limit is only one of the cases where we\u0027ll have a next link.  It will not cover the case were have 3 elements and limit is 1; in that case we\u0027ll have 4 pages and 3 links, but with that check we\u0027ll only expect 3 pages and 2 links.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"56dc70a46a6064ce2915bf80071d3134cd70a712","unresolved":false,"context_lines":[{"line_number":163,"context_line":""},{"line_number":164,"context_line":"            # Check if we have next and we shouldn\u0027t or the other way around"},{"line_number":165,"context_line":"            if remaining is not None:"},{"line_number":166,"context_line":"                if remaining or (num_expected and len(ids) % limit \u003d\u003d 0):"},{"line_number":167,"context_line":"                    self.assertIsNotNone(next, \u0027Missing link to next page\u0027)"},{"line_number":168,"context_line":"                else:"},{"line_number":169,"context_line":"                    self.assertIsNone(next, \u0027Unexpected link to next page\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_d1fb637a","line":166,"in_reply_to":"ba3cc151_75b875c8","updated":"2015-07-10 08:28:07.000000000","message":"yea, that right. I got now. Thanks for clarifying.\n\nI tested many scenario on my local env and its working fine.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"}],"tempest/services/volume/json/volumes_client.py":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"f458c48a9c1803aa1545122cb52c77aeb24598d2","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            url +\u003d \u0027?%s\u0027 % self._prepare_params(params)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        key \u003d None if return_body else \u0027volumes\u0027"},{"line_number":94,"context_line":"        return self._ext_get(url, key)"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    def show_volume(self, volume_id):"},{"line_number":97,"context_line":"        \"\"\"Returns the details of a single volume.\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_e29142f2","line":94,"updated":"2015-07-09 03:18:40.000000000","message":"humm, That actually not good as service clients going to be in lib. All method should return complete response as they get from API.\n\nBut I will not block this due to that fact as that decision is under spec and needs to be implemented other place too.\n\nAs of now m ok with his.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9d2f3fdfbe9742f66ef574218710fd52443aa01c","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            url +\u003d \u0027?%s\u0027 % self._prepare_params(params)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        key \u003d None if return_body else \u0027volumes\u0027"},{"line_number":94,"context_line":"        return self._ext_get(url, key)"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    def show_volume(self, volume_id):"},{"line_number":97,"context_line":"        \"\"\"Returns the details of a single volume.\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_10f7b7d3","line":94,"in_reply_to":"ba3cc151_e29142f2","updated":"2015-07-09 12:10:57.000000000","message":"Well, if you look at current code you\u0027ll see that it\u0027s not returning the whole body right now, so I preserved current behavior and I just added the possibility to return the body, which is precisely what the spec you mention wants.  When that spec lands this just needs to change return_body\u003dTrue and make changes in callers that don\u0027t want whole body.","commit_id":"0ea58afb6af962d1a20f39a2dd0020e3fd322e09"}]}