| *** lbragstad_ is now known as lbragstad | 13:15 | |
| abhishekk | croelandt, around ? | 13:34 |
|---|---|---|
| *** pdeore_ is now known as pdeore | 13:37 | |
| pdeore | Hi | 13:38 |
| pdeore | I've updated all the tempest protection test patches for metadef secure RBAC, | 13:38 |
| pdeore | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902/25 | 13:38 |
| pdeore | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802793/13 | 13:38 |
| pdeore | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802792/11 | 13:38 |
| pdeore | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802794/6 | 13:38 |
| pdeore | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802795/4 | 13:38 |
| croelandt | abhishekk: in a meeting, but shoot :) | 13:39 |
| pdeore | dansmith, lbragstad , croelandt , abhishekk , could you please have a look on the same when get some time.. | 13:39 |
| croelandt | sure | 13:40 |
| abhishekk | croelandt, no hurry, lets talk after your meeting :D | 13:40 |
| lbragstad | pdeore cool - thanks | 13:40 |
| abhishekk | pdeore, ack | 13:40 |
| *** whoami-rajat__ is now known as whoami-rajat | 14:00 | |
| dansmith | pdeore: don't hate me, and I'll be glad to settle my argument with lbragstad and then make some of those changes myself :) | 14:02 |
| lbragstad | ok - following back up on the subclass thing | 14:13 |
| lbragstad | yeah - i'm fine putting it in MetadefV2RbacNamespaceTest | 14:14 |
| lbragstad | fwiw - i used the original architecture from the keystone tests - which were implemented here https://review.opendev.org/c/openstack/keystone-tempest-plugin/+/686305 | 14:15 |
| dansmith | and why does that not just inherit from the base class itself? | 14:15 |
| lbragstad | it could | 14:16 |
| dansmith | personally, IMHO mixin classes should only be used if they're going to be mixed into multiple hierarchies.. like two separate classes both need some extra functionality but they're otherwise unrelated | 14:16 |
| lbragstad | i'm not entirely sure why it was isolated out in the keystone tests | 14:17 |
| lbragstad | yeah - agreed, since all this is related to the metadef stuff, it could live in a single hierarchy | 14:17 |
| dansmith | but if you're just separating out things that all have to be squashed into each other to be useful, it makes no sense to me, | 14:17 |
| dansmith | and just makes it hard to figure out where this thing is defined that you're using | 14:17 |
| abhishekk | just to note, we need create_namespaces method in all the module | 14:18 |
| abhishekk | modules | 14:18 |
| lbragstad | so - rbac.v2.base.RbacBaseTests is going to inherit from tempest.test.BaseTestCase? | 14:20 |
| lbragstad | and then test_namespaces.ProjectAdminTest would just inherit from RbacBaseTests (for example)? | 14:20 |
| dansmith | your abc can still be in there, | 14:21 |
| lbragstad | yeah - that makes sense | 14:21 |
| dansmith | so I would do BaseTestCase <- RbacBaseTests <- MetadefV2RbacNamespaceTest <- {admin, member, reader} | 14:21 |
| lbragstad | yeah - ok | 14:21 |
| lbragstad | that makes sense | 14:21 |
| dansmith | pdeore: I'm happy to make that change for you if you like | 14:22 |
| lbragstad | and RbacBaseTests should contain all the logic implemented in https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902/25/glance_tempest_plugin/tests/rbac/v2/base.py still | 14:22 |
| lbragstad | (e.g., do_request(), skip_checks(), etc...) | 14:22 |
| dansmith | yup | 14:22 |
| lbragstad | cool | 14:22 |
| dansmith | and also create_namespaces() probably right? | 14:23 |
| dansmith | because that's used elsewhere | 14:23 |
| dansmith | up the stack I mean | 14:23 |
| abhishekk | ++ | 14:23 |
| lbragstad | you mean something like MetadefV2RbacObjectTests? | 14:23 |
| lbragstad | yeah - i think so | 14:23 |
| dansmith | oh, I see, that's metadef specific but RbacBaseTests isn't | 14:24 |
| dansmith | so sure | 14:24 |
| lbragstad | if or when we ever get some sort of RbacBaseTest class in tempest proper, we would need a place for that stuff in glance, but planning for that now might be a pre-optimization | 14:25 |
| dansmith | well, it's metadef-specific, so fine to put it in a middle class | 14:26 |
| lbragstad | in MetadefV2RbacNamespaceTest? | 14:26 |
| dansmith | it's used by non-namepsace tests above, right? so needs to be somewhere generic | 14:26 |
| dansmith | or not even on an object itself, just a standalone utility method | 14:26 |
| lbragstad | well MetadefV2RbacObjectTest is going to need to inherit from MetadefV2RbacNamespaceTest then, right? | 14:26 |
| dansmith | well, I guess it needs do_request | 14:26 |
| dansmith | lemme just work up a proposed change :) | 14:27 |
| lbragstad | ack | 14:27 |
| lbragstad | if it needs to live in RbacBaseTest, that's fine | 14:28 |
| * abhishekk will be back in couple of hours | 14:31 | |
| *** abhishekk is now known as abhishekk|away | 14:31 | |
| dansmith | ooh, look at that.. pdeore was just copying lbragstad's image tests where the project test inherits from the admin test :) | 14:35 |
| dansmith | lbragstad: not sure I've run this locally, or at least not in a long time.. do you have a local.conf I can copy? | 14:38 |
| lbragstad | yeah - i think so | 14:39 |
| lbragstad | dansmith https://termbin.com/exml | 14:41 |
| dansmith | oh, I guess GLANCE_ENFORCE_SCOPE is all I need, maybe | 14:43 |
| lbragstad | and enable_plugin glance https://opendev.org/openstack/glance | 14:44 |
| dansmith | er, how do I get it to use my local copy of the plugin.. yeah, that's going to give me the tempest plugin somehow? | 14:44 |
| dansmith | and then I can push my changes into there to test? | 14:45 |
| lbragstad | oh - nevermind | 14:45 |
| lbragstad | you probably don't need that then | 14:45 |
| lbragstad | if you just install your local copy into the tempest env | 14:45 |
| dansmith | okay I haven't done that, but I guess I'll stack first and figure out what that looks like.. haven't run a tempest plugin locally I guess | 14:46 |
| dansmith | oh, I guess I have to pip install it into the venv | 14:49 |
| lbragstad | yeah | 14:49 |
| lbragstad | so tempest can discover the tests | 14:49 |
| dansmith | I suppose I can setup.py develop into there to keep it live | 14:51 |
| lbragstad | yeah - i usually $ /opt/stack/tempest/.tox/tempest/bin/pip -e install ~/glance-tempest-plugin/ | 14:53 |
| dansmith | ack | 14:53 |
| dansmith | ah, I bet I know some of the reason for the mixins at least.. the abc stuff doesn't play nice | 15:11 |
| dansmith | because the test runner tries to instantiate those classes if they inherit from abc | 15:11 |
| dansmith | grr | 15:11 |
| dansmith | lbragstad: how married are you to that abc protocol? AFAICT, it's somewhat pointless in these tests, since we inherit from it directly only once, and then subclass the first subclass, so the abc protections don't even help us in the later classes | 15:15 |
| lbragstad | if everything were to import from the actual base class directly, then i'd advocate for keeping it, but since we re-use stuff across personas, then i'm not as concerned | 15:16 |
| dansmith | yeah, that's my point | 15:17 |
| dansmith | we could keep it and still mix it in at the lower levels like it should be | 15:17 |
| dansmith | pdeore_: around? | 15:45 |
| *** abhishekk|away is now known as abhishekk | 15:51 | |
| abhishekk | dansmith, I don't think she is around | 15:51 |
| dansmith | okay, just wondering if I can push up these changes to her set for comment | 15:52 |
| dansmith | but didn't want to do that without asking her | 15:52 |
| abhishekk | I think you can, I will let her know that you asked before pushing changes | 15:52 |
| dansmith | heh okay | 15:53 |
| abhishekk | :D | 15:53 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Add protection testing for metadef namespaces https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902 | 15:54 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Refactor RbacBasetest to inherit from tempest https://review.opendev.org/c/openstack/glance-tempest-plugin/+/808990 | 15:54 |
| dansmith | lbragstad: abhishekk ^ let me know what you think and I'll rebase the rest on that if you think it's an improvement | 15:54 |
| dansmith | keeps the abc for test enforcement, which I'm not like super excited about, but.. | 15:55 |
| abhishekk | dansmith, ack | 15:55 |
| lbragstad | ok | 15:56 |
| abhishekk | I like the base patch | 15:57 |
| dansmith | I probably need to update the commit message since it didn't turn out exactly like that, | 15:58 |
| dansmith | but I think it crystalizes the "abc class makes sure you have these tests and nothing else" pattern, | 15:59 |
| dansmith | and lets the other regular classes do all the implementation of the client setup things | 15:59 |
| dansmith | if we didn't use the ABC then we wouldn't need that and the mixins per-class either | 15:59 |
| dansmith | but I understand the goal of it | 16:00 |
| abhishekk | ++ | 16:00 |
| dansmith | but the second patch is where the pattern makes more of a difference, with only one mix-in instead of two | 16:00 |
| abhishekk | This looks much cleaner though | 16:02 |
| abhishekk | just waiting for test results before voting | 16:14 |
| dansmith | let's make sure lbragstad thinks it's an improvement and not just moving stuff around | 16:14 |
| lbragstad | cool - so all the utilities will live in ImageV2RbacImageTest and the tests abcs will live in ImageV2RbacTemplate? | 16:19 |
| abhishekk | jokke_, around ? | 16:21 |
| abhishekk | lbragstad, yes, | 16:22 |
| lbragstad | yeah - i don't have any objection to that, i think it makes sense and is linear | 16:22 |
| lbragstad | thanks dansmith | 16:22 |
| lbragstad | there's nothing about how keystone did it that obviously needs to be ported here | 16:23 |
| abhishekk | just wondering (may be to work in following cycle) can we modify do_request method (or clients) to work like our functional synchronize way ? | 16:23 |
| lbragstad | or, if it's common (which is is) we could move it to tempest proper | 16:24 |
| jokke_ | abhishekk: yeah | 16:28 |
| abhishekk | jokke_, there are some comments on release patch | 16:29 |
| abhishekk | I think just one related to minor version bump | 16:29 |
| abhishekk | RC1 path is clear, I will put up a releasenote patch tomorrow and once it is merged then I will modify the hash in already up RC1 release patch | 16:31 |
| jokke_ | abhishekk: are we ok with the reno commit messages stating patch version rather than minor? | 16:33 |
| jokke_ | I can very quickly spin that release patch around if we don't need to change the reno commits ... the git log is only place where they show up | 16:34 |
| abhishekk | jokke_, ohh | 16:35 |
| abhishekk | It will render in final reno with correct version, right ? | 16:36 |
| abhishekk | If so then it is ok | 16:37 |
| jokke_ | yeah, and it will look silly in the git log anyways to rever and repropose same patches as I don't think we can do --amends into published commits | 16:37 |
| abhishekk | hmm | 16:39 |
| abhishekk | maybe smcginnis could suggest something if possible | 16:39 |
| abhishekk | jokke_, I will be back in 10/15 minutes | 16:40 |
| opendevreview | Merged openstack/glance-tempest-plugin master: Refactor RbacBasetest to inherit from tempest https://review.opendev.org/c/openstack/glance-tempest-plugin/+/808990 | 17:21 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Add protection testing for metadef namespaces https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902 | 17:30 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Implement API protection testing for metadef objects https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802793 | 17:30 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Implement API protection testing for metadef resource types https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802792 | 17:30 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Implement API protection testing for metadef properties https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802794 | 17:30 |
| opendevreview | Dan Smith proposed openstack/glance-tempest-plugin master: Implement API protection testing for metadef tags https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802795 | 17:30 |
| dansmith | rebased the rest ^ | 17:30 |
| abhishekk | thanks o/ | 17:31 |
| * abhishekk signing out for the day | 18:20 | |
| opendevreview | Merged openstack/glance master: [uwsgi] Add missing pefetch periodic job https://review.opendev.org/c/openstack/glance/+/803937 | 18:39 |
| opendevreview | Merged openstack/glance master: Reproduce bug #1932337 https://review.opendev.org/c/openstack/glance/+/796885 | 19:01 |
| opendevreview | Merged openstack/glance master: Fix failed cinder store migration for non-owners https://review.opendev.org/c/openstack/glance/+/796887 | 19:01 |
| lbragstad | dansmith qq https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902/27/glance_tempest_plugin/tests/rbac/v2/base.py#84 | 20:26 |
| dansmith | lbragstad: cha? | 20:27 |
| lbragstad | do you know why we have setup_client() there and it can't all be in https://review.opendev.org/c/openstack/glance-tempest-plugin/+/800902/27/glance_tempest_plugin/tests/rbac/v2/metadefs/test_namespaces.py#32 ? | 20:27 |
| dansmith | because of the last two lines I think | 20:27 |
| dansmith | because each of these uses a different client class (namespaces, objects, etc) | 20:27 |
| lbragstad | 92 - 93? | 20:28 |
| dansmith | yeah | 20:28 |
| dansmith | ...right? | 20:29 |
| lbragstad | so - we want the admin metadef namespace client to be shared across the various metadef test classes | 20:30 |
| lbragstad | and that's why its in RbacBaseTests? | 20:30 |
| dansmith | oh, sorry, I had this backwards | 20:31 |
| dansmith | I thought you were asking about things like this: https://review.opendev.org/c/openstack/glance-tempest-plugin/+/802793/14/glance_tempest_plugin/tests/rbac/v2/metadefs/test_objects.py | 20:31 |
| dansmith | where we setup_clients again, and put in the objects client | 20:31 |
| lbragstad | yeah - line 39 - 40 setup the admin objects client | 20:31 |
| dansmith | for the namespaces one, we probably want it to be in the base because create_namespaces() is used by other things and, I think calls that no? | 20:32 |
| lbragstad | ok, that makes sense | 20:32 |
| lbragstad | that seems right, but i wasn't sure | 20:32 |
| lbragstad | cls.admin_client (naming convention-wise) could be reused across test classes | 20:33 |
| lbragstad | so i wanted to double check if it really was supposed to be pushed down to the metadef namespaces test class | 20:33 |
| dansmith | it really should be namespace_client or something I would t hink | 20:33 |
| lbragstad | yeah - agreed | 20:33 |
| dansmith | that confused me at one point when rebasing the tags patch at the top | 20:34 |
| dansmith | lbragstad: I didn't do anything to those other than fix the structure, | 20:34 |
| dansmith | and I think those kinds of arguments are all valid, so feel free to -1 on that | 20:35 |
| lbragstad | ok - i put my thoughts in comments instead | 20:40 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!