| *** akekane_ is now known as abhishekk | 05:51 | |
| opendevreview | Abhishek Kekane proposed openstack/glance-specs master: Spec Lite: Policy tests refactoring https://review.opendev.org/c/openstack/glance-specs/+/797593 | 07:33 |
|---|---|---|
| *** bhagyashris_ is now known as bhagyashris | 08:27 | |
| dansmith | redrobot: so I have a question, maybe you can consult | 13:22 |
| dansmith | lance left this comment while we were working on the first rbac things: https://github.com/openstack/glance/blob/master/glance/api/policy.py#L171-L182 | 13:22 |
| dansmith | and I understand it and what we did | 13:23 |
| dansmith | initially in my attempt to modernize this, I made get_images more like get_image, in that it tested project stuff as well as some image visibility things | 13:23 |
| dansmith | however, thinking about it more and then re-reading that, | 13:24 |
| dansmith | I'm thinking that list should just check get_image against each image instead of checking get_images against each, | 13:24 |
| dansmith | which would make get_image more of a "can you even list bro?" check, but which wouldn't really have any project checks at all (since what is the project_id of a list?) | 13:25 |
| abhishekk | dansmith, I put up a second spec (lite) for tests refactoring | 13:51 |
| dansmith | abhishekk: ack, I have it open | 13:52 |
| abhishekk | cool | 13:52 |
| dansmith | abhishekk: any comment on my question to redrobot above? | 13:53 |
| abhishekk | looking at the comment from lance | 13:54 |
| abhishekk | As per his comment it makes sense to enforce get_image policy rather than get_images | 13:55 |
| dansmith | that makes the most sense to me as well, but it's not in line with what he set get_images policy to (which might have just been to make it work at the time) and is definitely a departure from the way it has been | 13:56 |
| * abhishekk need to attend one meeting | 13:59 | |
| abhishekk | will be back in 30 mins | 13:59 |
| * abhishekk is back | 15:00 | |
| abhishekk | I think at that time it was just a hurry to make it work | 15:01 |
| dansmith | abhishekk: can you tell me why this is supposed to fail with forbidden? https://github.com/openstack/glance/blob/master/glance/tests/unit/v2/test_images_resource.py#L1256 | 15:07 |
| abhishekk | looking | 15:07 |
| dansmith | ah, is it because that image is owned by tenant3 and the request is tenant1? | 15:09 |
| dansmith | from the name I thought it had something to do with being queued, or the hidden flag itself | 15:09 |
| dansmith | maybe that test should be named "test_update_hidden_on_non_owned_image" ? | 15:10 |
| abhishekk | frankly speaking not able to recollect, but the test is itself wrong | 15:13 |
| dansmith | in what way? | 15:13 |
| abhishekk | at least it should have been named correctly, and I wasn't intended to check for forbidden | 15:13 |
| dansmith | okay | 15:14 |
| dansmith | it's failing now because that test class uses a fake enforcer that allows anything, | 15:15 |
| abhishekk | The test should have checked for queued image should not be allowed to be hidden | 15:15 |
| dansmith | so I'm not sure why it was getting forbidden before | 15:15 |
| dansmith | hmm, okay I think it's allowing that though | 15:15 |
| dansmith | yeah, the image comes back with os_hidden=true | 15:17 |
| abhishekk | is it? | 15:18 |
| dansmith | yeah just a sec | 15:18 |
| dansmith | https://termbin.com/1nyd | 15:18 |
| dansmith | testtools.matchers._impl.MismatchError: {} != 'true' | 15:18 |
| dansmith | what are you expecting there? Conflict or BadRequest ? | 15:19 |
| abhishekk | I need to go back and look at the spec | 15:20 |
| abhishekk | I do recollect that I decided to raise BadRequest for update on queued call but I guess that was rejected in the discussion | 15:21 |
| dansmith | Okay I don't see anywhere in If8f02ca94fdb8e1ac7a81853cd392988900172d1 that you check for and reject based on state | 15:21 |
| abhishekk | yeah, neither do I | 15:22 |
| abhishekk | may be the test is for trying to update another users image | 15:25 |
| abhishekk | because there is no mention of restricting update call in spec as well | 15:25 |
| dansmith | yeah, but I think something else must have been causing it to fail before, | 15:26 |
| dansmith | because the policy being passed there is wide open and should allow anything | 15:26 |
| dansmith | potentially the authorization layer that just makes everything immutable, which is now disabled | 15:26 |
| dansmith | yeah, I guess that's it.. | 15:27 |
| dansmith | so the test really should be using UUID1 (which is owned by TENANT1) and making sure that we get back a BadRequest instead of forbidden yeah? | 15:27 |
| dansmith | along with whatever enforcement is required for that | 15:30 |
| abhishekk | really not sure, I think file a bug for this and flag that test as irrelevant at the moment | 15:30 |
| dansmith | mmkay | 15:31 |
| dansmith | https://bugs.launchpad.net/glance/+bug/1933360 | 15:34 |
| abhishekk | perfect, thank you | 15:37 |
| * abhishekk dinner break | 15:56 | |
| dansmith | abhishekk: I'm hoping to push up another rev of the poc set today, but I'm just going to leave that hidden test failing in it | 16:34 |
| abhishekk | dansmith ack | 16:35 |
| abhishekk | dansmith, could you skip that test with reference to bug? | 16:36 |
| abhishekk | just return in the beginning with comment will be good as well | 16:37 |
| dansmith | I mean, I could, but probably better to just shove a patch underneath to fix/change/whatever it | 16:37 |
| dansmith | not sure I'll have everything else passing anyway, so... | 16:37 |
| dansmith | that's the only unit failure, but there might still be other tests that fail | 16:38 |
| dansmith | if I have time I'll put at least a WIP underneath this, I'm just trying to avoid too much going back and forth | 16:38 |
| abhishekk | ack | 16:38 |
| opendevreview | Dan Smith proposed openstack/glance master: Refactor gateway get_repo auth layer https://review.opendev.org/c/openstack/glance/+/789913 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: Make property protection tests use member role https://review.opendev.org/c/openstack/glance/+/789914 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: WIP: Make image update check policy at API layer https://review.opendev.org/c/openstack/glance/+/789915 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: WIP: Add a member field to Image when appropriate https://review.opendev.org/c/openstack/glance/+/796066 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: WIP: Check get_image(s) in the API https://review.opendev.org/c/openstack/glance/+/796067 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: Fix broken test_update_queued_image_with_hidden https://review.opendev.org/c/openstack/glance/+/797721 | 17:07 |
| opendevreview | Dan Smith proposed openstack/glance master: Move lazy store update to locations layer https://review.opendev.org/c/openstack/glance/+/797722 | 17:07 |
| abhishekk | will this last patch of yours conflict with the earlier bug fix? | 17:13 |
| dansmith | the get_image(s) one? | 17:19 |
| dansmith | oh the lazy store update? | 17:21 |
| dansmith | yes, it'll have to be changed once that merges | 17:21 |
| abhishekk | yes | 17:22 |
| abhishekk | ack | 17:22 |
| * abhishekk signing out for the day | 17:46 | |
| opendevreview | Merged openstack/glance_store master: vmware: Use cookiejar from oslo.vmware client directly https://review.opendev.org/c/openstack/glance_store/+/787715 | 18:27 |
| opendevreview | Merged openstack/glance master: Replace collections.Iterable https://review.opendev.org/c/openstack/glance/+/773357 | 20:39 |
| opendevreview | Merged openstack/glance master: Remove references to sys.version_info https://review.opendev.org/c/openstack/glance/+/793513 | 20:41 |
| opendevreview | Merged openstack/glance master: Changed minversion in tox to 3.18.0 https://review.opendev.org/c/openstack/glance/+/794404 | 20:59 |
| opendevreview | Cyril Roelandt proposed openstack/python-glanceclient master: glance help <subcommand>: Clearly specify which options are mandatory https://review.opendev.org/c/openstack/python-glanceclient/+/797779 | 21:20 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!