)]}'
{"trove/extensions/security_group/models.py":[{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"a867fd6ca520ca5346d9f2e4b0b73387b7b0363c","unresolved":false,"context_lines":[{"line_number":169,"context_line":"            raise e"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    @classmethod"},{"line_number":172,"context_line":"    def get_security_group(self, group_id, tenant_id):"},{"line_number":173,"context_line":"        return SecurityGroup.find_by(id\u003dgroup_id,"},{"line_number":174,"context_line":"                                     tenant_id\u003dtenant_id,"},{"line_number":175,"context_line":"                                     deleted\u003dFalse)"},{"line_number":176,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1ae5cdf2_b831dba3","line":173,"range":{"start_line":172,"start_character":0,"end_line":173,"end_character":0},"updated":"2014-06-13 03:39:20.000000000","message":"Why make this a class method instead of leaving it as a model association between a SecGroupRule object and its corresponding SecGroup object?\n\nWe shouldn\u0027t even need to touch this piece of code.","commit_id":"67e53482a167f4e8a543c1a8428fec152c316917"}],"trove/extensions/security_group/service.py":[{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"a54d9e82fc9f3515c4828e676d7a4bcf70255ff8","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        context \u003d req.environ[wsgi.CONTEXT_KEY]"},{"line_number":70,"context_line":"        sec_group_rule \u003d models.SecurityGroupRule.find_by(id\u003did, deleted\u003dFalse)"},{"line_number":71,"context_line":"        sec_group \u003d models.SecurityGroup.find_by("},{"line_number":72,"context_line":"            context, id\u003dsec_group_rule.group_id)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if sec_group is None:"},{"line_number":75,"context_line":"            LOG.error(\"Attempting to delete Group Rule that does not exist or \""}],"source_content_type":"text/x-python","patch_set":4,"id":"1ae5cdf2_9f3dc5df","line":72,"updated":"2014-06-06 07:29:10.000000000","message":"Is there a reason to remove this layer of abstraction in the model? \n\nIt has nothing to do with updating the modified time of the sec group.","commit_id":"2458c7ef0d936ff08557d04dac81ccb652f43e9c"},{"author":{"_account_id":8415,"name":"Denis M.","email":"lildee1991@gmail.com","username":"dmakogon"},"change_message_id":"882a5bff8d92532e36fd5c25c74e966b554e3ada","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        context \u003d req.environ[wsgi.CONTEXT_KEY]"},{"line_number":70,"context_line":"        sec_group_rule \u003d models.SecurityGroupRule.find_by(id\u003did, deleted\u003dFalse)"},{"line_number":71,"context_line":"        sec_group \u003d models.SecurityGroup.find_by("},{"line_number":72,"context_line":"            context, id\u003dsec_group_rule.group_id)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if sec_group is None:"},{"line_number":75,"context_line":"            LOG.error(\"Attempting to delete Group Rule that does not exist or \""}],"source_content_type":"text/x-python","patch_set":4,"id":"1ae5cdf2_b6f54384","line":72,"in_reply_to":"1ae5cdf2_9f3dc5df","updated":"2014-06-06 12:07:57.000000000","message":"There was no need to define one more method that contains only return statement\nhttps://review.openstack.org/#/c/97510/4/trove/extensions/security_group/models.py,cm\n\ni decided to retrieve security group model explicitly. This small change is simpler than previous.","commit_id":"2458c7ef0d936ff08557d04dac81ccb652f43e9c"},{"author":{"_account_id":8214,"name":"Auston McReynolds","email":"amcreynolds@ebaysf.com","username":"amcreynolds"},"change_message_id":"911f00b0b70cb2e4d1531066a27301e2529b3aa0","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                raise exception.BadRequest(msg\u003dstr(e))"},{"line_number":112,"context_line":"            return rules"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        sec_group.save()"},{"line_number":115,"context_line":"        tcp_rules \u003d _create_rules(sec_group, tcp_ports, \u0027tcp\u0027)"},{"line_number":116,"context_line":"        udp_rules \u003d _create_rules(sec_group, udp_ports, \u0027udp\u0027)"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1ae5cdf2_f028cae3","line":114,"updated":"2014-06-05 18:26:29.000000000","message":"not to be a stickler meeseeks, but shouldn\u0027t this be right below the two _create_rules(...) invocations below? otherwise it\u0027s quite possible that no rules get created (due to an exception on the first one), yet we update the security-group updated field.","commit_id":"2458c7ef0d936ff08557d04dac81ccb652f43e9c"},{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"a54d9e82fc9f3515c4828e676d7a4bcf70255ff8","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                raise exception.BadRequest(msg\u003dstr(e))"},{"line_number":112,"context_line":"            return rules"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        sec_group.save()"},{"line_number":115,"context_line":"        tcp_rules \u003d _create_rules(sec_group, tcp_ports, \u0027tcp\u0027)"},{"line_number":116,"context_line":"        udp_rules \u003d _create_rules(sec_group, udp_ports, \u0027udp\u0027)"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1ae5cdf2_ff9169f1","line":114,"in_reply_to":"1ae5cdf2_f028cae3","updated":"2014-06-06 07:29:10.000000000","message":"Not being a stickler at all, methinks; makes sense to set the updated time in the model only once the rules have been updated.","commit_id":"2458c7ef0d936ff08557d04dac81ccb652f43e9c"},{"author":{"_account_id":8415,"name":"Denis M.","email":"lildee1991@gmail.com","username":"dmakogon"},"change_message_id":"1587d56f7e30fde33af8bac21a49478ff19ca3e7","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                raise exception.BadRequest(msg\u003dstr(e))"},{"line_number":112,"context_line":"            return rules"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        sec_group.save()"},{"line_number":115,"context_line":"        tcp_rules \u003d _create_rules(sec_group, tcp_ports, \u0027tcp\u0027)"},{"line_number":116,"context_line":"        udp_rules \u003d _create_rules(sec_group, udp_ports, \u0027udp\u0027)"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1ae5cdf2_f0d8c5f0","line":114,"in_reply_to":"1ae5cdf2_f028cae3","updated":"2014-06-05 21:15:36.000000000","message":"agreed, thanks for catch","commit_id":"2458c7ef0d936ff08557d04dac81ccb652f43e9c"},{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"cc14675c8bfd0568fae7265b0c5ce02ac3e4bac7","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        context \u003d req.environ[wsgi.CONTEXT_KEY]"},{"line_number":70,"context_line":"        sec_group_rule \u003d models.SecurityGroupRule.find_by(id\u003did, deleted\u003dFalse)"},{"line_number":71,"context_line":"        sec_group \u003d models.SecurityGroup.find_by("},{"line_number":72,"context_line":"            context, id\u003dsec_group_rule.group_id)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if sec_group is None:"},{"line_number":75,"context_line":"            LOG.error(\"Attempting to delete Group Rule that does not exist or \""}],"source_content_type":"text/x-python","patch_set":5,"id":"1ae5cdf2_5572b9e3","line":72,"range":{"start_line":72,"start_character":24,"end_line":72,"end_character":38},"updated":"2014-06-10 23:12:48.000000000","message":"I think the code reads better the way it is and doesn\u0027t break model abstraction. \n\nI\u0027d suggest we leave the code the way it is, especially given that it has nothing to do with updating the modified timestamp, which this bugfix is supposed to address.","commit_id":"d15a0c65026c377b0cf0253176a90120bdc05525"},{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"a867fd6ca520ca5346d9f2e4b0b73387b7b0363c","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        context \u003d req.environ[wsgi.CONTEXT_KEY]"},{"line_number":70,"context_line":"        sec_group_rule \u003d models.SecurityGroupRule.find_by(id\u003did, deleted\u003dFalse)"},{"line_number":71,"context_line":"        sec_group \u003d models.SecurityGroupRule.get_security_group("},{"line_number":72,"context_line":"            sec_group_rule.group_id, tenant_id)"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if sec_group is None:"},{"line_number":75,"context_line":"            LOG.error(\"Attempting to delete Group Rule that does not exist or \""}],"source_content_type":"text/x-python","patch_set":7,"id":"1ae5cdf2_5818d71d","line":72,"updated":"2014-06-13 03:39:20.000000000","message":"Same here.","commit_id":"67e53482a167f4e8a543c1a8428fec152c316917"}],"trove/tests/api/instances.py":[{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"8bfef8289a0cb3fb040c7cdfefca8e2c157f5978","unresolved":false,"context_lines":[{"line_number":853,"context_line":"            assert_equal(rule[\u0027from_port\u0027], 3306)"},{"line_number":854,"context_line":"            assert_equal(rule[\u0027to_port\u0027], 3306)"},{"line_number":855,"context_line":"            assert_is_not_none(rule[\u0027created\u0027])"},{"line_number":856,"context_line":"            global rule_id"},{"line_number":857,"context_line":"            rule_id \u003d rule[\u0027id\u0027]"},{"line_number":858,"context_line":""},{"line_number":859,"context_line":"    @test(depends_on\u003d[test_create_security_group_rule])"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ae5cdf2_5571d9dc","line":856,"updated":"2014-06-18 08:46:49.000000000","message":"Having globals means that we won\u0027t be able to parallelize such tests. Can we come up with a set of tests that don\u0027t involve inter dependencies between tests, and which can be parallelized in the future?","commit_id":"46b1c6a2d5a76a15bcbf30881442a4ec17728593"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"fc3ac512a334a51697ed8e288f3cdb4336abf2c6","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    @test"},{"line_number":838,"context_line":"    def test_create_security_group_rule(self):"},{"line_number":839,"context_line":"        # Need to sleep to verify created/updated timestamps"},{"line_number":840,"context_line":"        time.sleep(20)"},{"line_number":841,"context_line":"        cidr \u003d \"1.2.3.4/16\""},{"line_number":842,"context_line":"        self.testSecurityGroupRules \u003d ("},{"line_number":843,"context_line":"            dbaas.security_group_rules.create("}],"source_content_type":"text/x-python","patch_set":9,"id":"1ae5cdf2_7ce487df","line":840,"updated":"2014-06-21 11:11:06.000000000","message":"20s is a long time to wait. 1s should suffice ;)","commit_id":"d4bf9cf38267a21abd8ecf654d89e9bda794b316"},{"author":{"_account_id":5293,"name":"Nikhil","email":"SlickNik@gmail.com","username":"slicknik"},"change_message_id":"1a855bbc1371575ad3268417842b8e438b32ddd0","unresolved":false,"context_lines":[{"line_number":862,"context_line":"            raise SkipTest(\"Test is disabled in fake mode\")"},{"line_number":863,"context_line":"        group \u003d dbaas.security_groups.get(instance_info.id)"},{"line_number":864,"context_line":"        global first_time_updated"},{"line_number":865,"context_line":"        first_time_updated \u003d group.updated"},{"line_number":866,"context_line":"        assert_not_equal(self.testSecurityGroup.created,"},{"line_number":867,"context_line":"                         group.updated)"},{"line_number":868,"context_line":"        assert_not_equal(self.testSecurityGroup.updated,"}],"source_content_type":"text/x-python","patch_set":9,"id":"1ae5cdf2_27a735a9","line":865,"updated":"2014-06-23 10:18:36.000000000","message":"Why do you need a global and a separate test to simply verify timestamps?\n\nThe update is happening as part of the previous test, so the asserts should also be done there.\n\n(Also, please see my last comment about globals and parallelizing test runs).","commit_id":"d4bf9cf38267a21abd8ecf654d89e9bda794b316"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"fc3ac512a334a51697ed8e288f3cdb4336abf2c6","unresolved":false,"context_lines":[{"line_number":864,"context_line":"        global first_time_updated"},{"line_number":865,"context_line":"        first_time_updated \u003d group.updated"},{"line_number":866,"context_line":"        assert_not_equal(self.testSecurityGroup.created,"},{"line_number":867,"context_line":"                         group.updated)"},{"line_number":868,"context_line":"        assert_not_equal(self.testSecurityGroup.updated,"},{"line_number":869,"context_line":"                         group.updated)"},{"line_number":870,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"1ae5cdf2_5ce18bce","line":867,"updated":"2014-06-21 11:11:06.000000000","message":"not a big deal but if you are checking here for the two times, it would be nice to check that updated is greater than created. For a bonus, you could make sure that the difference is at least 20s (or 1s if you take my suggestion) because that\u0027s how long you are sleeping for.","commit_id":"d4bf9cf38267a21abd8ecf654d89e9bda794b316"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"407ce8b8fad5b88e8ffe1b1b793dfa771a99cc8e","unresolved":false,"context_lines":[{"line_number":837,"context_line":"    @test"},{"line_number":838,"context_line":"    def test_create_security_group_rule(self):"},{"line_number":839,"context_line":"        # Need to sleep to verify created/updated timestamps"},{"line_number":840,"context_line":"        time.sleep(10)"},{"line_number":841,"context_line":"        cidr \u003d \"1.2.3.4/16\""},{"line_number":842,"context_line":"        self.testSecurityGroupRules \u003d ("},{"line_number":843,"context_line":"            dbaas.security_group_rules.create("}],"source_content_type":"text/x-python","patch_set":13,"id":"baada198_21523f3b","line":840,"updated":"2014-07-07 18:08:39.000000000","message":"why are we back to 10s here? I thought (in a previous review around patch set 9) you agreed to drop this to 1s","commit_id":"936270ba19c52d2991a491204582413bb0ec5177"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"407ce8b8fad5b88e8ffe1b1b793dfa771a99cc8e","unresolved":false,"context_lines":[{"line_number":867,"context_line":"    @test(depends_on\u003d[test_create_security_group_rule])"},{"line_number":868,"context_line":"    def test_delete_security_group_rule(self):"},{"line_number":869,"context_line":"        # Need to sleep to verify created/updated timestamps"},{"line_number":870,"context_line":"        time.sleep(10)"},{"line_number":871,"context_line":"        group_before \u003d dbaas.security_groups.get("},{"line_number":872,"context_line":"            self.testSecurityGroup.id)"},{"line_number":873,"context_line":"        dbaas.security_group_rules.delete(self.test_rule_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"baada198_4143b36b","line":870,"updated":"2014-07-07 18:08:39.000000000","message":"why are we back to 10s here? I thought (in a previous review around patch set 9) you agreed to drop this to 1s","commit_id":"936270ba19c52d2991a491204582413bb0ec5177"},{"author":{"_account_id":5390,"name":"Steve Leon","username":"steve-leon","inactive":true},"change_message_id":"2bcd19c22d172022237a684689fc3817bdc1bc98","unresolved":false,"context_lines":[{"line_number":876,"context_line":"        if not CONFIG.fake_mode:"},{"line_number":877,"context_line":"            group \u003d dbaas.security_groups.get("},{"line_number":878,"context_line":"                self.testSecurityGroup.id)"},{"line_number":879,"context_line":"            assert_not_equal(group_before.created,"},{"line_number":880,"context_line":"                             group.updated)"},{"line_number":881,"context_line":""},{"line_number":882,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"baada198_0a76b2d8","line":879,"updated":"2014-07-08 22:53:36.000000000","message":"I rather you check for the \u0027updated\u0027 value in the group_before instead of the \u0027created\u0027. This will verify that the \u0027updated\u0027 value is still being updated after a security group action.\n\nOr maybe you can add a new check that compares both \u0027updated\u0027 values.","commit_id":"b5b13a86a48e4ff8b6785b576f2ed774133efa14"}]}