| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acl : check for function/s-r in normalize https://review.opendev.org/c/openstack/project-config/+/875997 | 01:58 |
|---|---|---|
| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acls : fix some missed NoOp functions https://review.opendev.org/c/openstack/project-config/+/877569 | 01:58 |
| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acl : check for capital booleans in normalize https://review.opendev.org/c/openstack/project-config/+/877571 | 02:37 |
| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acl : fix some missed NoOp functions https://review.opendev.org/c/openstack/project-config/+/877569 | 02:38 |
| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acl : check for function/s-r in normalize https://review.opendev.org/c/openstack/project-config/+/875997 | 02:38 |
| opendevreview | Ian Wienand proposed openstack/project-config master: gerrit/acl : check for capital booleans in normalize https://review.opendev.org/c/openstack/project-config/+/877571 | 02:38 |
| *** gibi_pto is now known as gibi | 08:04 | |
| opendevreview | Merged openstack/openstack-zuul-jobs master: Start translations for 2023.1 (Antelope) stable branch https://review.opendev.org/c/openstack/openstack-zuul-jobs/+/877550 | 08:07 |
| *** jpena|off is now known as jpena | 08:21 | |
| *** thelounge554 is now known as thelounge55 | 08:37 | |
| *** jpena is now known as jpena|off | 08:43 | |
| *** jpena|off is now known as jpena | 08:52 | |
| *** elodilles_pto is now known as elodilles | 09:26 | |
| lajoskatona | Hi, I have a question regarding gerrit UI. If I list a bunch of patches (or just view a dashboard like the self dashboard), I see the extra column RP (Review-Priority), and it has a big green tick in it even if the patch itself has no priority set | 15:28 |
| clarkb | lajoskatona: can you link to an example listing? | 15:29 |
| lajoskatona | shall I set something differently for my dashboards, or is this the new default? | 15:29 |
| clarkb | the green tick means that the requirment for that field is satisfied. Its possible that the submit-requirement is overly broad | 15:29 |
| lajoskatona | clarkb: example Neutron list: https://review.opendev.org/q/project:openstack%252Fneutron+status:open | 15:29 |
| clarkb | lajoskatona: ya so if you open one of those changes review-priority does apply to the neutron repo. Then if you hover it in the UI it shows you the view condition option and if ou do that the condition is no minimum vote set | 15:30 |
| lajoskatona | clarkb: for these the review-prioroty is mostly not set (I suppose that is 0), so it is just a little difficult to parse which patches I have already a +2 because the columns are close to ech other :-) | 15:31 |
| clarkb | lajoskatona: I think the issue here is that we ported the old function value which didn't allow merging with a minimum value to submit-requirements to not permit a minimum value, but now gerrit renders taht slightly differently due to the submit requirements extra ui tooling | 15:31 |
| clarkb | basically this is equivalent to what you had before but with more UI status info. It may be the case that you never actually wanted the minimum vote blocks behavior but that is what existed and we ported it and now it is more apparent | 15:32 |
| lajoskatona | clarkb: ok, so it should have worked this way in the past? | 15:34 |
| clarkb | lajoskatona: the underlying functionality was he same. But we converted it from label function to submit-requirements and gerrit has better support for submit requirements in the UI | 15:35 |
| clarkb | if you give me a few minutes I can link you to the change | 15:35 |
| elodilles | i can also second the new faulty behaviour: we have the same thing but with our PTL-Approved flag. now every patch looks on our dashboard as PTL-Approve ✔ | 15:37 |
| elodilles | here is the link to our dashboard: https://releases.openstack.org/reference/reviewer_guide.html#review-inbox | 15:38 |
| elodilles | (with the link 'Gerrit Release Review Inbox', didn't want to flood the channel with the lengthy link o:)) | 15:39 |
| clarkb | note it isn't faulty | 15:41 |
| clarkb | at least not with my current understanding of the issue | 15:41 |
| elodilles | yepp, the label seems right if we query directly, but the UI shows it wrongly :) | 15:41 |
| clarkb | I haven't looked at your example elodilles but the UI is correct in lajoskatona case from what I can tell | 15:42 |
| clarkb | lajoskatona: https://review.opendev.org/c/openstack/project-config/+/875995/6/gerrit/acls/openstack/neutron.config this is the update. The function anywithblock means any value except the lowest value blocks. We converted that to a submit requirement that says any value but the lowest value blocks | 15:43 |
| clarkb | lajoskatona: since your changes have no votes that counts as any value that is not the min value and you get the check mark. The functionality should be identical its just that gerrit shows you that info in the UI now where it didn't before because the function system didn't support that | 15:43 |
| clarkb | elodilles: can you link to your dashboard? The link above is not a gerrit link | 15:44 |
| elodilles | clarkb: yes, let me copy here the link under 'Gerrit Release Review Inbox' | 15:44 |
| clarkb | lajoskatona: this may expose that you don't want to restrict merging with review priority and the old/new behavior is wrong. But I don't think we changed the behavior just made it mor eevident | 15:44 |
| elodilles | clarkb: | 15:45 |
| elodilles | https://review.opendev.org/dashboard/?title=Releases+Inbox&foreach=is%3Aopen&Antelope=project%3Aopenstack%2Freleases+file%3A%5Edeliverables%2Fantelope%2F.%2A+NOT+label%3AWorkflow%2D1&Zed=project%3Aopenstack%2Freleases+file%3A%5Edeliverables%2Fzed%2F.%2A+NOT+label%3AWorkflow%2D1&Yoga=project%3Aopenstack%2Freleases+file%3A%5Edeliverables%2Fyoga%2F.%2A+NOT+label%3AWorkflow%2D1&Xena=project%3Aopenst | 15:45 |
| elodilles | ack%2Freleases+file%3A%5Edeliverables%2Fxena%2F.%2A+NOT+label%3AWorkflow%2D1&Independent=project%3Aopenstack%2Freleases+file%3A%5Edeliverables%2F_independent%2F.%2A+NOT+label%3AWorkflow%2D1&Tools+and+Jobs=%28%28+project%3Aopenstack%2Freleases+file%3A%5Etools%2F.%2A+%29+OR+project%3Aopenstack%2Frelease%2Dtest+OR+%28+project%3Aopenstack%2Freleases+file%3A%5Eopenstack_releases%2F.%2A+%29+OR+project | 15:45 |
| elodilles | %3Aopenstack%2Freno+OR+%28+project%3Aopenstack%2Fproject%2Dconfig+file%3A%5Eroles%2Fcopy%2Drelease%2Dtools%2Dscripts%2Ffiles%2Frelease%2Dtools%2F.%2A+%29%29&Other=%28%28+project%3Aopenstack%2Freleases+NOT+file%3A%5Edeliverables%2F.%2A%29+OR+%28+project%3Aopenstack%2Freleases+directory%3Adeliverables+NOT+label%3AWorkflow%2D1++%2Ddirectory%3Adeliverables%2Fantelope+%2Ddirectory%3Adeliverables%2Fze | 15:45 |
| clarkb | oh wow | 15:45 |
| elodilles | d+%2Ddirectory%3Adeliverables%2Fyoga+%2Ddirectory%3Adeliverables%2Fxena+%2Ddirectory%3Adeliverables%2F_independent%29%29&All+Releases=is%3Aopen+project%3Aopenstack%2Freleases | 15:45 |
| elodilles | :S | 15:45 |
| elodilles | it is worse than i thought | 15:45 |
| elodilles | anyway, here is a part of it: https://review.opendev.org/q/is:open+is:open+project:openstack/releases | 15:46 |
| clarkb | elodilles: you have the same exact situation as lajoskatona this isn't broken the UI is correct | 15:46 |
| clarkb | elodilles: https://review.opendev.org/c/openstack/project-config/+/875996/6/gerrit/acls/openstack/releases.config is the update | 15:47 |
| elodilles | i see, though we don't know now which patch has PTL-Approved+1 by looking at our board :( | 15:47 |
| clarkb | I think you should update your submit requirement condition in that case | 15:48 |
| elodilles | of course if we query expicitly for label:PTL-Approved+1 then we get the correct list | 15:48 |
| elodilles | clarkb: ack, thanks, will look into that | 15:48 |
| clarkb | basically we converted the deprecated function that is going away in the future so that we can upgrade gerrit on April 6 | 15:48 |
| clarkb | we did so by directly porting the existing behavior to a submit requirement with the same behavior. Gerrit will now render that information for you in the web UI in a richer manner and that is creating the confusion but the underlying behavior should not have changed | 15:49 |
| elodilles | ah, i see, so this is needed for the upgrade, ack | 15:49 |
| clarkb | elodilles: yes Gerrit 3.7 will not accept config updates if they include a function other than NoOp/NoBlock | 15:49 |
| clarkb | so we need to convert all of our configs ahead of time otherwise the next update people make after the upgrade will fail in a confusing way | 15:50 |
| elodilles | :S | 15:50 |
| lajoskatona | clarkb: thanks, I check it, and read about it to understand the possibilities | 15:53 |
| *** jpena is now known as jpena|off | 17:23 | |
| clarkb | elodilles: I'm looking more closely at PTL Approval and I think it is buggy. We allow a -1 vote but no one has permisions to set that? and we don't actually require anyone to vote positively to land anything | 18:02 |
| clarkb | I think we should drop the -1 vote from the label entirely snce it is unused. Then 0 the default value becomes the min value and the requirement won't be satisfied unless the PTL has approved it | 18:05 |
| clarkb | however, keep in mind this will prevent you from landing changes without ptl approval. If we need to do that then we should remove the minimum requirement altogether (there is some standard way of expressing that that ianw found in gerrit migratiosn we can apply) | 18:05 |
| fungi | clarkb: i believe that was done in an attempt to work around the vote display issues in the change list view, but turned out to be inadequate | 18:10 |
| fungi | should be able to reset the lowest vote there to 0 | 18:10 |
| clarkb | ah ok | 18:11 |
| clarkb | I'm not sure if the releases team overrides ptls occasionally | 18:11 |
| clarkb | but if they do we want to make sure that is still possible after making changes | 18:11 |
| fungi | though i could be wrong, i'm not finding the change i was thinking of in the git history for gerrit/acls/openstack/releases.config | 18:12 |
| clarkb | if we remove the -1 what will happen is 0 becomes minimum and with the current submit-requirement it will force a +1 to be mergeable | 18:13 |
| clarkb | I think this more accurately expresses what elodilles expects the workflow to be. However we may need an out for that. We can add an out allowing the release team to add a +1 there to override | 18:14 |
| clarkb | or we can simply stop making it a submit requirement | 18:14 |
| fungi | yeah, okay, it was done in https://review.opendev.org/867801 back in december, that's the change i was thinking of | 18:14 |
| clarkb | aha and it ws noop then too | 18:15 |
| clarkb | so ya probably the best thing is to go back to that state | 18:15 |
| fungi | so should be entirely safe to undo, the -1 was only added to try getting the column included | 18:15 |
| fungi | which didn't work | 18:16 |
| clarkb | remove the -1 and also update the submit-requirement to be more of a trigger requirement | 18:16 |
| clarkb | ianw: ^ when you have a chance this discussion will likely interest you | 18:16 |
| fungi | it's possible other projects recently copied the same pattern for similarly hopeful (but ultimately fruitless) reasons | 18:16 |
| fungi | i know there was a rollcall label change proposed for openstack/governance to try the same approach | 18:17 |
| elodilles | clarkb: thanks for thinking on the issue. well, we need quite often to merge patches without PTL-Approved | 18:27 |
| elodilles | clarkb: 1st of all it is only needed when a file changes under deliverables/ directory, and sometimes we also merge those patches as well without PTL-Approved | 18:27 |
| elodilles | clarkb: it just makes our review much easier to have the PTL-Approved | 18:28 |
| clarkb | elodilles: ya I think the best option then is to convert this to a proper noop with a -1 vote | 18:28 |
| clarkb | but then it will drop the column from the listing entirely Ithink? | 18:28 |
| clarkb | becuse it won't be a submit rquirement? ianw would know more | 18:28 |
| elodilles | yes, that was the other problem some month ago :/ | 18:29 |
| fungi | keep in mind that ptl-approved is not something a human ever sets | 18:29 |
| clarkb | fungi: yes I called that out earlier | 18:29 |
| elodilles | that is also an issue as with that we loose just that important functionality why PTL-Approved exists :/ | 18:29 |
| clarkb | elodilles: right if you want that column at all it has to be a submit-requirement | 18:29 |
| clarkb | submit rquirements imply something is authorizing the change to be mergeable and without that it cannot merge | 18:30 |
| clarkb | really the underlying issue here is we are trying to abuse gerrit tools for something ehy weren't intended for | 18:30 |
| clarkb | and gerrit has made assumptions about how they should operate that are at odds with how you want it to operate | 18:30 |
| elodilles | clarkb: ack, then we need to play something with the submit-requirement option :S I'll try to look into it. if you have any idea how it will work the way we want then it would be awesome | 18:30 |
| fungi | maybe a hashtag would make more sense? have the zuul job set or remove the ptl-approved hashtag on the change if a ptl or release liaison for the deliverable has voted favorably, then filter on that hashtag in the dashboard query? | 18:31 |
| fungi | though humans could probably also set that hashtag, emulating zuul. at least the label can be limited to zuul's account | 18:31 |
| fungi | yeah, forget that suggestion ;) | 18:31 |
| elodilles | at 1st it sounded good :] | 18:32 |
| fungi | maybe just also allow the release team to set ptl-approved if they need to bypass actual ptl approval? | 18:32 |
| fungi | that could be the simplest solution | 18:32 |
| clarkb | yup that is an option | 18:32 |
| elodilles | that would be logical for release patches, but for other non-release patches (tools, python module, zuul job related patches) it is quite bothersome :/ | 18:33 |
| elodilles | anyway, i'll discuss this within release team as well, and we will try to get to a compromise :S if not sooner, then on the PTG | 18:36 |
| elodilles | thanks so far to thinking on it! | 18:36 |
| fungi | well, it's one extra click. alternatively, adjust the job to set ptl-approved automatically on any change which doesn't change files that need ptl approval? | 18:36 |
| clarkb | you might be able to do file specific submittableIf queries | 18:37 |
| elodilles | fungi: yepp, that is another workaround that could work ^^^ | 18:37 |
| clarkb | basicaly say submittiable if it changes anything but the releases dir or updates the releases dir and has a +1 from ptl approval | 18:37 |
| clarkb | the reason Gerrit made this change is so that you could write rules like this much more easily than the old prolog system | 18:38 |
| elodilles | clarkb: since we run the 'check approval' job anyway, it shouldn't be a big issue, though if nicer solution exists.... | 18:38 |
| opendevreview | Jeremy Stanley proposed openstack/project-config master: Restore rax-ord quota but lower max-concurrency https://review.opendev.org/c/openstack/project-config/+/877715 | 19:56 |
| opendevreview | Merged openstack/project-config master: Restore rax-ord quota but lower max-concurrency https://review.opendev.org/c/openstack/project-config/+/877715 | 20:16 |
| ianw | <clarkb> elodilles: ya I think the best option then is to convert this to a proper noop with a -1 vote | 21:52 |
| ianw | <clarkb> but then it will drop the column from the listing entirely Ithink? | 21:52 |
| ianw | ^ so I *think* this is something we've brought up with upstream | 21:53 |
| ianw | https://groups.google.com/g/repo-discuss/c/gYVD-iBR9Ow/m/p_i5L4_vAQAJ | 21:54 |
| ianw | i think part of the issue is that between zuul merging code, not people clicking buttons, and the way we've used labels to flag information like this, rather than straight "X ran and it worked or didn't", the ui is not crafted with us in mind | 21:57 |
| ianw | i think, that if we remove the submit-requirement it won't show | 22:13 |
| ianw | it being the column. i think the column only shows when there's a blocking vote | 22:13 |
| ianw | that said, it is probably worth having the s-r reflect reality of what's happening, which does not seem to be that -1 blocks submission, as that is set by zuul looking at who's voted to note if ptls have looked at things | 22:22 |
| opendevreview | Ian Wienand proposed openstack/project-config master: openstack/release : return to non-blocking submit rule https://review.opendev.org/c/openstack/project-config/+/877721 | 22:44 |
| ianw | ^ that's my understanding of what's going on there. we can probably discuss the details further there? | 22:44 |
| clarkb | ianw: ++ tjhanks! | 23:03 |
| opendevreview | Merged openstack/project-config master: gerrit/acl : fix some missed NoOp functions https://review.opendev.org/c/openstack/project-config/+/877569 | 23:20 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!