)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"4d030430b8b9d676befd744dd4a56f8e6b6ddbe6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"831e8ee6_25f1abf9","updated":"2022-11-29 09:31:05.000000000","message":"Globally these changes LGTM. You have a couple of pep8 error but nothing bad.\n\nPlease can you add a release note and some specific tests to ensure that this scenario is well covered over the years.\nMaybe somewhere here:\nhttps://opendev.org/openstack/oslo.limit/src/branch/master/oslo_limit/tests/test_limit.py\n\nThis file seems to host a couple of tests related to endpoint.","commit_id":"37f7acec4edf15163d87de3bf9d9dafca4bfb0c0"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"d3c66da69c3f13d6c284173a756460139cf653e0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"75399195_043b267b","in_reply_to":"091306e0_03d3e73f","updated":"2022-12-11 06:25:38.000000000","message":"Done","commit_id":"37f7acec4edf15163d87de3bf9d9dafca4bfb0c0"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"22c460e72a01cef1f9d665f662eecff2966686d5","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"091306e0_03d3e73f","in_reply_to":"831e8ee6_25f1abf9","updated":"2022-11-29 11:12:27.000000000","message":"Sure, I will update with release note and test cases. Thanks!","commit_id":"37f7acec4edf15163d87de3bf9d9dafca4bfb0c0"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"d3c66da69c3f13d6c284173a756460139cf653e0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"85bc8914_48e839fd","updated":"2022-12-11 06:25:38.000000000","message":"Hi Herve, the testcase and release note were added.","commit_id":"99172f9c2db51369ec85145285a3fbc3b1b257a7"},{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"4f66879538552bcdd3346945115e6e7f74b306a4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"cf23db2d_0e5644ed","updated":"2022-12-12 09:02:51.000000000","message":"LGTM","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"27629ff87f375e443e9ab9dc64bccd71fef67904","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"24b94652_cb3da131","updated":"2023-02-03 16:18:41.000000000","message":"WIP","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"}],"oslo_limit/limit.py":[{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"adb040e479a22ae78242d1f38b6a6fce76c05440","unresolved":true,"context_lines":[{"line_number":413,"context_line":"def _get_endpoint_id_from_keystone():"},{"line_number":414,"context_line":"    \u0027\u0027\u0027Return endpoint_id from CONF or from keystone."},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"    If from keystone, write into CONF(memery, not file). So"},{"line_number":417,"context_line":"    that we only need connect to keystone at most once."},{"line_number":418,"context_line":""},{"line_number":419,"context_line":"    :return: endpoint_id, can be None just as the legacy act."}],"source_content_type":"text/x-python","patch_set":3,"id":"9dfc2467_d73fbf4b","line":416,"range":{"start_line":416,"start_character":22,"end_line":416,"end_character":55},"updated":"2023-02-01 01:47:23.000000000","message":"I\u0027d prefer not relying on oslo.config interface but use a global var.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"471117e4b71c162cf4461a91b77a55305789a00f","unresolved":true,"context_lines":[{"line_number":413,"context_line":"def _get_endpoint_id_from_keystone():"},{"line_number":414,"context_line":"    \u0027\u0027\u0027Return endpoint_id from CONF or from keystone."},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"    If from keystone, write into CONF(memery, not file). So"},{"line_number":417,"context_line":"    that we only need connect to keystone at most once."},{"line_number":418,"context_line":""},{"line_number":419,"context_line":"    :return: endpoint_id, can be None just as the legacy act."}],"source_content_type":"text/x-python","patch_set":3,"id":"831c5040_e6f59b5f","line":416,"range":{"start_line":416,"start_character":22,"end_line":416,"end_character":55},"in_reply_to":"1f9983a1_f3eb32dd","updated":"2023-02-06 14:51:18.000000000","message":"\u003e Don\u0027t need to change [the place where CONF.oslo_limit.endpoint_id in use] in elsewhere.\nMy main concern is this point, in case we are adding any logic which requires raw input then the current overrides result in confusing behavior. I know it\u0027s not yet happened but I\u0027d avoid override as much as possible so that we can separate actual user input and how it is translated in logics.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"27629ff87f375e443e9ab9dc64bccd71fef67904","unresolved":false,"context_lines":[{"line_number":413,"context_line":"def _get_endpoint_id_from_keystone():"},{"line_number":414,"context_line":"    \u0027\u0027\u0027Return endpoint_id from CONF or from keystone."},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"    If from keystone, write into CONF(memery, not file). So"},{"line_number":417,"context_line":"    that we only need connect to keystone at most once."},{"line_number":418,"context_line":""},{"line_number":419,"context_line":"    :return: endpoint_id, can be None just as the legacy act."}],"source_content_type":"text/x-python","patch_set":3,"id":"1f9983a1_f3eb32dd","line":416,"range":{"start_line":416,"start_character":22,"end_line":416,"end_character":55},"in_reply_to":"9dfc2467_d73fbf4b","updated":"2023-02-03 16:18:41.000000000","message":"Thanks for comments!\nThat\u0027s a choice. but use \u0027CONF.set_override\u0027:\n* We can check/convert the value by opts defination.\n* Don\u0027t need to change [the place where CONF.oslo_limit.endpoint_id in use] in elsewhere.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"adb040e479a22ae78242d1f38b6a6fce76c05440","unresolved":true,"context_lines":[{"line_number":427,"context_line":"        if CONF.oslo_limit.service_name and \\"},{"line_number":428,"context_line":"                CONF.oslo_limit.service_type:"},{"line_number":429,"context_line":"            conn \u003d _get_keystone_connection()"},{"line_number":430,"context_line":"            conn.service_name \u003d CONF.oslo_limit.service_name"},{"line_number":431,"context_line":"            conn.service_type \u003d CONF.oslo_limit.service_type"},{"line_number":432,"context_line":"            # Actually region_name is optional."},{"line_number":433,"context_line":"            if CONF.oslo_limit.region_name:"}],"source_content_type":"text/x-python","patch_set":3,"id":"8f3d5b22_10916331","line":430,"range":{"start_line":430,"start_character":48,"end_line":430,"end_character":60},"updated":"2023-02-01 01:47:23.000000000","message":"so these options come from adaptor parameters. Doesn\u0027t this usage conflict with the parameter being used to discover keystone endpoint ? We probably should have new parameters. See the previous discussion in https://review.opendev.org/c/openstack/oslo.limit/+/726929","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"471117e4b71c162cf4461a91b77a55305789a00f","unresolved":true,"context_lines":[{"line_number":427,"context_line":"        if CONF.oslo_limit.service_name and \\"},{"line_number":428,"context_line":"                CONF.oslo_limit.service_type:"},{"line_number":429,"context_line":"            conn \u003d _get_keystone_connection()"},{"line_number":430,"context_line":"            conn.service_name \u003d CONF.oslo_limit.service_name"},{"line_number":431,"context_line":"            conn.service_type \u003d CONF.oslo_limit.service_type"},{"line_number":432,"context_line":"            # Actually region_name is optional."},{"line_number":433,"context_line":"            if CONF.oslo_limit.region_name:"}],"source_content_type":"text/x-python","patch_set":3,"id":"897e55a3_a997a2d4","line":430,"range":{"start_line":430,"start_character":48,"end_line":430,"end_character":60},"in_reply_to":"1a108ec3_ec50bdd4","updated":"2023-02-06 14:51:18.000000000","message":"My point is that these ksa options are used to look up the keystone endpoint from catalog (to access limit information), and you can\u0027t really change these as it\u0027d break that endpoint detection.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"27629ff87f375e443e9ab9dc64bccd71fef67904","unresolved":false,"context_lines":[{"line_number":427,"context_line":"        if CONF.oslo_limit.service_name and \\"},{"line_number":428,"context_line":"                CONF.oslo_limit.service_type:"},{"line_number":429,"context_line":"            conn \u003d _get_keystone_connection()"},{"line_number":430,"context_line":"            conn.service_name \u003d CONF.oslo_limit.service_name"},{"line_number":431,"context_line":"            conn.service_type \u003d CONF.oslo_limit.service_type"},{"line_number":432,"context_line":"            # Actually region_name is optional."},{"line_number":433,"context_line":"            if CONF.oslo_limit.region_name:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a108ec3_ec50bdd4","line":430,"range":{"start_line":430,"start_character":48,"end_line":430,"end_character":60},"in_reply_to":"8f3d5b22_10916331","updated":"2023-02-03 16:18:41.000000000","message":"Thanks! I read the comments in https://review.opendev.org/c/openstack/oslo.limit/+/726929 and https://review.opendev.org/c/openstack/oslo.limit/+/733881.\n\n\u003e Should https://github.com/openstack/oslo.limit/blob/master/oslo_limit/limit.py#L34\n\u003e be loading from the olso_limit group or should that be using\n\u003e keystone_authtoken?\n\u003e keystone_authtoken is specific to keystonemiddleware, it should not be reused here.\n\nIn other openstack products, **the ksa opts are widely used**. such as ironic/cyborg and many other nova product.\nref: https://opendev.org/openstack/nova/src/branch/master/nova/conf/ironic.py#L99\nhttps://opendev.org/openstack/nova/src/branch/master/nova/conf/glance.py#L208\n\nI believe the values are isolated by different register_group_name(I can test later). Changing the oslo_limit.service_name/region_name does not afffect anywhere else. So I think we don\u0027t need register new prefix_xxx. What do you think?\n\nKindly correct me if I miss understand. 😊","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"adb040e479a22ae78242d1f38b6a6fce76c05440","unresolved":true,"context_lines":[{"line_number":437,"context_line":"            CONF.set_override(\u0027endpoint_id\u0027, epid, group\u003d\u0027oslo_limit\u0027)"},{"line_number":438,"context_line":"        else:"},{"line_number":439,"context_line":"            raise ValueError(\"service name and type aren\u0027t given\")"},{"line_number":440,"context_line":"    except Exception as e:"},{"line_number":441,"context_line":"        msg \u003d \u0027Get endpoint id from keystone failed: %s\u0027 % e"},{"line_number":442,"context_line":"        LOG.error(msg)"},{"line_number":443,"context_line":"    return epid"}],"source_content_type":"text/x-python","patch_set":3,"id":"7ab9ba22_3ed5088a","line":440,"range":{"start_line":440,"start_character":4,"end_line":440,"end_character":25},"updated":"2023-02-01 01:47:23.000000000","message":"we have to reraise the exception, otherwise invalid id (None) is returned. I understand this is an issue with the current implementation but I prefer fixing it now.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"},{"author":{"_account_id":34325,"name":"Jneeee","email":"jneeee@qq.com","username":"junnan.liu"},"change_message_id":"27629ff87f375e443e9ab9dc64bccd71fef67904","unresolved":true,"context_lines":[{"line_number":437,"context_line":"            CONF.set_override(\u0027endpoint_id\u0027, epid, group\u003d\u0027oslo_limit\u0027)"},{"line_number":438,"context_line":"        else:"},{"line_number":439,"context_line":"            raise ValueError(\"service name and type aren\u0027t given\")"},{"line_number":440,"context_line":"    except Exception as e:"},{"line_number":441,"context_line":"        msg \u003d \u0027Get endpoint id from keystone failed: %s\u0027 % e"},{"line_number":442,"context_line":"        LOG.error(msg)"},{"line_number":443,"context_line":"    return epid"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a363633_3d80eab5","line":440,"range":{"start_line":440,"start_character":4,"end_line":440,"end_character":25},"in_reply_to":"7ab9ba22_3ed5088a","updated":"2023-02-03 16:18:41.000000000","message":"Accept.","commit_id":"7622ed88414f033257ab96e46136d6d2e4ec2c4e"}]}