)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"8a1dfdee5a585ed9d8bd5bb76e75ae911c8e721f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c9e8963a_f2ef59c0","updated":"2024-01-23 18:39:38.000000000","message":"About done with this and ready for review. Only issue is listing the services with \u0027--long\u0027, the \u0027Enabled\u0027 column is empty instead of returning True. I will need help with that since I couldn\u0027t figure out how to fix it.","commit_id":"a54dbc8b858bf8ee1570a995bc13a540b47d1906"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"71aae55f50a4adf7268b8f0ead02bdc24e868731","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1b937941_65f32de6","updated":"2024-01-24 11:59:10.000000000","message":"The test failures are happening because SDK prefixes all boolean values with `is_`, so `enabled` becomes `is_enabled`. You need to handle this. You also need a release note. I\u0027ll push a follow-up to address both issues shortly, which you can use as a blueprint going forward then 😊","commit_id":"a54dbc8b858bf8ee1570a995bc13a540b47d1906"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"40dbfbdf529858f5e9106ac61ce8bfd8e4ac10fb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e9340f2a_335216fd","updated":"2024-01-26 12:02:01.000000000","message":"Looks like you fixed one of the failing tests but there are more to fix here. You can run _just_ the tests in `test_service.py` like so:\n\n```\ntox -e functional -- -n openstackclient/tests/functional/identity/v3/test_service.py\n```","commit_id":"caa9c74e7885a2a088c25c77be50fee89a65696b"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"0ed5c2aed1575cdf09efb22fc7cdb9b0edd712d2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"62c868ff_b7376421","updated":"2024-01-25 17:39:14.000000000","message":"Ready for review again!","commit_id":"caa9c74e7885a2a088c25c77be50fee89a65696b"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"d62e33d2c35ecec96be88bb2e0d74c4dfbe8258d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e412ce3c_503b208b","in_reply_to":"e9340f2a_335216fd","updated":"2024-01-26 22:28:44.000000000","message":"Oh okay, I had run `tox -e functional -- -n openstackclient.tests.functional.identity.v3.test_service.ServiceTests` instead. Is there a difference?","commit_id":"caa9c74e7885a2a088c25c77be50fee89a65696b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d8190ec4c0d6f64e55695e07aa86f42f4bcd82ef","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b0f91ded_17374844","updated":"2024-02-02 10:36:02.000000000","message":"Comparing a [recent successful build](https://61399e755e944b799ac7-40d76dc3f646b86e4453b642950e6efd.ssl.cf5.rackcdn.com/887976/7/check/osc-functional-devstack/2c5e024/job-output.txt) of the `osc-functional-devstack` job, I see:\n\n```\n2024-01-31 20:53:30.681709 | controller | + lib/neutron_plugins/services/l3:is_networking_extension_supported:433 :   local extension\u003dauto-allocated-topology\n2024-01-31 20:53:30.685786 | controller | ++ lib/neutron_plugins/services/l3:is_networking_extension_supported:435 :   oscwrap --os-cloud devstack-admin --os-region RegionOne extension list --network -c Alias -f value\n2024-01-31 20:53:33.702872 | controller | ++ functions-common:oscwrap:2461            :   return 0\n2024-01-31 20:53:33.710672 | controller | + lib/neutron_plugins/services/l3:is_networking_extension_supported:435 :   EXT_LIST\u003d\u0027address-group\n2024-01-31 20:53:33.712344 | controller | address-scope\n...\n```\n\nversus:\n\n```\n2024-02-01 10:17:03.892439 | controller | + lib/neutron_plugins/services/l3:is_networking_extension_supported:433 :   local extension\u003dauto-allocated-topology\n2024-02-01 10:17:03.897283 | controller | ++ lib/neutron_plugins/services/l3:is_networking_extension_supported:435 :   oscwrap --os-cloud devstack-admin --os-region RegionOne extension list --network -c Alias -f value\n2024-02-01 10:17:05.942829 | controller | public endpoint for network service in RegionOne region not found\n2024-02-01 10:17:06.144751 | controller | ++ functions-common:oscwrap:2461            :   return 1\n2024-02-01 10:17:06.148476 | controller | + lib/neutron_plugins/services/l3:is_networking_extension_supported:435 :   EXT_LIST\u003d\n2024-02-01 10:17:06.152277 | controller | + lib/neutron_plugins/services/l3:is_networking_extension_supported:436 :   [[ \u0027\u0027 \u003d~ auto-allocated-topology ]]\n2024-02-01 10:17:06.155484 | controller | + lib/neutron_plugins/services/l3:create_neutron_initial_network:172 :   is_provider_network\n2024-02-01 10:17:06.158225 | controller | + functions-common:is_provider_network:2270 :   \u0027[\u0027 \u0027\u0027 \u003d\u003d True \u0027]\u0027\n2024-02-01 10:17:06.161803 | controller | + functions-common:is_provider_network:2273 :   return 1\n2024-02-01 10:17:06.166233 | controller | ++ lib/neutron_plugins/services/l3:create_neutron_initial_network:202 :   oscwrap --os-cloud devstack --os-region RegionOne network create private -f value -c id\n2024-02-01 10:17:08.145608 | controller | usage: openstack network create [-h] [-f {json,shell,table,value,yaml}]\n2024-02-01 10:17:08.145684 | controller |                                 [-c COLUMN] [--noindent] [--prefix PREFIX]\n2024-02-01 10:17:08.145742 | controller |                                 [--max-width \u003cinteger\u003e] [--fit-width]\n2024-02-01 10:17:08.145808 | controller |                                 [--print-empty]\n2024-02-01 10:17:08.145878 | controller |                                 [--extra-property type\u003d\u003cproperty_type\u003e,name\u003d\u003cproperty_name\u003e,value\u003d\u003cproperty_value\u003e]\n2024-02-01 10:17:08.145944 | controller |                                 [--share | --no-share] --subnet \u003csubnet\u003e\n2024-02-01 10:17:08.146009 | controller |                                 \u003cname\u003e\n2024-02-01 10:17:08.146094 | controller | openstack network create: error: the following arguments are required: --subnet\n```\n\nthis is the first issue:\n\n```\n2024-02-01 10:17:05.942829 | controller | public endpoint for network service in RegionOne region not found\n```\n\nbased on that, it sounds like the service isn\u0027t getting created correctly, or is not getting created with the correct region info.","commit_id":"95ae1cb6ae1b26b2660bf0ce3ab4fa7419f764c4"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"96d82a2de47945b4d3b75667c9f346438d66d871","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4f92f0dc_25444854","updated":"2024-02-01 09:42:04.000000000","message":"recheck unrelated failure","commit_id":"95ae1cb6ae1b26b2660bf0ce3ab4fa7419f764c4"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"fda0f35330a371d21ab74954c5f79cb629c7aa20","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"42e530f8_15ef01f1","in_reply_to":"b0f91ded_17374844","updated":"2024-02-02 11:41:03.000000000","message":"My suggestion here is to split this patch into four `DNM` patches - one for each converted command - and push them to see where the failures occur. This will help us narrow things down. Alternatively, you can try running the commands twice - once with this patch and once without - to see the differences in the created services. e.g.\n\n```\ncd python-openstackclient\nvirtualenv .venv\nsource .venv/bin/activate\npip install -e .\ngit checkout {this-branch}\nopenstack service create ...\nopenstack service show ...\nopenstack service list ...\nopenstack service update ...\nopenstack service delete ...\n# store results for all of the above\ngit checkout master\nopenstack service create ...\n# etc. - rinse and repeat\n```","commit_id":"95ae1cb6ae1b26b2660bf0ce3ab4fa7419f764c4"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"a5298e87bd188026b1160a35e5c22d3f4d479df0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"277e3947_7b7ae803","updated":"2024-02-05 21:32:06.000000000","message":"Patchset 5: testing create service commands with Zuul","commit_id":"60182dd79e5e31fb178c345da5503b453e2d882c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"bfd5e18aa7f426df5ab5a83e32c93d694f494042","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"d6e5ce24_33da1c98","updated":"2024-02-05 21:37:22.000000000","message":"Patchset 6: testing delete service commands with Zuul","commit_id":"f0e1b6f604357e67e1d480ad6c3edfbfcf6f8fda"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"049181bce77b30df9ebbd830c9efdc1515024489","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"b7c551c7_0073e9c3","updated":"2024-02-13 11:53:23.000000000","message":"Okay, so we know that the `service show` command is what\u0027s causing the difference. Nice work. Now to debug this further, I would suggest you go compare the logs from this failing job versus a passing job. You can find other runs of the job by clicking the *View build history* link on the right of a job result page (e.g. from [the last CI run for this patch](https://zuul.opendev.org/t/openstack/build/9e5f04b8fb1a43e186c11c2c4b75ee49)). The CI result shows us that we\u0027re failing to stack, so we want to look at the logs from the `stack.sh` invocation which can be found at `controller/logs/devstacklog.txt` under the *Logs* tab. Search the file for invocations of `service show` and see what the difference between the failing case and a success case are.\n\nPS: By way of a *gigantic* hint, consider what happens when you try to run `service show` for a service that doesn\u0027t exist (yet)...","commit_id":"8d2a366a29b83cee1d79ed9656d3dc89fecfa580"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"33c4ff36530a929d32b480fab2789a8e7311ae30","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"d1b701b7_09321739","updated":"2024-06-12 16:43:10.000000000","message":"recheck: the functional test failure is unrelated","commit_id":"a408640256b7c8fead95264e8142dc4648bf7950"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d3f4b5b6f3661e485721504f8ce17e35190fe152","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":22,"id":"5aa0168a_cda4a053","updated":"2024-07-03 20:52:55.000000000","message":"Nice work here. Some nits in the common module but I\u0027m otherwise +2","commit_id":"72aec3faf93e6ae9512304f3d02ecba469120de2"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"56c56d858ebbace4f8bdfcbba5e42ed9c6d6a0b2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"9eb361ce_21be37f0","updated":"2024-07-09 09:54:50.000000000","message":"One final outstanding issue I think","commit_id":"cd8d5fc9a6a2aa0314abf8b232480a41c35f299c"}],"openstackclient/identity/common.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d3f4b5b6f3661e485721504f8ce17e35190fe152","unresolved":true,"context_lines":[{"line_number":92,"context_line":"    result \u003d None"},{"line_number":93,"context_line":"    match \u003d 0"},{"line_number":94,"context_line":"    for i in data:"},{"line_number":95,"context_line":"        if name_type_or_id \u003d\u003d getattr(i, \u0027type\u0027):"},{"line_number":96,"context_line":"            match +\u003d 1"},{"line_number":97,"context_line":"            result \u003d i"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"6b4edb7d_95cb9252","line":95,"range":{"start_line":95,"start_character":30,"end_line":95,"end_character":49},"updated":"2024-07-03 20:52:55.000000000","message":"Rather than doing this, you should be able to just do:\n\n```\ni.type\n```\n\nAlso, the name `i` suggest this is an integer value when it clearly isn\u0027t. Perhaps you could rename these like:\n\n\n```\nservices \u003d identity_client.services()\nresult \u003d None\nfor service in services:\n    ...\n```\n\nFinally, `match` is a reserved keyword in Python 3.12 (I think). It\u0027s also unnecessary. You can simply check if `result is not None` inside the loop and fail if so (since that means you found a match).\n\n```\nfor service in services:\n    if name_type_or_id \u003d\u003d service.type:\n        if result:\n            raise ...\n        result \u003d service\n```","commit_id":"72aec3faf93e6ae9512304f3d02ecba469120de2"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"aadc40c9518d26bbbb3cf8c4fc8101ea265c7fe3","unresolved":false,"context_lines":[{"line_number":92,"context_line":"    result \u003d None"},{"line_number":93,"context_line":"    match \u003d 0"},{"line_number":94,"context_line":"    for i in data:"},{"line_number":95,"context_line":"        if name_type_or_id \u003d\u003d getattr(i, \u0027type\u0027):"},{"line_number":96,"context_line":"            match +\u003d 1"},{"line_number":97,"context_line":"            result \u003d i"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"f0540610_345542a0","line":95,"range":{"start_line":95,"start_character":30,"end_line":95,"end_character":49},"in_reply_to":"6b4edb7d_95cb9252","updated":"2024-07-08 16:19:31.000000000","message":"Acknowledged","commit_id":"72aec3faf93e6ae9512304f3d02ecba469120de2"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"56c56d858ebbace4f8bdfcbba5e42ed9c6d6a0b2","unresolved":true,"context_lines":[{"line_number":92,"context_line":"    result \u003d None"},{"line_number":93,"context_line":"    for service in services:"},{"line_number":94,"context_line":"        if name_type_or_id \u003d\u003d service.type:"},{"line_number":95,"context_line":"            result \u003d service"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"    if result is None:"},{"line_number":98,"context_line":"        msg \u003d _(\"No service with a type, name or ID of \u0027%s\u0027 exists.\")"}],"source_content_type":"text/x-python","patch_set":23,"id":"7ade89c6_54a8fca5","line":95,"updated":"2024-07-09 09:54:50.000000000","message":"You need an additional check here to ensure that `result` is not already set. If it is, that means there\u0027s a duplicate match and you need to raise an exception.","commit_id":"cd8d5fc9a6a2aa0314abf8b232480a41c35f299c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"892232b9681bbd69fb5551a8dbe7697a164e05b5","unresolved":false,"context_lines":[{"line_number":92,"context_line":"    result \u003d None"},{"line_number":93,"context_line":"    for service in services:"},{"line_number":94,"context_line":"        if name_type_or_id \u003d\u003d service.type:"},{"line_number":95,"context_line":"            result \u003d service"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"    if result is None:"},{"line_number":98,"context_line":"        msg \u003d _(\"No service with a type, name or ID of \u0027%s\u0027 exists.\")"}],"source_content_type":"text/x-python","patch_set":23,"id":"02ddcd7c_fdb959f3","line":95,"in_reply_to":"7ade89c6_54a8fca5","updated":"2024-07-09 15:02:36.000000000","message":"Acknowledged","commit_id":"cd8d5fc9a6a2aa0314abf8b232480a41c35f299c"}],"openstackclient/identity/v3/service.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"265df85f89bab134f4b91c21976c0acde2cf5609","unresolved":true,"context_lines":[{"line_number":41,"context_line":"        \u0027Type\u0027,"},{"line_number":42,"context_line":"        \u0027Enabled\u0027,"},{"line_number":43,"context_line":"        \u0027Description\u0027,"},{"line_number":44,"context_line":"    )"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    return ("},{"line_number":47,"context_line":"        column_headers,"}],"source_content_type":"text/x-python","patch_set":2,"id":"6414db0c_ccde74a9","line":44,"updated":"2024-01-24 17:07:14.000000000","message":"The functional tests failures are happening because we\u0027re expecting fields to be in lower case like they were previously. We can either edit the tests or make all these lower case again, whatever you prefer.","commit_id":"375f913b57baa25a2dfefc4996dc685789588575"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"0ed5c2aed1575cdf09efb22fc7cdb9b0edd712d2","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        \u0027Type\u0027,"},{"line_number":42,"context_line":"        \u0027Enabled\u0027,"},{"line_number":43,"context_line":"        \u0027Description\u0027,"},{"line_number":44,"context_line":"    )"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    return ("},{"line_number":47,"context_line":"        column_headers,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f8c09b7_df7bacec","line":44,"in_reply_to":"6414db0c_ccde74a9","updated":"2024-01-25 17:39:14.000000000","message":"Done","commit_id":"375f913b57baa25a2dfefc4996dc685789588575"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":216,"context_line":"    def take_action(self, parsed_args):"},{"line_number":217,"context_line":"        identity_client \u003d self.app.client_manager.identity"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"        service \u003d common.find_service(identity_client, parsed_args.service)"},{"line_number":220,"context_line":""},{"line_number":221,"context_line":"        service._info.pop(\u0027links\u0027)"},{"line_number":222,"context_line":"        return zip(*sorted(service._info.items()))"}],"source_content_type":"text/x-python","patch_set":11,"id":"0a9d9037_1088e769","side":"PARENT","line":219,"range":{"start_line":219,"start_character":25,"end_line":219,"end_character":37},"updated":"2024-05-13 10:58:06.000000000","message":"Rather than replacing this call, how about migrating the function to support SDK client? You might need to temporarily duplicate the method (e.g. `find_service_sdk`) until all the other callers are migrated but at least we keep things centralised that way?","commit_id":"5e5b89f906f864f50c5bdc1854d120fde3996c87"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":216,"context_line":"    def take_action(self, parsed_args):"},{"line_number":217,"context_line":"        identity_client \u003d self.app.client_manager.identity"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"        service \u003d common.find_service(identity_client, parsed_args.service)"},{"line_number":220,"context_line":""},{"line_number":221,"context_line":"        service._info.pop(\u0027links\u0027)"},{"line_number":222,"context_line":"        return zip(*sorted(service._info.items()))"}],"source_content_type":"text/x-python","patch_set":11,"id":"04da6607_ba0bd759","side":"PARENT","line":219,"range":{"start_line":219,"start_character":25,"end_line":219,"end_character":37},"in_reply_to":"0a9d9037_1088e769","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"5e5b89f906f864f50c5bdc1854d120fde3996c87"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":124,"context_line":"        result \u003d 0"},{"line_number":125,"context_line":"        for i in parsed_args.service:"},{"line_number":126,"context_line":"            try:"},{"line_number":127,"context_line":"                service \u003d identity_client.find_service(i)"},{"line_number":128,"context_line":"                identity_client.delete_service(service.id)"},{"line_number":129,"context_line":"            except Exception as e:"},{"line_number":130,"context_line":"                result +\u003d 1"}],"source_content_type":"text/x-python","patch_set":11,"id":"a2d2125f_e8f7fd98","line":127,"updated":"2024-05-13 10:58:06.000000000","message":"The semantics of this have changed now. Remember how SDK\u0027s `find_xxx` methods allow you to do lookup by name or ID? Well the help string for the `\u003cservice\u003e` option says that you can lookup by name, ID **or** type. Perhaps you want to keep the call to `common.find_service` and simply migrate that instead? More notes on this below.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":124,"context_line":"        result \u003d 0"},{"line_number":125,"context_line":"        for i in parsed_args.service:"},{"line_number":126,"context_line":"            try:"},{"line_number":127,"context_line":"                service \u003d identity_client.find_service(i)"},{"line_number":128,"context_line":"                identity_client.delete_service(service.id)"},{"line_number":129,"context_line":"            except Exception as e:"},{"line_number":130,"context_line":"                result +\u003d 1"}],"source_content_type":"text/x-python","patch_set":11,"id":"e723a4c7_b571de3f","line":127,"in_reply_to":"a2d2125f_e8f7fd98","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":220,"context_line":"    def take_action(self, parsed_args):"},{"line_number":221,"context_line":"        identity_client \u003d self.app.client_manager.sdk_connection.identity"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"        service \u003d identity_client.find_service(parsed_args.service)"},{"line_number":224,"context_line":"        kwargs \u003d {}"},{"line_number":225,"context_line":"        if parsed_args.type:"},{"line_number":226,"context_line":"            kwargs[\u0027type\u0027] \u003d parsed_args.type"}],"source_content_type":"text/x-python","patch_set":11,"id":"d5dcd7a1_4e887dd8","line":223,"updated":"2024-05-13 10:58:06.000000000","message":"Ditto. We\u0027ve lost the ability to set properties on a service that is retrieved by type.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":220,"context_line":"    def take_action(self, parsed_args):"},{"line_number":221,"context_line":"        identity_client \u003d self.app.client_manager.sdk_connection.identity"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"        service \u003d identity_client.find_service(parsed_args.service)"},{"line_number":224,"context_line":"        kwargs \u003d {}"},{"line_number":225,"context_line":"        if parsed_args.type:"},{"line_number":226,"context_line":"            kwargs[\u0027type\u0027] \u003d parsed_args.type"}],"source_content_type":"text/x-python","patch_set":11,"id":"ea3d3e24_74c3d5bd","line":223,"in_reply_to":"d5dcd7a1_4e887dd8","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d3f4b5b6f3661e485721504f8ce17e35190fe152","unresolved":false,"context_lines":[{"line_number":162,"context_line":"            column_headers \u003d (\u0027ID\u0027, \u0027Name\u0027, \u0027Type\u0027, \u0027Description\u0027, \u0027Enabled\u0027)"},{"line_number":163,"context_line":"        else:"},{"line_number":164,"context_line":"            columns \u003d (\u0027id\u0027, \u0027name\u0027, \u0027type\u0027)"},{"line_number":165,"context_line":"            column_headers \u003d (\u0027ID\u0027, \u0027Name\u0027, \u0027Type\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"        data \u003d identity_client.services()"},{"line_number":168,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"aa91549d_ed256df2","line":165,"updated":"2024-07-03 20:52:55.000000000","message":"nit: as a follow-up, you could do:\n\n```\ncolumns \u003d (\u0027id\u0027, \u0027name\u0027, \u0027type\u0027)\ncolumn_headers \u003d (\u0027ID\u0027, \u0027Name\u0027, \u0027Type\u0027)\nif parsed_args.long:\n    columns +\u003d (\u0027description\u0027, \u0027is_enabled\u0027)\n    column_headers +\u003d (\u0027Description\u0027, \u0027Enabled\u0027)\n```\n\nbut this is just a nit and if done, should be done separately IMO","commit_id":"72aec3faf93e6ae9512304f3d02ecba469120de2"}],"openstackclient/tests/unit/identity/v3/test_service.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":27,"context_line":"class TestService(identity_fakes.TestIdentityv3):"},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super().setUp()"},{"line_number":30,"context_line":"        super().setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"        self.app.client_manager.sdk_connection.identity \u003d mock.Mock("},{"line_number":33,"context_line":"            spec\u003d_proxy.Proxy,"}],"source_content_type":"text/x-python","patch_set":11,"id":"e40a71cf_a076aa82","line":30,"updated":"2024-05-13 10:58:06.000000000","message":"You need to remove this duplicated lines. Below also.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":27,"context_line":"class TestService(identity_fakes.TestIdentityv3):"},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super().setUp()"},{"line_number":30,"context_line":"        super().setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"        self.app.client_manager.sdk_connection.identity \u003d mock.Mock("},{"line_number":33,"context_line":"            spec\u003d_proxy.Proxy,"}],"source_content_type":"text/x-python","patch_set":11,"id":"963671e9_cb600094","line":30,"in_reply_to":"e40a71cf_a076aa82","updated":"2024-05-13 18:32:33.000000000","message":"My mistake. My branches diverged when I was working on the show service command/leaving everything else the same, and I must have messed up the merge conflict here.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":29,"context_line":"        super().setUp()"},{"line_number":30,"context_line":"        super().setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"        self.app.client_manager.sdk_connection.identity \u003d mock.Mock("},{"line_number":33,"context_line":"            spec\u003d_proxy.Proxy,"},{"line_number":34,"context_line":"        )"},{"line_number":35,"context_line":"        self.identity_sdk_client \u003d ("},{"line_number":36,"context_line":"            self.app.client_manager.sdk_connection.identity"},{"line_number":37,"context_line":"        )"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class TestServiceCreate(TestService):"}],"source_content_type":"text/x-python","patch_set":11,"id":"6d20bc55_c01756e8","line":37,"range":{"start_line":32,"start_character":0,"end_line":37,"end_character":9},"updated":"2024-05-13 10:58:06.000000000","message":"Double check, but I think this is already defined in the `TestIdentityv3` base class (or rather, in a `FakeClientMixin` it uses) and shouldn\u0027t be necessary here? If it isn\u0027t, you can actually get rid of this whole `TestService` base class and use `identity_fakes.TestIdentityv3` directly as the base class below. You\u0027re not using `find_service` in the SDK implementation so you don\u0027t want to mock that. You want to mock the thing you **are** using, `services`.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cc601199a20b91d35dabe224635259cd0cc3146f","unresolved":false,"context_lines":[{"line_number":29,"context_line":"        super().setUp()"},{"line_number":30,"context_line":"        super().setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"        self.app.client_manager.sdk_connection.identity \u003d mock.Mock("},{"line_number":33,"context_line":"            spec\u003d_proxy.Proxy,"},{"line_number":34,"context_line":"        )"},{"line_number":35,"context_line":"        self.identity_sdk_client \u003d ("},{"line_number":36,"context_line":"            self.app.client_manager.sdk_connection.identity"},{"line_number":37,"context_line":"        )"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class TestServiceCreate(TestService):"}],"source_content_type":"text/x-python","patch_set":11,"id":"e06b9cd5_97f17141","line":37,"range":{"start_line":32,"start_character":0,"end_line":37,"end_character":9},"in_reply_to":"4f48696d_53ae7769","updated":"2024-05-16 09:10:12.000000000","message":"No, I mean anywhere that inherits from `TestService` can now inherit from `identity_fakes.TestIdentityv3`. You can still use `self.identity_sdk_client` since it\u0027s defined in the base class (`TestIdentityv3`)","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":true,"context_lines":[{"line_number":29,"context_line":"        super().setUp()"},{"line_number":30,"context_line":"        super().setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"        self.app.client_manager.sdk_connection.identity \u003d mock.Mock("},{"line_number":33,"context_line":"            spec\u003d_proxy.Proxy,"},{"line_number":34,"context_line":"        )"},{"line_number":35,"context_line":"        self.identity_sdk_client \u003d ("},{"line_number":36,"context_line":"            self.app.client_manager.sdk_connection.identity"},{"line_number":37,"context_line":"        )"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class TestServiceCreate(TestService):"}],"source_content_type":"text/x-python","patch_set":11,"id":"4f48696d_53ae7769","line":37,"range":{"start_line":32,"start_character":0,"end_line":37,"end_character":9},"in_reply_to":"6d20bc55_c01756e8","updated":"2024-05-13 18:32:33.000000000","message":"You\u0027re right, it is. Does that mean anywhere I called `self.identity_sdk_client` I replace it with `identity_fakes.TestIdentityv3`?","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def setUp(self):"},{"line_number":185,"context_line":"        super().setUp()"},{"line_number":186,"context_line":"        super().setUp()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":189,"context_line":"            identity_exc.NotFound(None)"}],"source_content_type":"text/x-python","patch_set":11,"id":"c2693675_aa17b26d","line":186,"updated":"2024-05-13 10:58:06.000000000","message":"Ditto","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def setUp(self):"},{"line_number":185,"context_line":"        super().setUp()"},{"line_number":186,"context_line":"        super().setUp()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":189,"context_line":"            identity_exc.NotFound(None)"}],"source_content_type":"text/x-python","patch_set":11,"id":"62a6a3f6_0f224a87","line":186,"in_reply_to":"c2693675_aa17b26d","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":186,"context_line":"        super().setUp()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":189,"context_line":"            identity_exc.NotFound(None)"},{"line_number":190,"context_line":"        )"},{"line_number":191,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":192,"context_line":"        self.identity_sdk_client.delete_service.return_value \u003d None"}],"source_content_type":"text/x-python","patch_set":11,"id":"07c8df20_dd27f5bb","line":189,"updated":"2024-05-13 10:58:06.000000000","message":"This is an exception from python-keystoneclient. An openstacksdk-based call should return an openstacksdk-derived exception. What exception does `get_service` return if there\u0027s no match (hint: you can test this in the REPL or a throwaway script)\n\nAs a general point though, do you need this mock? Where is `get_service` getting called from the `DeleteService.take_action` method now?","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        super().setUp()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":189,"context_line":"            identity_exc.NotFound(None)"},{"line_number":190,"context_line":"        )"},{"line_number":191,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":192,"context_line":"        self.identity_sdk_client.delete_service.return_value \u003d None"}],"source_content_type":"text/x-python","patch_set":11,"id":"87ff3c8f_5b15cbdb","line":189,"in_reply_to":"07c8df20_dd27f5bb","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":284,"context_line":"        super().setUp()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":287,"context_line":"            identity_exc.NotFound(None)"},{"line_number":288,"context_line":"        )"},{"line_number":289,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":290,"context_line":"        self.identity_sdk_client.update_service.return_value \u003d self.service"}],"source_content_type":"text/x-python","patch_set":11,"id":"0b9acef5_59f93480","line":287,"updated":"2024-05-13 10:58:06.000000000","message":"Same point about exception types as previously, as well as about who the caller of this method is.","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        super().setUp()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":287,"context_line":"            identity_exc.NotFound(None)"},{"line_number":288,"context_line":"        )"},{"line_number":289,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":290,"context_line":"        self.identity_sdk_client.update_service.return_value \u003d self.service"}],"source_content_type":"text/x-python","patch_set":11,"id":"347d6ed1_b3919dc7","line":287,"in_reply_to":"0b9acef5_59f93480","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":448,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":449,"context_line":"            identity_exc.NotFound(None)"},{"line_number":450,"context_line":"        )"},{"line_number":451,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":452,"context_line":""},{"line_number":453,"context_line":"        # Get the command object to test"},{"line_number":454,"context_line":"        self.cmd \u003d service.ShowService(self.app, None)"}],"source_content_type":"text/x-python","patch_set":11,"id":"3ba6c25e_c77ec754","line":451,"updated":"2024-05-13 10:58:06.000000000","message":"From a quick look, `find_xxx` in python-keystoneclient has different semantics from `find_xxx` in openstacksdk. The latter effectively means \"try to retrieve by ID, but if that fails then list all resources, filter by name and return exactly one match\", while the former means \"list all resources, filter by a given field and return exactly one match\". Those are different things. This is worth noting if you end up migrating `openstackclient.identity.common.find_service`...","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":448,"context_line":"        self.identity_sdk_client.get_service.side_effect \u003d ("},{"line_number":449,"context_line":"            identity_exc.NotFound(None)"},{"line_number":450,"context_line":"        )"},{"line_number":451,"context_line":"        self.identity_sdk_client.find_service.return_value \u003d self.service"},{"line_number":452,"context_line":""},{"line_number":453,"context_line":"        # Get the command object to test"},{"line_number":454,"context_line":"        self.cmd \u003d service.ShowService(self.app, None)"}],"source_content_type":"text/x-python","patch_set":11,"id":"9025fcee_551d6edf","line":451,"in_reply_to":"3ba6c25e_c77ec754","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"764773b2b966690747a1c8b09d1438494e28a0ed","unresolved":true,"context_lines":[{"line_number":501,"context_line":"            self.assertEqual("},{"line_number":502,"context_line":"                \"ClientException\","},{"line_number":503,"context_line":"                str(e),"},{"line_number":504,"context_line":"            )"}],"source_content_type":"text/x-python","patch_set":11,"id":"f1cd619d_429a654d","line":504,"updated":"2024-05-13 10:58:06.000000000","message":"This is somewhat unrelated, but this is a weird way to do this check. You should just use `assertRaises`","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"},{"author":{"_account_id":35548,"name":"Antonia Gaete","email":"antoniagaete@osuosl.org","username":"antoniagaete"},"change_message_id":"f589b61efc5cbc4236f3ca6e731fb53992152b16","unresolved":false,"context_lines":[{"line_number":501,"context_line":"            self.assertEqual("},{"line_number":502,"context_line":"                \"ClientException\","},{"line_number":503,"context_line":"                str(e),"},{"line_number":504,"context_line":"            )"}],"source_content_type":"text/x-python","patch_set":11,"id":"d06814ae_f6532d24","line":504,"in_reply_to":"f1cd619d_429a654d","updated":"2024-05-13 18:32:33.000000000","message":"Acknowledged","commit_id":"c0868d04ba60c184658034354e0eaf452c06ca5c"}]}