)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c295d7ffb5808d208ac49beda101f0620fa76747","unresolved":false,"context_lines":[{"line_number":12,"context_line":"2. rules: the policy rules of the group."},{"line_number":13,"context_line":"3. policy: the policy name."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This object will be used in InsanceGroup object."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Related to blueprint complex-anti-affinity-policies"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5f7c97a3_30fef537","line":15,"range":{"start_line":15,"start_character":28,"end_line":15,"end_character":40},"updated":"2018-06-26 19:19:59.000000000","message":"InstanceGroup","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"d64c0ed02eb33f656383155548ee715be17b2f94","unresolved":false,"context_lines":[{"line_number":12,"context_line":"2. rules: the policy rules of the group."},{"line_number":13,"context_line":"3. policy: the policy name."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This object will be used in InsanceGroup object."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Related to blueprint complex-anti-affinity-policies"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5f7c97a3_de93025b","line":15,"range":{"start_line":15,"start_character":28,"end_line":15,"end_character":40},"in_reply_to":"5f7c97a3_30fef537","updated":"2018-06-27 10:53:06.000000000","message":"Done","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"00fa2f75a24bc5eb20fd788d93af7256dd7bcaed","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. rules: the policy rules of the group."},{"line_number":14,"context_line":"4. policy: the policy name."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"This object will be used in InstanceGroup object."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Related to blueprint complex-anti-affinity-policies"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"5f7c97a3_1a9d26ba","line":16,"updated":"2018-07-09 14:03:00.000000000","message":"nit: I\u0027d also mention that InstanceGroupPolicy isn\u0027t created in the database directly, but rather when its parent InstanceGroup is created.","commit_id":"6bd4410d81ad7d41d27ef5eea3437994c156441a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"365f8fef544cd4ea51d114c8ffc874c44cbfde41","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. rules: the policy rules of the group."},{"line_number":14,"context_line":"4. policy: the policy name."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"This object will be used in InstanceGroup object."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Related to blueprint complex-anti-affinity-policies"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"5f7c97a3_1a91a646","line":16,"in_reply_to":"5f7c97a3_1a9d26ba","updated":"2018-07-09 14:16:24.000000000","message":"Done","commit_id":"6bd4410d81ad7d41d27ef5eea3437994c156441a"}],"nova/objects/__init__.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c295d7ffb5808d208ac49beda101f0620fa76747","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    __import__(\u0027nova.objects.instance_action\u0027)"},{"line_number":47,"context_line":"    __import__(\u0027nova.objects.instance_fault\u0027)"},{"line_number":48,"context_line":"    __import__(\u0027nova.objects.instance_group\u0027)"},{"line_number":49,"context_line":"    __import__(\u0027nova.objects.instance_group_policy\u0027)"},{"line_number":50,"context_line":"    __import__(\u0027nova.objects.instance_info_cache\u0027)"},{"line_number":51,"context_line":"    __import__(\u0027nova.objects.instance_mapping\u0027)"},{"line_number":52,"context_line":"    __import__(\u0027nova.objects.instance_numa_topology\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f7c97a3_509a9112","line":49,"updated":"2018-06-26 19:19:59.000000000","message":"Seems this could just live in the same instance_group module with the InstanceGroup and InstanceGroupList.","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"d64c0ed02eb33f656383155548ee715be17b2f94","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    __import__(\u0027nova.objects.instance_action\u0027)"},{"line_number":47,"context_line":"    __import__(\u0027nova.objects.instance_fault\u0027)"},{"line_number":48,"context_line":"    __import__(\u0027nova.objects.instance_group\u0027)"},{"line_number":49,"context_line":"    __import__(\u0027nova.objects.instance_group_policy\u0027)"},{"line_number":50,"context_line":"    __import__(\u0027nova.objects.instance_info_cache\u0027)"},{"line_number":51,"context_line":"    __import__(\u0027nova.objects.instance_mapping\u0027)"},{"line_number":52,"context_line":"    __import__(\u0027nova.objects.instance_numa_topology\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f7c97a3_9ef82a4a","line":49,"in_reply_to":"5f7c97a3_509a9112","updated":"2018-06-27 10:53:06.000000000","message":"My original consideration is maybe someday we will add some CRUD on policy(like the case that append a new rule to policy) in the future.\n\nSo, I regard instance_group_policy as a separated object.\n\nBut it\u0027s okay to me to put it into instance_group module, especially, if we have some strict limit on this. : )","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2eacd1730f5d5eaca45cd1710a8474d6427db409","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    __import__(\u0027nova.objects.instance_action\u0027)"},{"line_number":47,"context_line":"    __import__(\u0027nova.objects.instance_fault\u0027)"},{"line_number":48,"context_line":"    __import__(\u0027nova.objects.instance_group\u0027)"},{"line_number":49,"context_line":"    __import__(\u0027nova.objects.instance_group_policy\u0027)"},{"line_number":50,"context_line":"    __import__(\u0027nova.objects.instance_info_cache\u0027)"},{"line_number":51,"context_line":"    __import__(\u0027nova.objects.instance_mapping\u0027)"},{"line_number":52,"context_line":"    __import__(\u0027nova.objects.instance_numa_topology\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f7c97a3_b7e989ab","line":49,"in_reply_to":"5f7c97a3_9ef82a4a","updated":"2018-06-27 12:55:18.000000000","message":"\u003e But it\u0027s okay to me to put it into instance_group module, especially, if we have some strict limit on this. : )\n\nWe don\u0027t really have strict rules about it, it\u0027s just that the instance group policy is a child of instance group so it seems more natural to include the objects in the same module. We do similar things for numa-related objects.","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    __import__(\u0027nova.objects.instance_action\u0027)"},{"line_number":47,"context_line":"    __import__(\u0027nova.objects.instance_fault\u0027)"},{"line_number":48,"context_line":"    __import__(\u0027nova.objects.instance_group\u0027)"},{"line_number":49,"context_line":"    __import__(\u0027nova.objects.instance_group_policy\u0027)"},{"line_number":50,"context_line":"    __import__(\u0027nova.objects.instance_info_cache\u0027)"},{"line_number":51,"context_line":"    __import__(\u0027nova.objects.instance_mapping\u0027)"},{"line_number":52,"context_line":"    __import__(\u0027nova.objects.instance_numa_topology\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_72653f1c","line":49,"updated":"2018-06-27 13:58:26.000000000","message":"As noted in PS5, let\u0027s put the new InstanceGroupPolicy object in the instance_group module. It\u0027s a child object of InstanceGroup and logically fits into the same module.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    __import__(\u0027nova.objects.instance_action\u0027)"},{"line_number":47,"context_line":"    __import__(\u0027nova.objects.instance_fault\u0027)"},{"line_number":48,"context_line":"    __import__(\u0027nova.objects.instance_group\u0027)"},{"line_number":49,"context_line":"    __import__(\u0027nova.objects.instance_group_policy\u0027)"},{"line_number":50,"context_line":"    __import__(\u0027nova.objects.instance_info_cache\u0027)"},{"line_number":51,"context_line":"    __import__(\u0027nova.objects.instance_mapping\u0027)"},{"line_number":52,"context_line":"    __import__(\u0027nova.objects.instance_numa_topology\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_c34315b0","line":49,"in_reply_to":"5f7c97a3_72653f1c","updated":"2018-06-28 03:46:12.000000000","message":"Done","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"}],"nova/objects/instance_group.py":[{"author":{"_account_id":27621,"name":"Vishakha Agarwal","email":"agarwalvishakha18@gmail.com","username":"Vishakha"},"change_message_id":"f0371f25bee42f74ee2e65d6c9685e742152bc3e","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db import exception as db_exc"},{"line_number":18,"context_line":"from oslo_utils import uuidutils"},{"line_number":19,"context_line":"from oslo_utils import versionutils"},{"line_number":20,"context_line":"from oslo_serialization import jsonutils"},{"line_number":21,"context_line":"from sqlalchemy.orm import contains_eager"},{"line_number":22,"context_line":"from sqlalchemy.orm import joinedload"},{"line_number":23,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_577fbe16","line":20,"updated":"2018-06-28 06:30:11.000000000","message":"This import should come after from oslo_db statement","commit_id":"6c9c4cbf279d080e1239d01089d635cbef19790c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e4df3319db95ae9d9dedce7bbcbc84c11ff47f02","unresolved":false,"context_lines":[{"line_number":516,"context_line":"    @staticmethod"},{"line_number":517,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":518,"context_line":"        for key in policy.fields:"},{"line_number":519,"context_line":"            if key \u003d\u003d \u0027rules_json\u0027:"},{"line_number":520,"context_line":"                policy.rules_json \u003d db_policy.get(\"rules\")"},{"line_number":521,"context_line":"            else:"},{"line_number":522,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":523,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_6e976f47","line":520,"range":{"start_line":519,"start_character":12,"end_line":520,"end_character":58},"updated":"2018-06-28 19:19:42.000000000","message":"This doesn\u0027t need to be handled any different.","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a18a426f21999ae74ad017114c8019b407aaeaca","unresolved":false,"context_lines":[{"line_number":516,"context_line":"    @staticmethod"},{"line_number":517,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":518,"context_line":"        for key in policy.fields:"},{"line_number":519,"context_line":"            if key \u003d\u003d \u0027rules_json\u0027:"},{"line_number":520,"context_line":"                policy.rules_json \u003d db_policy.get(\"rules\")"},{"line_number":521,"context_line":"            else:"},{"line_number":522,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":523,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_ac7eeae0","line":520,"range":{"start_line":519,"start_character":12,"end_line":520,"end_character":58},"in_reply_to":"5f7c97a3_11d4db1e","updated":"2018-06-29 02:27:24.000000000","message":"Oh right, sorry.","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"69f3bf8220339e3d585f841119eace81404850e4","unresolved":false,"context_lines":[{"line_number":516,"context_line":"    @staticmethod"},{"line_number":517,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":518,"context_line":"        for key in policy.fields:"},{"line_number":519,"context_line":"            if key \u003d\u003d \u0027rules_json\u0027:"},{"line_number":520,"context_line":"                policy.rules_json \u003d db_policy.get(\"rules\")"},{"line_number":521,"context_line":"            else:"},{"line_number":522,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":523,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_11d4db1e","line":520,"range":{"start_line":519,"start_character":12,"end_line":520,"end_character":58},"in_reply_to":"5f7c97a3_5166d3bd","updated":"2018-06-29 01:31:13.000000000","message":"otherwise we will get back a KeyError on db_policy[\u0027rules_json\u0027]","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"79b57170475cfa3c5378068f3425ca2ac9a50cc1","unresolved":false,"context_lines":[{"line_number":516,"context_line":"    @staticmethod"},{"line_number":517,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":518,"context_line":"        for key in policy.fields:"},{"line_number":519,"context_line":"            if key \u003d\u003d \u0027rules_json\u0027:"},{"line_number":520,"context_line":"                policy.rules_json \u003d db_policy.get(\"rules\")"},{"line_number":521,"context_line":"            else:"},{"line_number":522,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":523,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_5166d3bd","line":520,"range":{"start_line":519,"start_character":12,"end_line":520,"end_character":58},"in_reply_to":"5f7c97a3_6e976f47","updated":"2018-06-29 01:28:42.000000000","message":"No, we need store db.rules into obj.runles_json in here.","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e4df3319db95ae9d9dedce7bbcbc84c11ff47f02","unresolved":false,"context_lines":[{"line_number":528,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":529,"context_line":"    def _create_in_db(context, updates):"},{"line_number":530,"context_line":"        group_policy \u003d api_models.InstanceGroupPolicy()"},{"line_number":531,"context_line":"        if \u0027rules_json\u0027 in updates:"},{"line_number":532,"context_line":"            updates[\u0027rules_json\u0027] \u003d updates[\u0027rules_json\u0027]"},{"line_number":533,"context_line":"        group_policy.update(updates)"},{"line_number":534,"context_line":"        group_policy.save(context.session)"},{"line_number":535,"context_line":"        return group_policy"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_4e73934f","line":532,"range":{"start_line":531,"start_character":8,"end_line":532,"end_character":57},"updated":"2018-06-28 19:19:42.000000000","message":"This doesn\u0027t do anything and shouldn\u0027t be needed anyway.","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"79b57170475cfa3c5378068f3425ca2ac9a50cc1","unresolved":false,"context_lines":[{"line_number":528,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":529,"context_line":"    def _create_in_db(context, updates):"},{"line_number":530,"context_line":"        group_policy \u003d api_models.InstanceGroupPolicy()"},{"line_number":531,"context_line":"        if \u0027rules_json\u0027 in updates:"},{"line_number":532,"context_line":"            updates[\u0027rules_json\u0027] \u003d updates[\u0027rules_json\u0027]"},{"line_number":533,"context_line":"        group_policy.update(updates)"},{"line_number":534,"context_line":"        group_policy.save(context.session)"},{"line_number":535,"context_line":"        return group_policy"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_11061bcf","line":532,"range":{"start_line":531,"start_character":8,"end_line":532,"end_character":57},"in_reply_to":"5f7c97a3_4e73934f","updated":"2018-06-29 01:28:42.000000000","message":"Done","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e4df3319db95ae9d9dedce7bbcbc84c11ff47f02","unresolved":false,"context_lines":[{"line_number":547,"context_line":"        return jsonutils.loads(self.rules_json)"},{"line_number":548,"context_line":""},{"line_number":549,"context_line":"    @rules.setter"},{"line_number":550,"context_line":"    def rules(self, r):"},{"line_number":551,"context_line":"        self.rules_json \u003d jsonutils.dumps(r)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_8e9e8b53","line":550,"range":{"start_line":550,"start_character":20,"end_line":550,"end_character":21},"updated":"2018-06-28 19:19:42.000000000","message":"nit: name this \u0027rules\u0027","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"79b57170475cfa3c5378068f3425ca2ac9a50cc1","unresolved":false,"context_lines":[{"line_number":547,"context_line":"        return jsonutils.loads(self.rules_json)"},{"line_number":548,"context_line":""},{"line_number":549,"context_line":"    @rules.setter"},{"line_number":550,"context_line":"    def rules(self, r):"},{"line_number":551,"context_line":"        self.rules_json \u003d jsonutils.dumps(r)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_b10ac7ce","line":550,"range":{"start_line":550,"start_character":20,"end_line":550,"end_character":21},"in_reply_to":"5f7c97a3_8e9e8b53","updated":"2018-06-29 01:28:42.000000000","message":"Done","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e270f124793537626b06b50fbc4eeef3d58099f8","unresolved":false,"context_lines":[{"line_number":545,"context_line":"        return jsonutils.loads(self.rules_json)"},{"line_number":546,"context_line":""},{"line_number":547,"context_line":"    @rules.setter"},{"line_number":548,"context_line":"    def rules(self, r):"},{"line_number":549,"context_line":"        self.rules_json \u003d jsonutils.dumps(r)"}],"source_content_type":"text/x-python","patch_set":10,"id":"5f7c97a3_ec5dc27c","line":548,"range":{"start_line":548,"start_character":20,"end_line":548,"end_character":21},"updated":"2018-06-29 02:28:18.000000000","message":"rules","commit_id":"39e6c9918a33f6a369f0564389527944c4664713"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"0fcc71d94c40fadba837728b5c06c9015bd2db64","unresolved":false,"context_lines":[{"line_number":545,"context_line":"        return jsonutils.loads(self.rules_json)"},{"line_number":546,"context_line":""},{"line_number":547,"context_line":"    @rules.setter"},{"line_number":548,"context_line":"    def rules(self, r):"},{"line_number":549,"context_line":"        self.rules_json \u003d jsonutils.dumps(r)"}],"source_content_type":"text/x-python","patch_set":10,"id":"5f7c97a3_475cd118","line":548,"range":{"start_line":548,"start_character":20,"end_line":548,"end_character":21},"in_reply_to":"5f7c97a3_ec5dc27c","updated":"2018-06-29 03:02:26.000000000","message":"oops, I remember I have fixed this, I will fix it in next PS.","commit_id":"39e6c9918a33f6a369f0564389527944c4664713"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"74e720ecb44f5f4f645762b146ec0d69a08eefda","unresolved":false,"context_lines":[{"line_number":542,"context_line":""},{"line_number":543,"context_line":"    @property"},{"line_number":544,"context_line":"    def rules(self):"},{"line_number":545,"context_line":"        if hasattr(self, \u0027rules_json\u0027) and self.rules_json:"},{"line_number":546,"context_line":"            return jsonutils.loads(self.rules_json)"},{"line_number":547,"context_line":"        return None"},{"line_number":548,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_7d6110d4","line":545,"range":{"start_line":545,"start_character":11,"end_line":545,"end_character":38},"updated":"2018-07-03 19:04:56.000000000","message":"For versioned objects, this would usually be written as:\n\n  if \u0027rules_json\u0027 in self and self.rules_json:","commit_id":"7415570a24c83625557db7127442559ed53dc333"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebc53508f618adf724172da2e6afb3aaff0d7a9d","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027rules_json\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_0b7d081d","line":513,"updated":"2018-07-05 18:15:47.000000000","message":"I know that it sucks for me to swoop in late like this and complain about this, but....I think storing this on the object as json, with no restriction on the contents, is a bad idea.\n\nThe API enforces (or would enforce, I assume) that this is a flat key\u003dvalue dict, which is good, but we should make the same hard guarantees for our internal APIs I think.\n\nWhat about something like this?\n\nhttps://pastebin.com/ea8f5xeZ\n\nDiff from what you have here is:\n\nhttps://pastebin.com/SPM9drrS\n\nMaybe it also needs to allow properties\u003d in the init so that you can actually provide it with the same name or something is warranted, but since we don\u0027t load/edit/save this a lot I think it should be okay.\n\nAnyway, I\u0027d just really like to avoid storing actual json in the object and opening the opportunity for people to use this interface to store arbitrarily complex structures in this field.","commit_id":"feddef465dcab333dcce3b025215f3b49b4c1642"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9a989c509b01f8e0579a7031d08d795ec6a122a5","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027rules_json\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_2b6b24f0","line":513,"in_reply_to":"5f7c97a3_0b7d081d","updated":"2018-07-05 18:40:52.000000000","message":"He was originally using DictOfStringsField but I led him astray here:\n\nhttps://review.openstack.org/#/c/573628/5/nova/objects/instance_group_policy.py@29\n\nand:\n\nhttps://review.openstack.org/#/c/573628/6/nova/tests/unit/objects/test_instance_group_policy.py@42\n\nI\u0027m OK with using DictOfStringsField, just have to be careful about where the serialize/deserialize happens.","commit_id":"feddef465dcab333dcce3b025215f3b49b4c1642"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6fe170ffeaa342cdcea7c8c6595c7b322d20e455","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027rules_json\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_665f1375","line":513,"in_reply_to":"5f7c97a3_2b6b24f0","updated":"2018-07-05 19:31:17.000000000","message":"If you use the properties property, then it gets type-casted every time you access it so you don\u0027t have to worry about it in each place. Maybe even worth calling the field _rules to make it obvious that it shouldn\u0027t be accessed directly.","commit_id":"feddef465dcab333dcce3b025215f3b49b4c1642"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"15adaa8c7643de3ca640685953ff52cd24e4a075","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027rules_json\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_1d4b0928","line":513,"in_reply_to":"5f7c97a3_665f1375","updated":"2018-07-06 08:11:48.000000000","message":"@danms Thanks for your detail demo and diff, and I will address your idea in next PS. and properties property is a good idea, good to learn it.\n\nand how about rename \"properties\" to \"rules\"? and InstanceGroupPolicy._rules is an internal field, and the rules is for user.","commit_id":"feddef465dcab333dcce3b025215f3b49b4c1642"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c1314e6970849a283f2128ef73b4cb712472a994","unresolved":false,"context_lines":[{"line_number":542,"context_line":"        return group_policy"},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"    @base.remotable"},{"line_number":545,"context_line":"    def create(self):"},{"line_number":546,"context_line":"        if self.obj_attr_is_set(\u0027id\u0027):"},{"line_number":547,"context_line":"            raise exception.ObjectActionError(action\u003d\u0027create\u0027,"},{"line_number":548,"context_line":"                                              reason\u003d\u0027already created\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_f4597f28","line":545,"updated":"2018-07-06 13:50:45.000000000","message":"See https://review.openstack.org/#/c/563375/29/nova/objects/instance_group.py@270 - we need to either use this method or remove it. I think we should use it, otherwise there isn\u0027t a whole lot of point in this versioned object.","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"c33104a323bc6d9fc00bbb873bfd7af8842d6ff2","unresolved":false,"context_lines":[{"line_number":542,"context_line":"        return group_policy"},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"    @base.remotable"},{"line_number":545,"context_line":"    def create(self):"},{"line_number":546,"context_line":"        if self.obj_attr_is_set(\u0027id\u0027):"},{"line_number":547,"context_line":"            raise exception.ObjectActionError(action\u003d\u0027create\u0027,"},{"line_number":548,"context_line":"                                              reason\u003d\u0027already created\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_01014404","line":545,"in_reply_to":"5f7c97a3_f4597f28","updated":"2018-07-09 07:36:10.000000000","message":"See reply in https://review.openstack.org/#/c/563375/29/nova/objects/instance_group.py@270","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"57ca303192e53b090bdab9ee8a6473b39396c99f","unresolved":false,"context_lines":[{"line_number":552,"context_line":"    @property"},{"line_number":553,"context_line":"    def rules(self):"},{"line_number":554,"context_line":"        # NOTE(danms): Coerce our rules into a typed dict for convenience"},{"line_number":555,"context_line":"        rules \u003d {}"},{"line_number":556,"context_line":"        if \u0027_rules\u0027 in self and self._rules:"},{"line_number":557,"context_line":"            if \u0027max_server_per_host\u0027 in self._rules:"},{"line_number":558,"context_line":"                rules[\u0027max_server_per_host\u0027] \u003d \\"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_9903a476","line":555,"updated":"2018-07-06 13:28:59.000000000","message":"Since rules is nullable, it seems we should return None instead of {} if _rules is None.\n\nSo:\n\nif \u0027_rules\u0027 in self and self._rules is None:\n    return None","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"c33104a323bc6d9fc00bbb873bfd7af8842d6ff2","unresolved":false,"context_lines":[{"line_number":552,"context_line":"    @property"},{"line_number":553,"context_line":"    def rules(self):"},{"line_number":554,"context_line":"        # NOTE(danms): Coerce our rules into a typed dict for convenience"},{"line_number":555,"context_line":"        rules \u003d {}"},{"line_number":556,"context_line":"        if \u0027_rules\u0027 in self and self._rules:"},{"line_number":557,"context_line":"            if \u0027max_server_per_host\u0027 in self._rules:"},{"line_number":558,"context_line":"                rules[\u0027max_server_per_host\u0027] \u003d \\"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_feeaa70b","line":555,"in_reply_to":"5f7c97a3_9903a476","updated":"2018-07-09 07:36:10.000000000","message":"Done","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"65b6e932d269c9d82694b1ed41a4a306dba5a0f7","unresolved":false,"context_lines":[{"line_number":552,"context_line":"    @property"},{"line_number":553,"context_line":"    def rules(self):"},{"line_number":554,"context_line":"        # NOTE(danms): Coerce our rules into a typed dict for convenience"},{"line_number":555,"context_line":"        rules \u003d {}"},{"line_number":556,"context_line":"        if \u0027_rules\u0027 in self and self._rules:"},{"line_number":557,"context_line":"            if \u0027max_server_per_host\u0027 in self._rules:"},{"line_number":558,"context_line":"                rules[\u0027max_server_per_host\u0027] \u003d \\"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_3445b715","line":555,"in_reply_to":"5f7c97a3_9903a476","updated":"2018-07-06 13:55:34.000000000","message":"This is also causing the notification tests to fail later in the series:\n\nhttp://logs.openstack.org/01/563401/20/check/nova-tox-functional/bffd91a/testr_results.html.gz","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b886ded0a0dd77150cad9385ede65bd6359308d3","unresolved":false,"context_lines":[{"line_number":541,"context_line":"        group_policy.save(context.session)"},{"line_number":542,"context_line":"        return group_policy"},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"    @base.remotable"},{"line_number":545,"context_line":"    def create(self):"},{"line_number":546,"context_line":"        if self.obj_attr_is_set(\u0027id\u0027):"},{"line_number":547,"context_line":"            raise exception.ObjectActionError(action\u003d\u0027create\u0027,"},{"line_number":548,"context_line":"                                              reason\u003d\u0027already created\u0027)"},{"line_number":549,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":550,"context_line":"        return self._from_db_object(self._context, self, db_policy)"},{"line_number":551,"context_line":""},{"line_number":552,"context_line":"    @property"},{"line_number":553,"context_line":"    def rules(self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f7c97a3_d060f437","line":550,"range":{"start_line":544,"start_character":4,"end_line":550,"end_character":67},"updated":"2018-07-09 12:30:35.000000000","message":"Might as well remove this if nothing is using it, and from https://review.openstack.org/#/c/563375/29/nova/objects/instance_group.py@270 it sounds like we still need to populate InstanceGroup._policies so we can load it from the DB.","commit_id":"fef979c55a14b794657d7cf8a7c8408f4b395fa2"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"21965af9dfd5f7b9600abd05c4988add00978fc7","unresolved":false,"context_lines":[{"line_number":541,"context_line":"        group_policy.save(context.session)"},{"line_number":542,"context_line":"        return group_policy"},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"    @base.remotable"},{"line_number":545,"context_line":"    def create(self):"},{"line_number":546,"context_line":"        if self.obj_attr_is_set(\u0027id\u0027):"},{"line_number":547,"context_line":"            raise exception.ObjectActionError(action\u003d\u0027create\u0027,"},{"line_number":548,"context_line":"                                              reason\u003d\u0027already created\u0027)"},{"line_number":549,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":550,"context_line":"        return self._from_db_object(self._context, self, db_policy)"},{"line_number":551,"context_line":""},{"line_number":552,"context_line":"    @property"},{"line_number":553,"context_line":"    def rules(self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f7c97a3_04bb8de4","line":550,"range":{"start_line":544,"start_character":4,"end_line":550,"end_character":67},"in_reply_to":"5f7c97a3_d060f437","updated":"2018-07-09 13:57:26.000000000","message":"Yeah, I will remove it.","commit_id":"fef979c55a14b794657d7cf8a7c8408f4b395fa2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b886ded0a0dd77150cad9385ede65bd6359308d3","unresolved":false,"context_lines":[{"line_number":555,"context_line":"            return None"},{"line_number":556,"context_line":"        # NOTE(danms): Coerce our rules into a typed dict for convenience"},{"line_number":557,"context_line":"        rules \u003d {}"},{"line_number":558,"context_line":"        if \u0027_rules\u0027 in self and self._rules:"},{"line_number":559,"context_line":"            if \u0027max_server_per_host\u0027 in self._rules:"},{"line_number":560,"context_line":"                rules[\u0027max_server_per_host\u0027] \u003d \\"},{"line_number":561,"context_line":"                        int(self._rules[\u0027max_server_per_host\u0027])"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f7c97a3_907d9ce7","line":558,"range":{"start_line":558,"start_character":8,"end_line":558,"end_character":44},"updated":"2018-07-09 12:30:35.000000000","message":"nit: this can probably go away now","commit_id":"fef979c55a14b794657d7cf8a7c8408f4b395fa2"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"21965af9dfd5f7b9600abd05c4988add00978fc7","unresolved":false,"context_lines":[{"line_number":555,"context_line":"            return None"},{"line_number":556,"context_line":"        # NOTE(danms): Coerce our rules into a typed dict for convenience"},{"line_number":557,"context_line":"        rules \u003d {}"},{"line_number":558,"context_line":"        if \u0027_rules\u0027 in self and self._rules:"},{"line_number":559,"context_line":"            if \u0027max_server_per_host\u0027 in self._rules:"},{"line_number":560,"context_line":"                rules[\u0027max_server_per_host\u0027] \u003d \\"},{"line_number":561,"context_line":"                        int(self._rules[\u0027max_server_per_host\u0027])"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f7c97a3_a7597b5b","line":558,"range":{"start_line":558,"start_character":8,"end_line":558,"end_character":44},"in_reply_to":"5f7c97a3_907d9ce7","updated":"2018-07-09 13:57:26.000000000","message":"Done","commit_id":"fef979c55a14b794657d7cf8a7c8408f4b395fa2"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0bae8686fec566d4f256cd4c5ef17ef0864c0011","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027_rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_783a22cd","line":513,"updated":"2018-07-09 15:37:28.000000000","message":"For follow-up: put a comment above this telling people not to use _rules, but use rules instead.\n\nAlso, are we sure we want this to be nullable\u003dTrue and not just always have this be an empty dict if there were no rules? If we create with rules\u003d{}, we\u0027ll store the empty dict, which means people will have to test for falsey-ness and not specifically \"is None\".","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dcf4a61674137d5c1696eca08c9363b1315fc21","unresolved":false,"context_lines":[{"line_number":510,"context_line":"        \u0027id\u0027: fields.IntegerField(),"},{"line_number":511,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":512,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":513,"context_line":"        \u0027_rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"},{"line_number":514,"context_line":"    }"},{"line_number":515,"context_line":""},{"line_number":516,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_933a81c1","line":513,"in_reply_to":"5f7c97a3_783a22cd","updated":"2018-07-09 16:02:12.000000000","message":"\u003e Also, are we sure we want this to be nullable\u003dTrue and not just always have this be an empty dict if there were no rules? If we create with rules\u003d{}, we\u0027ll store the empty dict, which means people will have to test for falsey-ness and not specifically \"is None\".\n\nDoesn\u0027t matter much to me either way - the REST API will return rules\u003d{} if it\u0027s None to match how we handle a flavor with no extra specs:\n\nhttps://review.openstack.org/#/c/567534/22/nova/api/openstack/compute/server_groups.py@99","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0bae8686fec566d4f256cd4c5ef17ef0864c0011","unresolved":false,"context_lines":[{"line_number":523,"context_line":"        for key in policy.fields:"},{"line_number":524,"context_line":"            if key \u003d\u003d \u0027_rules\u0027:"},{"line_number":525,"context_line":"                db_rules \u003d db_policy.get(\"rules\")"},{"line_number":526,"context_line":"                policy._rules \u003d jsonutils.loads(db_rules) if db_rules else None"},{"line_number":527,"context_line":"            else:"},{"line_number":528,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":529,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_d8da5648","line":526,"updated":"2018-07-09 15:37:28.000000000","message":"So, it feels weird to me to put the loading code in here, but the saving code ends up in InstanceGroup. I wonder if it\u0027s a sign that we don\u0027t need a separate object for this.\n\nI don\u0027t think we need to store the id of the policy row here, since we don\u0027t need it to load/save this object independently. I also don\u0027t really think we need to store the group_id in this object since we don\u0027t need to look up the group, given only the policy object.\n\nWith that, it means our policy field on the group would just be an object field, which holds just a policy name and a dict of rules.\n\nWe\u0027re already adding a policy field to group for this (and have policies already). I wonder if it would just be better to add the rules dict to the group object. Since it\u0027s 1:1 group-to-policy, there\u0027s no need to have more than one policy object for a given group, and I think the split of load/save code would be a lot simpler and cleaner if we just do it all there.\n\nThoughts?","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7fd6f02f0b0e267fee49793432335fb0ac8e3929","unresolved":false,"context_lines":[{"line_number":523,"context_line":"        for key in policy.fields:"},{"line_number":524,"context_line":"            if key \u003d\u003d \u0027_rules\u0027:"},{"line_number":525,"context_line":"                db_rules \u003d db_policy.get(\"rules\")"},{"line_number":526,"context_line":"                policy._rules \u003d jsonutils.loads(db_rules) if db_rules else None"},{"line_number":527,"context_line":"            else:"},{"line_number":528,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":529,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_735ec545","line":526,"in_reply_to":"5f7c97a3_139d11ba","updated":"2018-07-09 16:37:26.000000000","message":"Works for me. Yikun is asleep by now and was going to work on addressing my comments in the API change tomorrow. I can help with refactoring/rebasing the rest of the series if you can make the model changes you want today.","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ce05e3d6ad8a48859c7794546a33c9a3914cdf5c","unresolved":false,"context_lines":[{"line_number":523,"context_line":"        for key in policy.fields:"},{"line_number":524,"context_line":"            if key \u003d\u003d \u0027_rules\u0027:"},{"line_number":525,"context_line":"                db_rules \u003d db_policy.get(\"rules\")"},{"line_number":526,"context_line":"                policy._rules \u003d jsonutils.loads(db_rules) if db_rules else None"},{"line_number":527,"context_line":"            else:"},{"line_number":528,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":529,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_139d11ba","line":526,"in_reply_to":"5f7c97a3_9807fe1d","updated":"2018-07-09 16:04:46.000000000","message":"Converting to the singleton policy field is fine with me, leaving the convert-to-readonly-ness of the policies field. The important thing for me is the removal of this extra object and moving the rules to the main one.","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dcf4a61674137d5c1696eca08c9363b1315fc21","unresolved":false,"context_lines":[{"line_number":523,"context_line":"        for key in policy.fields:"},{"line_number":524,"context_line":"            if key \u003d\u003d \u0027_rules\u0027:"},{"line_number":525,"context_line":"                db_rules \u003d db_policy.get(\"rules\")"},{"line_number":526,"context_line":"                policy._rules \u003d jsonutils.loads(db_rules) if db_rules else None"},{"line_number":527,"context_line":"            else:"},{"line_number":528,"context_line":"                setattr(policy, key, db_policy[key])"},{"line_number":529,"context_line":"        policy.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f7c97a3_9807fe1d","line":526,"in_reply_to":"5f7c97a3_d8da5648","updated":"2018-07-09 16:02:12.000000000","message":"\u003e So, it feels weird to me to put the loading code in here, but the saving code ends up in InstanceGroup. I wonder if it\u0027s a sign that we don\u0027t need a separate object for this.\n\nI\u0027ve struggled with the same thing for the create() method which was removed in this latest patch set. See: https://review.openstack.org/#/c/563375/29/nova/objects/instance_group.py@270\n\n\u003e We\u0027re already adding a policy field to group for this (and have policies already). I wonder if it would just be better to add the rules dict to the group object. Since it\u0027s 1:1 group-to-policy, there\u0027s no need to have more than one policy object for a given group, and I think the split of load/save code would be a lot simpler and cleaner if we just do it all there.\n\nSo you\u0027re suggesting drop the InstanceGroupPolicy object, use InstanceGroup.policies (the list of strings which is only ever 1) and add InstanceGroup.rules? I still don\u0027t really like InstanceGroup.policies in the model since it\u0027s only ever 1 entry, but it wouldn\u0027t be the end of the world to do it that way.","commit_id":"e0343eefd83c2fe292bc671a5384cd3cc2d176c6"}],"nova/objects/instance_group_policy.py":[{"author":{"_account_id":7634,"name":"Takashi Natsume","email":"takanattie@gmail.com","username":"natsumet"},"change_message_id":"8ed22307f77af760dbfb70ddf49490f256d12faa","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"@base.NovaObjectRegistry.register"},{"line_number":22,"context_line":"class InstanceGroupPolicy(base.NovaObject):"},{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_0e43ffb1","line":22,"updated":"2018-06-21 02:40:50.000000000","message":"Just a question:\nWhy doesn\u0027t this object have save and destroy method?\nAre they unnecessary?","commit_id":"ada8eb1a8d3c7ae6411bad6271b183624dde9839"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"d3fd567fb4e7d33387c2e167f067d67d9538a371","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"@base.NovaObjectRegistry.register"},{"line_number":22,"context_line":"class InstanceGroupPolicy(base.NovaObject):"},{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_991c3b67","line":22,"in_reply_to":"5f7c97a3_0e43ffb1","updated":"2018-06-21 03:23:57.000000000","message":"Yes, now We don\u0027t support change policy, and we destroy the policy when group destroyed [1].\n\nSo, I think we can add them when we really need them.\n\n[1] https://github.com/openstack/nova/blob/9882a60e69a5ab8da314a199a56defc05098b743/nova/objects/instance_group.py#L272","commit_id":"ada8eb1a8d3c7ae6411bad6271b183624dde9839"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c295d7ffb5808d208ac49beda101f0620fa76747","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    fields \u003d {"},{"line_number":27,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":28,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":29,"context_line":"        \u0027rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"},{"line_number":30,"context_line":"    }"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f7c97a3_b0c0c5e1","line":29,"range":{"start_line":29,"start_character":24,"end_line":29,"end_character":42},"updated":"2018-06-26 19:19:59.000000000","message":"One thing I realized recently is that this is going to coerce all of the values to strings as well, which might get a little weird with the max_server_per_host rule that has an integer value. You must be casting that to a string in the scheduler filter code that\u0027s using it?\n\nFor something I\u0027m working on where the values can be strings, booleans or integers, I\u0027m just serializing the json dictionary from the API and deserializing it when the field is accessed:\n\nhttps://review.openstack.org/#/c/515423/29/nova/objects/migrate_data.py\n\nI think you\u0027re just following how Flavor.extra_specs are modeled but it does mean we\u0027ll have to cast the values to the appropriate types in whatever consuming code is using these.\n\n(later)\n\nYup I see you have to cast to int in the filter here:\n\nhttps://review.openstack.org/#/c/571166/10/nova/scheduler/filters/affinity_filter.py@106\n\nI guess that\u0027s probably OK since we\u0027re defining what these are and how they can be used. I\u0027m sure we have flavor extra specs where the values are integers and we have to cast appropriately as well.","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"d64c0ed02eb33f656383155548ee715be17b2f94","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    fields \u003d {"},{"line_number":27,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":28,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":29,"context_line":"        \u0027rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"},{"line_number":30,"context_line":"    }"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f7c97a3_5e7852f6","line":29,"range":{"start_line":29,"start_character":24,"end_line":29,"end_character":42},"in_reply_to":"5f7c97a3_b0c0c5e1","updated":"2018-06-27 10:53:06.000000000","message":"Yes, as you mentioned, I refered to the Flavor.extra_specs, and also metadata.\n\nand the other case is user how to create a group with rule using cli:\nnova server-create-group group_name anti-affinity --rules max_server_per_host\u003d3 --rules future_key\u003dfuture_value\n\nBut if we don\u0027t use string to store the value, we maybe need something like:\nnova server-create-group group_name anti-affinity --rules max_server_per_host\u003d3 --rules future_key\u003d\"future_value\"\n\nSo, I think we can cast it when we use it, because we know the type when we really need it.","commit_id":"e324eca140a51bffaedc8828b76f3e452d4a86df"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"@base.NovaObjectRegistry.register"},{"line_number":22,"context_line":"class InstanceGroupPolicy(base.NovaObject):"},{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_8d5eba10","line":22,"updated":"2018-06-27 13:58:26.000000000","message":"We don\u0027t need any getter methods to get an InstanceGroupPolicy by policy or group_id? I guess it depends on how this gets used later in the series.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"72d81402397b3306d10d5d510102be0be5f72910","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"@base.NovaObjectRegistry.register"},{"line_number":22,"context_line":"class InstanceGroupPolicy(base.NovaObject):"},{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_de39b940","line":22,"in_reply_to":"5f7c97a3_8d5eba10","updated":"2018-06-27 16:05:08.000000000","message":"After looking at the next patch in the series, I guess this is just constructed when loading (or saving) the InstanceGroup, and when loading the instance group we load the policy object from the backref, so that\u0027s why there are no getter methods on this object.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"@base.NovaObjectRegistry.register"},{"line_number":22,"context_line":"class InstanceGroupPolicy(base.NovaObject):"},{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_633c8092","line":22,"in_reply_to":"5f7c97a3_de39b940","updated":"2018-06-28 03:46:12.000000000","message":"yep","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    fields \u003d {"},{"line_number":27,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":28,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":29,"context_line":"        \u0027rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_ad129653","line":26,"updated":"2018-06-27 13:58:26.000000000","message":"Seems odd that there wouldn\u0027t be an id field even though there is one in the table:\n\nhttps://review.openstack.org/#/c/560832/10/nova/db/sqlalchemy/api_models.py@416\n\nWe also have a pretty common pattern of checking if the id is set during create():\n\nhttps://github.com/openstack/nova/blob/dbfba3951325524da184ddb681f71fd88e5ee81c/nova/objects/instance_group.py#L392","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    # Version 1.0: Initial version"},{"line_number":24,"context_line":"    VERSION \u003d \u00271.0\u0027"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    fields \u003d {"},{"line_number":27,"context_line":"        \u0027policy\u0027: fields.StringField(),"},{"line_number":28,"context_line":"        \u0027group_id\u0027: fields.IntegerField(),"},{"line_number":29,"context_line":"        \u0027rules\u0027: fields.DictOfStringsField(nullable\u003dTrue),"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_03b82c0f","line":26,"in_reply_to":"5f7c97a3_ad129653","updated":"2018-06-28 03:46:12.000000000","message":"Done","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @staticmethod"},{"line_number":33,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":34,"context_line":"        if not db_policy:"},{"line_number":35,"context_line":"            return None"},{"line_number":36,"context_line":"        for key in policy.fields:"},{"line_number":37,"context_line":"            if key \u003d\u003d \u0027rules\u0027:"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_0d1dca60","line":34,"range":{"start_line":34,"start_character":8,"end_line":34,"end_character":25},"updated":"2018-06-27 13:58:26.000000000","message":"When would this ever happen?","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aff832d0863468e23a675eb67692f34e8a1ebb68","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @staticmethod"},{"line_number":33,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":34,"context_line":"        if not db_policy:"},{"line_number":35,"context_line":"            return None"},{"line_number":36,"context_line":"        for key in policy.fields:"},{"line_number":37,"context_line":"            if key \u003d\u003d \u0027rules\u0027:"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_b56e602a","line":34,"range":{"start_line":34,"start_character":8,"end_line":34,"end_character":25},"in_reply_to":"5f7c97a3_0d1dca60","updated":"2018-06-27 14:37:40.000000000","message":"I guess here:\n\nhttps://review.openstack.org/#/c/563375/17/nova/objects/instance_group.py@216\n\nI don\u0027t like this though - _from_db_object should expect to get a db object, I don\u0027t think we have any other objects that handle None coming in like this - the caller should be aware of what it\u0027s doing, so in this case https://review.openstack.org/#/c/563375/17/nova/objects/instance_group.py@216 should handle the None logic.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @staticmethod"},{"line_number":33,"context_line":"    def _from_db_object(context, policy, db_policy):"},{"line_number":34,"context_line":"        if not db_policy:"},{"line_number":35,"context_line":"            return None"},{"line_number":36,"context_line":"        for key in policy.fields:"},{"line_number":37,"context_line":"            if key \u003d\u003d \u0027rules\u0027:"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_63d80052","line":34,"range":{"start_line":34,"start_character":8,"end_line":34,"end_character":25},"in_reply_to":"5f7c97a3_b56e602a","updated":"2018-06-28 03:46:12.000000000","message":"Done","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"37cdb744e10b6107fa384477539e82393d5608dc","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        return group_policy"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    @base.remotable"},{"line_number":57,"context_line":"    def create(self):"},{"line_number":58,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":59,"context_line":"        return self._from_db_object(self._context, self, db_policy)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_be195dd2","line":57,"updated":"2018-06-27 16:06:33.000000000","message":"Hmm, is this even ever used anywhere? Because when we save the policy on the group, it\u0027s done using this method:\n\nhttps://review.openstack.org/#/c/563375/17/nova/objects/instance_group.py@75\n\nI don\u0027t see where we actually call InstanceGroupPolicy.create() in any runtime code.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"ee7030bd7f1c6f8163f76304b24fe432897502fd","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        return group_policy"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    @base.remotable"},{"line_number":57,"context_line":"    def create(self):"},{"line_number":58,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":59,"context_line":"        return self._from_db_object(self._context, self, db_policy)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_6f377a4d","line":57,"in_reply_to":"5f7c97a3_639a4014","updated":"2018-06-28 11:31:20.000000000","message":"oh sorry, I noticed your comment in [1].\n[1] https://review.openstack.org/#/c/563375/17/nova/objects/instance_group.py@75\n\nI will bring it back in next PS. : )","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f3ec76ea6eaaf9f65c5e9eb216c3331467c2fc19","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        return group_policy"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    @base.remotable"},{"line_number":57,"context_line":"    def create(self):"},{"line_number":58,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":59,"context_line":"        return self._from_db_object(self._context, self, db_policy)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_3bff8436","line":57,"in_reply_to":"5f7c97a3_6f377a4d","updated":"2018-06-28 13:53:47.000000000","message":"Yeah I like having the create() on this object so the InstanceGroup code doesn\u0027t have to do it.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e346e8456fd687ae3c5a39de6a4c7b0373785522","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        return group_policy"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    @base.remotable"},{"line_number":57,"context_line":"    def create(self):"},{"line_number":58,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":59,"context_line":"        return self._from_db_object(self._context, self, db_policy)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_defe9938","line":57,"in_reply_to":"5f7c97a3_be195dd2","updated":"2018-06-27 16:11:00.000000000","message":"Looking at the API handler code toward the end of the series, we construct the InstanceGroupPolicy object, set it on the InstanceGroup.policy field, and then call InstanceGroup.create():\n\nhttps://review.openstack.org/#/c/567534/16/nova/api/openstack/compute/server_groups.py@209\n\nSo either we don\u0027t need this InstanceGroupPolicy.create() method, or https://review.openstack.org/#/c/563375/17/nova/objects/instance_group.py@75 should be changed to just call policy.create() (this method in the new object).","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        return group_policy"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    @base.remotable"},{"line_number":57,"context_line":"    def create(self):"},{"line_number":58,"context_line":"        db_policy \u003d self._create_in_db(self._context, self.obj_get_changes())"},{"line_number":59,"context_line":"        return self._from_db_object(self._context, self, db_policy)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_639a4014","line":57,"in_reply_to":"5f7c97a3_defe9938","updated":"2018-06-28 03:46:12.000000000","message":"Yes, the create method is not necessary anymore in current implementation, I will remove it.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"}],"nova/tests/unit/objects/test_instance_group.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e4df3319db95ae9d9dedce7bbcbc84c11ff47f02","unresolved":false,"context_lines":[{"line_number":374,"context_line":"        obj \u003d objects.InstanceGroupPolicy(context\u003dself.context)"},{"line_number":375,"context_line":"        obj.group_id \u003d _INST_GROUP_POLICY_DB[\u0027group_id\u0027]"},{"line_number":376,"context_line":"        obj.policy \u003d _INST_GROUP_POLICY_DB[\u0027policy\u0027]"},{"line_number":377,"context_line":"        obj.rules_json \u003d _INST_GROUP_POLICY_DB[\u0027rules\u0027]"},{"line_number":378,"context_line":"        obj.create()"},{"line_number":379,"context_line":"        self.assertEqual(set([]), obj.obj_what_changed())"},{"line_number":380,"context_line":"        mock_create.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_0eb4bbd3","line":377,"range":{"start_line":377,"start_character":8,"end_line":377,"end_character":55},"updated":"2018-06-28 19:19:42.000000000","message":"This is a bit weird and not how someone would use this object. Instead, do something like this:\n\nobj.rules \u003d jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])\n\nThen the assertion below is going to correctly check that we serialized it when inserting it into the database.","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"79b57170475cfa3c5378068f3425ca2ac9a50cc1","unresolved":false,"context_lines":[{"line_number":374,"context_line":"        obj \u003d objects.InstanceGroupPolicy(context\u003dself.context)"},{"line_number":375,"context_line":"        obj.group_id \u003d _INST_GROUP_POLICY_DB[\u0027group_id\u0027]"},{"line_number":376,"context_line":"        obj.policy \u003d _INST_GROUP_POLICY_DB[\u0027policy\u0027]"},{"line_number":377,"context_line":"        obj.rules_json \u003d _INST_GROUP_POLICY_DB[\u0027rules\u0027]"},{"line_number":378,"context_line":"        obj.create()"},{"line_number":379,"context_line":"        self.assertEqual(set([]), obj.obj_what_changed())"},{"line_number":380,"context_line":"        mock_create.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_31feb7af","line":377,"range":{"start_line":377,"start_character":8,"end_line":377,"end_character":55},"in_reply_to":"5f7c97a3_0eb4bbd3","updated":"2018-06-29 01:28:42.000000000","message":"Done","commit_id":"8eeaab5b11d2bbb932f26b06aea9f0ff82b2e9a7"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"592ee6f99ba8e67b73096ae4ed378de3040d04ac","unresolved":false,"context_lines":[{"line_number":15,"context_line":"import copy"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"import mock"},{"line_number":18,"context_line":"from oslo_serialization import jsonutils"},{"line_number":19,"context_line":"from oslo_utils import timeutils"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"from nova import exception"}],"source_content_type":"text/x-python","patch_set":16,"id":"5f7c97a3_63f93780","line":18,"range":{"start_line":18,"start_character":0,"end_line":18,"end_character":40},"updated":"2018-07-06 11:20:09.000000000","message":"remove","commit_id":"4aab737ae8f2ac496f71a45f8e346fa03667b4bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"57ca303192e53b090bdab9ee8a6473b39396c99f","unresolved":false,"context_lines":[{"line_number":423,"context_line":"    def test_get_set_rules_None(self):"},{"line_number":424,"context_line":"        obj \u003d objects.InstanceGroupPolicy(context\u003dself.context)"},{"line_number":425,"context_line":"        obj._rules \u003d None"},{"line_number":426,"context_line":"        self.assertEqual(obj.rules, {})"},{"line_number":427,"context_line":"        self.assertIsNone(obj._rules)"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_19ceb45a","line":426,"range":{"start_line":426,"start_character":36,"end_line":426,"end_character":38},"updated":"2018-07-06 13:28:59.000000000","message":"If _rules is nullable it seems sort of weird that we\u0027d get {} instead of None here. In other words, this is odd:\n\nobj \u003d objects.InstanceGroupPolicy(context\u003dself.context, rules\u003dNone)\nself.assertEqual({}, obj.rules)","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"c33104a323bc6d9fc00bbb873bfd7af8842d6ff2","unresolved":false,"context_lines":[{"line_number":423,"context_line":"    def test_get_set_rules_None(self):"},{"line_number":424,"context_line":"        obj \u003d objects.InstanceGroupPolicy(context\u003dself.context)"},{"line_number":425,"context_line":"        obj._rules \u003d None"},{"line_number":426,"context_line":"        self.assertEqual(obj.rules, {})"},{"line_number":427,"context_line":"        self.assertIsNone(obj._rules)"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"5f7c97a3_9eedb325","line":426,"range":{"start_line":426,"start_character":36,"end_line":426,"end_character":38},"in_reply_to":"5f7c97a3_19ceb45a","updated":"2018-07-09 07:36:10.000000000","message":"Done","commit_id":"d09f55aa00cc7e029186d6f6e6d23aae55eac115"}],"nova/tests/unit/objects/test_instance_group_policy.py":[{"author":{"_account_id":7634,"name":"Takashi Natsume","email":"takanattie@gmail.com","username":"natsumet"},"change_message_id":"8ed22307f77af760dbfb70ddf49490f256d12faa","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import mock"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from oslo_serialization import jsonutils"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from nova import objects"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_2e403ba2","line":16,"updated":"2018-06-21 02:40:50.000000000","message":"Unnecessary blank line.","commit_id":"ada8eb1a8d3c7ae6411bad6271b183624dde9839"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dcfc87de0dcf1f409f6d7b5b4c1adbc831bfa284","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import mock"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from oslo_serialization import jsonutils"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from nova import objects"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_5aad31e8","line":16,"in_reply_to":"5f7c97a3_2e403ba2","updated":"2018-06-21 06:46:35.000000000","message":"Done","commit_id":"ada8eb1a8d3c7ae6411bad6271b183624dde9839"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2013 OpenStack Foundation"},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_6d9d5eb9","line":1,"updated":"2018-06-27 13:58:26.000000000","message":"You can remove this old copyright line.\n\nAlso, don\u0027t need a new module for this, just put it in test_instance_group.py.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from nova.objects import instance_group_policy"},{"line_number":20,"context_line":"from nova.tests.unit.objects import test_objects"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"_INST_GROUP_POLICY_DB \u003d {"},{"line_number":23,"context_line":"    \u0027group_id\u0027: 1,"},{"line_number":24,"context_line":"    \u0027policy\u0027: \u0027anti-affinity\u0027,"},{"line_number":25,"context_line":"    \u0027rules\u0027: \u0027{\"max_server_per_host\": \"3\"}\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_adae16e1","line":22,"updated":"2018-06-27 13:58:26.000000000","message":"The db record would have an id field.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from nova.objects import instance_group_policy"},{"line_number":20,"context_line":"from nova.tests.unit.objects import test_objects"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"_INST_GROUP_POLICY_DB \u003d {"},{"line_number":23,"context_line":"    \u0027group_id\u0027: 1,"},{"line_number":24,"context_line":"    \u0027policy\u0027: \u0027anti-affinity\u0027,"},{"line_number":25,"context_line":"    \u0027rules\u0027: \u0027{\"max_server_per_host\": \"3\"}\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_065cba17","line":22,"in_reply_to":"5f7c97a3_adae16e1","updated":"2018-06-28 03:46:12.000000000","message":"Done","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        obj.group_id \u003d _INST_GROUP_POLICY_DB[\u0027group_id\u0027]"},{"line_number":35,"context_line":"        obj.policy \u003d _INST_GROUP_POLICY_DB[\u0027policy\u0027]"},{"line_number":36,"context_line":"        obj.rules \u003d jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])"},{"line_number":37,"context_line":"        obj.create()"},{"line_number":38,"context_line":"        mock_create.assert_called_once_with("},{"line_number":39,"context_line":"            self.context, {"},{"line_number":40,"context_line":"                \u0027group_id\u0027: _INST_GROUP_POLICY_DB[\u0027group_id\u0027],"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_8dfd5ab3","line":37,"updated":"2018-06-27 13:58:26.000000000","message":"It would be good to assert this once you\u0027ve created the object to make sure changes are reset:\n\nself.assertEqual(set([]), obj.obj_what_changed())","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":39,"context_line":"            self.context, {"},{"line_number":40,"context_line":"                \u0027group_id\u0027: _INST_GROUP_POLICY_DB[\u0027group_id\u0027],"},{"line_number":41,"context_line":"                \u0027policy\u0027: _INST_GROUP_POLICY_DB[\u0027policy\u0027],"},{"line_number":42,"context_line":"                \u0027rules\u0027: jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])})"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    @mock.patch(\u0027nova.objects.InstanceGroup._create_in_db\u0027,"},{"line_number":45,"context_line":"                return_value\u003d_INST_GROUP_POLICY_DB)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_6dd47e47","line":42,"range":{"start_line":42,"start_character":35,"end_line":42,"end_character":40},"updated":"2018-06-27 13:58:26.000000000","message":"This is wrong - why isn\u0027t the test failing? We\u0027re passing a serialized version of \u0027rules\u0027 to the sqlalchemy ORM insert routine.\n\n(later)\n\nOh I see, you\u0027ve mocked out _create_in_db so we don\u0027t actually test that _create_in_db is serializing the rules dict before inserting it into the database...hmmm, it would be nice if we could test that, but I\u0027m not exactly sure how, unless you mock the API model.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f3ec76ea6eaaf9f65c5e9eb216c3331467c2fc19","unresolved":false,"context_lines":[{"line_number":39,"context_line":"            self.context, {"},{"line_number":40,"context_line":"                \u0027group_id\u0027: _INST_GROUP_POLICY_DB[\u0027group_id\u0027],"},{"line_number":41,"context_line":"                \u0027policy\u0027: _INST_GROUP_POLICY_DB[\u0027policy\u0027],"},{"line_number":42,"context_line":"                \u0027rules\u0027: jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])})"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    @mock.patch(\u0027nova.objects.InstanceGroup._create_in_db\u0027,"},{"line_number":45,"context_line":"                return_value\u003d_INST_GROUP_POLICY_DB)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_7bcc1c34","line":42,"range":{"start_line":42,"start_character":35,"end_line":42,"end_character":40},"in_reply_to":"5f7c97a3_222e798c","updated":"2018-06-28 13:53:47.000000000","message":"To be clear, we don\u0027t have to do that, it\u0027s just an idea. I\u0027m not sure how much cleaner it makes all of this though.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"ee7030bd7f1c6f8163f76304b24fe432897502fd","unresolved":false,"context_lines":[{"line_number":39,"context_line":"            self.context, {"},{"line_number":40,"context_line":"                \u0027group_id\u0027: _INST_GROUP_POLICY_DB[\u0027group_id\u0027],"},{"line_number":41,"context_line":"                \u0027policy\u0027: _INST_GROUP_POLICY_DB[\u0027policy\u0027],"},{"line_number":42,"context_line":"                \u0027rules\u0027: jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])})"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    @mock.patch(\u0027nova.objects.InstanceGroup._create_in_db\u0027,"},{"line_number":45,"context_line":"                return_value\u003d_INST_GROUP_POLICY_DB)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_222e798c","line":42,"range":{"start_line":42,"start_character":35,"end_line":42,"end_character":40},"in_reply_to":"5f7c97a3_6dd47e47","updated":"2018-06-28 11:31:20.000000000","message":"and I saw your comment about use ._rules to reduce redundunt serialize code, also I found [1] which the same way you mentioned.\n\n[1] https://github.com/openstack/nova/blob/4e7672d/nova/objects/migrate_data.py#L100-L111\n\nI will make it work soon. :)","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2a53895c4fd68be44d3c46bdd2d3b974fa51c25a","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                \u0027policy\u0027: _INST_GROUP_POLICY_DB[\u0027policy\u0027],"},{"line_number":42,"context_line":"                \u0027rules\u0027: jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])})"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    @mock.patch(\u0027nova.objects.InstanceGroup._create_in_db\u0027,"},{"line_number":45,"context_line":"                return_value\u003d_INST_GROUP_POLICY_DB)"},{"line_number":46,"context_line":"    def test_from_db(self, mock_create):"},{"line_number":47,"context_line":"        inst \u003d instance_group_policy.InstanceGroupPolicy(self.context)"},{"line_number":48,"context_line":"        policy \u003d inst._from_db_object("}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_4d0422c6","line":45,"range":{"start_line":44,"start_character":4,"end_line":45,"end_character":51},"updated":"2018-06-27 13:58:26.000000000","message":"This isn\u0027t used, and you could merge the rest of this test into test_create.","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"dce67d7c0d3109de8cfc0649e438559023d4a596","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                \u0027policy\u0027: _INST_GROUP_POLICY_DB[\u0027policy\u0027],"},{"line_number":42,"context_line":"                \u0027rules\u0027: jsonutils.loads(_INST_GROUP_POLICY_DB[\u0027rules\u0027])})"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    @mock.patch(\u0027nova.objects.InstanceGroup._create_in_db\u0027,"},{"line_number":45,"context_line":"                return_value\u003d_INST_GROUP_POLICY_DB)"},{"line_number":46,"context_line":"    def test_from_db(self, mock_create):"},{"line_number":47,"context_line":"        inst \u003d instance_group_policy.InstanceGroupPolicy(self.context)"},{"line_number":48,"context_line":"        policy \u003d inst._from_db_object("}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_06159a3d","line":45,"range":{"start_line":44,"start_character":4,"end_line":45,"end_character":51},"in_reply_to":"5f7c97a3_4d0422c6","updated":"2018-06-28 03:46:12.000000000","message":"Done","commit_id":"c51e8dea3ff93b8902a40d8e8152de75a01fe094"}]}