)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"3211b95ce8186d892896134f23d407d48bd72c6d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"6e5b6066_f6c8d1c5","updated":"2022-05-25 08:03:47.000000000","message":"from a quick glance, the current code tests some capability operation, but the refector removes that and adds new tests. Can you please keep the refactoring to just changing the structure and add new tests in a separate patch?\n\nHave you also checked if the  https://review.opendev.org/c/openstack/cinder-tempest-plugin/+/778357/ can be tuned a bit instead?","commit_id":"c7a8ec99a0e01a400e8247050ba61cd658cb611c"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"806c1300afa98f6e6006f1eb00511ca586c9a8a1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e6138ef5_d5fe9d19","in_reply_to":"6e5b6066_f6c8d1c5","updated":"2022-05-25 08:08:05.000000000","message":"Sure ok , let me refactor existing tests. good idea !","commit_id":"c7a8ec99a0e01a400e8247050ba61cd658cb611c"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"6285b7f255eacedd92b1465fc4144dd6be46acae","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"ad9136ed_a2beaa26","updated":"2022-06-29 07:04:15.000000000","message":"Luigi , \nPlease note that there is no real change in the way it should work as long we pass to the base functions the clients (initialized based on manager for example:\nif we set the credentionals like :\ncredentials \u003d [\u0027system_reader\u0027, \u0027system_admin\u0027]\nWe should expect to get managers in the class :\ncls.os_system_reader\ncls.os_system_admin\n\nUnder each manager we have all clients implemented based on authentication\nSo if we like to access to volume_clients we should call to \ncls.os_system_admin.volumes_client ... \n\nto make it short , allocate a manager for above based on credentials...  \nmanager \u003d cls.client_manager(credentials\u003dcreds.credentials)\n\n\nSo \nfrom tempest point of view we dont care if its os_admin or os_system_admin , they both managers and have access to all openstack clients. the difference is the permissions.\n\nNow for the way its implemented:\nself.do_request(..)  which is a kind of callaback to run commands based on clients\nthe idea here is that we can call to any method based on the client... \nclients can be:\ncls.persona \u003d getattr(cls, \u0027os_%s\u0027 % cls.credentials[0])\ncls.client \u003d cls.persona.users_v3_client\n\nat this point we have the client based on the persona (its only the needed  manager like cls.os_system_admin or others )and the client it self which is volume_clients in out case or others.\ni guess that RBAC should be used in scenarios as well and combinations in persona , like create volume with persona X  and person Z try to read or modify it.\n\nif we agree till now , we can move on and continue to implement it .\ni am wrong here , please raise a flag.\n\nBenny","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"daf67fdf695355f1f890cfec185526af8de434cb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a8321054_fbc10290","updated":"2022-07-14 13:16:15.000000000","message":"lgtm if it works, plugin maintainers have more insight into this than me","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"efb47d0054e4730a52d173a68b1271ccdc3929d5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"f5f36a81_0d0c37ff","updated":"2022-06-28 23:47:55.000000000","message":"recheck","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"71d047e25a4f1c6387da65e624f3c0d33df00e28","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"7d8aeec6_c00b2073","updated":"2022-06-28 12:25:38.000000000","message":"recheck\n\nunrelated test_volume_backup_restore timeout in lvm-lio-barbican job (usual resource issue?)","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"855da30ee28eb4429917052f5f9a0f333cd28932","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":6,"id":"62f1411a_4706ee17","in_reply_to":"ad9136ed_a2beaa26","updated":"2022-06-29 07:19:27.000000000","message":"And for the inheritence i implemented , which we can discuss about.\nthe idea here is a parent class initialized all supported credentials.\nclass VolumeV3RbacTests(VolumeV3RbacBaseTests):\n\n    credentials \u003d [\u0027project_admin\u0027, \u0027project_member\u0027, \u0027project_reader\u0027,\n                   \u0027system_admin\u0027, \u0027primary\u0027, \u0027admin\u0027]\n\n\neach subclass inherits from it with the related persona and its done for any additional code that will be needed under persona ... \nexample: class ProjectAdminBaseTests(VolumeV3RbacTests)\n\nHere we initialzed the needed persona which the tests inherits...and the code is open for extentions , adding new method or skip somehting for this persona ..\n\nand the tests can be re-used with different persona.\nassume you have a testcase which need persona os_system and os_domain, they alreay initialized because of the parent class.\n\nWe can make it more flat but its going to be painfull if the code need to be extended or changes needed.\n\nI am expecting from more people to comment 😊\n\nWhat do you think ?","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"}],"cinder_tempest_plugin/rbac/v3/base.py":[{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"3211b95ce8186d892896134f23d407d48bd72c6d","unresolved":true,"context_lines":[{"line_number":1,"context_line":"# Copyright 2022 Red Hat, Inc."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":3,"id":"d08bb248_a89a61cf","line":1,"updated":"2022-05-25 08:03:47.000000000","message":"not needed","commit_id":"c7a8ec99a0e01a400e8247050ba61cd658cb611c"},{"author":{"_account_id":22873,"name":"Martin Kopec","email":"mkopec@redhat.com","username":"mkopec"},"change_message_id":"daf67fdf695355f1f890cfec185526af8de434cb","unresolved":true,"context_lines":[{"line_number":24,"context_line":"CONF \u003d config.CONF"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"class VolumeV3RbacBaseTests(base.BaseVolumeAdminTest):"},{"line_number":28,"context_line":"    \"\"\"V3Rbac base inherits from baseVolumeAdmin BaseVolumeTest"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    This class inherit from baseVolumes and call do_request per client"}],"source_content_type":"text/x-python","patch_set":6,"id":"15c75163_a0d2742a","line":27,"range":{"start_line":27,"start_character":33,"end_line":27,"end_character":52},"updated":"2022-07-14 13:16:15.000000000","message":"shouldn\u0027t this be BaseVolumeTest? BaseVolumeAdminTest is a test class for all Volume Admin API tests (based on the docstring of the class) and VolumeV3RbacBaseTests doesn\u0027t contain any admin tests -\u003e ProjectAdminBaseTests could maybe inherit from BaseVolumeAdminTest","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"3a6b6c99b5b2f2728192b30ee4a04ada3f89cf35","unresolved":true,"context_lines":[{"line_number":24,"context_line":"CONF \u003d config.CONF"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"class VolumeV3RbacBaseTests(base.BaseVolumeAdminTest):"},{"line_number":28,"context_line":"    \"\"\"V3Rbac base inherits from baseVolumeAdmin BaseVolumeTest"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    This class inherit from baseVolumes and call do_request per client"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a9c6bd3_12ed78cc","line":27,"range":{"start_line":27,"start_character":33,"end_line":27,"end_character":52},"in_reply_to":"15c75163_a0d2742a","updated":"2022-07-14 13:45:02.000000000","message":"In line #63 we define which client to initialized \ncredentials \u003d [\u0027project_admin\u0027, \u0027project_member\u0027, \u0027project_reader\u0027,\n                   \u0027system_admin\u0027, \u0027primary\u0027, \u0027admin\u0027]\n\nThe role of this class is to expose do_request for all RBAC tests classes which initialized based on line 63.\ncls.os_project_admin , cls.os_project_member, cls.os_project_reader ...\n\nand to continue support exisiting tempest code (base functions with cleanup) , once we expose in all base function the client as param we can re-use the code with any persona we like base on permission.\n\nfor example : cls.os_project_admin.snapshots_client_latest or \ncls.os_project_member.snapshots_client_latest  if its doable... \n\n\ncreate a volume (in tempest base code) and be called and we pass the persona we want (client -\u003e \nThere is no need to re-create all bases or call to code from lib/services/volumes when we have wrappers.\n\n\nHope its clear of course if you disagree we can change the structure.","commit_id":"a05555d1915012596a70f5078f514e7012f0700e"}],"cinder_tempest_plugin/rbac/v3/test_volumes_capabilities.py":[{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"363ebd198390dd7e4cef7d333a761d4e3dfad7f4","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"34d28017_6a28010f","line":42,"updated":"2022-06-08 12:32:04.000000000","message":"I\u0027d move test_backend_capabilities in a separate (mixin?) class in this same file.\n\nThe reason is that VolumesCapabilitiesMember and VolumesCapabilitiesReader from a logical point of view are sibling of VolumesCapabilitiesAdmin: they test the same type of object.","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"85e47e4536e6f4537aab9fc8f1de902559136dc1","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"d12d7529_fc28abe0","line":42,"in_reply_to":"1752d2cf_e9887a00","updated":"2022-06-08 13:36:55.000000000","message":"ack , thanks . understrand the meaning . checking\n\nSee how it structured in keystone .\n\nhttps://github.com/openstack/keystone-tempest-plugin/blob/4eff632695fe79a5d78f400dca3ceab663c83788/keystone_tempest_plugin/tests/rbac/v3/test_access_rule.py#L145\n\n\nhttps://github.com/openstack/keystone-tempest-plugin/blob/master/keystone_tempest_plugin/tests/rbac/v3/test_grant.py#L873","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"4e0d05d8b9635c1771faaa7a516bb9aee1f9ffe1","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"a9a5c053_0c101392","line":42,"in_reply_to":"34d28017_6a28010f","updated":"2022-06-08 12:56:49.000000000","message":"Hi ,\nThe main part of each class is the credential, temespt allocates the clients objects for all initialized with the right permissions.\ncls.persona \u003d getattr(cls, \u0027os_%s\u0027 % cls.credential)\n\nSo on VolumesCapabilitiesMember we will create os_project_member with all clients\nWe re-use the same testcase on child but with different persona\n\nit comes that we have multiple os_XXXXX with clients per authentication info.\non each one we use the relevant client re-using the test and send the client to the base code (re-use code for tempest) which inlcludes cleanup and waiters.\n\nThe class tests are running the same testcases with different persona\n\n\nCould you please explain what to do or whats wrong ?","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"c9eb412e99ca75419ed9b627b4746e37f3cdf51d","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"4d4c8ac0_cb17b046","line":42,"in_reply_to":"5fe50466_01b7fc2c","updated":"2022-06-28 09:16:08.000000000","message":"It\u0027s the inheritance: the classes should not really linked that way IMHO.\n\nBut  as I said, it seems no ones care, so please just rebase this change.","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":11075,"name":"Benny Kopilov","email":"bkopilov@redhat.com","username":"bkopilov"},"change_message_id":"0fd99919fc626a70b6066cf2d12d2cf472e2ff63","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"5fe50466_01b7fc2c","line":42,"in_reply_to":"98cddfbe_203836f5","updated":"2022-06-27 15:24:53.000000000","message":"Ack, What exactly you dont like ? i am trying to re-use all base.py code by passing the client instead of duplication.\n\nHow do you recommend to do it ?\n\nBenny","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"21532746719236237e4f0748a401d2199e6aab5b","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"1752d2cf_e9887a00","line":42,"in_reply_to":"a9a5c053_0c101392","updated":"2022-06-08 13:04:04.000000000","message":"I don\u0027t find it consistent for the VolumesCapabilitiesMember and VolumesCapabilitiesReader  to inherit from VolumesCapabilitiesAdmin. They should inherit from a common class. Testing \u0027reader\u0027 is not a specialization of \u0027admin\u0027, they are both a special case of \u0027run a test on a persona\u0027","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"},{"author":{"_account_id":10459,"name":"Luigi Toscano","email":"ltoscano@redhat.com","username":"ltoscano"},"change_message_id":"ddb31b76d7a7f2a365137164376db8488ee2352d","unresolved":true,"context_lines":[{"line_number":39,"context_line":"            \u00271d1ab281-18f3-43f8-bbe7-3d9a9646e5bc\u0027)("},{"line_number":40,"context_line":"            cls.test_backend_capabilities)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def test_backend_capabilities(self):"},{"line_number":43,"context_line":"        pools \u003d self.admin_stats_client.list_pools()[\u0027pools\u0027]"},{"line_number":44,"context_line":"        host_name \u003d pools[0][\u0027name\u0027]"},{"line_number":45,"context_line":"        self.do_request("}],"source_content_type":"text/x-python","patch_set":5,"id":"98cddfbe_203836f5","line":42,"in_reply_to":"d12d7529_fc28abe0","updated":"2022-06-27 15:14:06.000000000","message":"It seems everyone did it wrong. I don\u0027t like that solution, it doesn\u0027t look very logical to me.\n\nThat said, it seems we have bigger fishes to fry, please rebase this change as it is not mergeable anymore.","commit_id":"c8939d2304edb932f0d85fb58716f9abfffbba4f"}]}