| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove VirtualBox https://review.opendev.org/c/openstack/openstack-manuals/+/948650 | 00:19 |
|---|---|---|
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 00:22 |
| opendevreview | Dmitriy Chubinidze proposed openstack/openstack-manuals master: Update "availability zone" term in Glossary https://review.opendev.org/c/openstack/openstack-manuals/+/948618 | 07:25 |
| opendevreview | Merged openstack/openstack-manuals master: Add Rocky Linux to Glossary https://review.opendev.org/c/openstack/openstack-manuals/+/948731 | 12:15 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 12:25 |
| opendevreview | Dmitriy Chubinidze proposed openstack/openstack-manuals master: WIP: Add AlmaLinux https://review.opendev.org/c/openstack/openstack-manuals/+/949034 | 13:45 |
| opendevreview | Dmitriy Chubinidze proposed openstack/openstack-manuals master: Add AlmaLinux to Glossary https://review.opendev.org/c/openstack/openstack-manuals/+/949034 | 13:55 |
| mnaser | i'm finding it harder and harder to land code in openstack projects lately | 15:07 |
| mnaser | (and if I'm struggling, i can't imagine new contributors having a good time) | 15:07 |
| mnaser | for example, i have a trivial backport that's open for 22 days.. but other changes are being merged | 15:08 |
| mnaser | i feel like we need to have some sort of project-wide way of pinging cores for reviews and tracking it | 15:12 |
| *** ralonsoh is now known as ralonsoh_out | 15:15 | |
| frickler | as core for a lot of projects, my impression is that I get rather pinged too much, so I'm not sure I'd advocate for even more pinging. otoh I acknowledge that important reviews do still get missed | 16:08 |
| frickler | what does help in my experience is getting to know the project better, which helps with knowing whom to ping who would actually be interested in doing a review and have time for it, but again I agree that this isn't an option for everyone | 16:10 |
| frickler | getting better at removing inactive cores might also be an option, but then, only doing a single review per cycle might still be valuable, so that's often a tough decision, too | 16:11 |
| mnasiadka | I think it's rather something we should analyse project-by-project instead of just assuming it's that bad everywhere | 16:12 |
| frickler | well I'm not aware of any project that isn't struggling with review backlog, but yes, different shades of red are definitively at play there | 16:14 |
| gouthamr | people are busy and code-reviews and maintenance is a side job for most.. i've found that having some scheduled activity works: review jams every week? personal review block? review assignments from PTLs | 16:14 |
| mnasiadka | Well, there's a lot of people that just want to land a feature, and there's only a handful of reviewers that are rather focused for the project sustainability in terms of maintenance and stability | 16:18 |
| gouthamr | yes | 16:18 |
| mnasiadka | In Kolla I think it did help a bit that we allowed the community to deliver a list of patches that they would like reviews on - in ideal world that wouldn't be needed because all patches would be reviewed on time - but we're past the initial hype where a patch had more than 2 reviewers :) | 16:19 |
| gouthamr | we should keep advocating for "earning trust" ... drive by contributors are welcome, but, reviewers are definitely going to pay more attention when you show up and help review other changes, or address tech debt/help with project maintenance in addition to chasing feature changes | 16:20 |
| gouthamr | its human nature | 16:20 |
| mnasiadka | yes, it's a lot better if somebody shows up on the weekly meeting or just does some interaction on the IRC channel regarding his contribution | 16:21 |
| mnasiadka | And we have a lot of contributions where people do not update the patch ever beyond PS1 | 16:21 |
| mnasiadka | And that's how we end up in a place that we usually review patches by known contributors or other core reviewers, because we know they are going to persist on adapting the patch as per reviews - compared to spending at least 15 minutes on a patch that will never get updated. | 16:22 |
| opendevreview | Dmitriy Chubinidze proposed openstack/openstack-manuals master: Add AlmaLinux to Glossary https://review.opendev.org/c/openstack/openstack-manuals/+/949034 | 16:48 |
| gouthamr | have we ever considered using the reviewers plugin on review.opendev.org? | 16:48 |
| gouthamr | it _may_ help | 16:48 |
| gouthamr | reviewers ofcourse could totally ignore changes even with review prompts | 16:49 |
| mnaser | maybe that might be more helpful, it just feels like it's a big issue that needs to be figured out for sustainability | 17:18 |
| JayF | frickler: I *hope* Ironic doesn't have a severe problem with review backlog. I personally really, really try to ensure things don't get lost even if I'm not always successful | 17:20 |
| gmaan | while fixing multiple things in many projects for example, rbac, CI, tempest plugins, I have observed that many project lack in review frequency and many time it is once or twice in a week (soimetime month). | 17:24 |
| gmaan | that is very common for core who are part time doing the upstream activity | 17:24 |
| gmaan | I feel asking projects to implement the 'Review Priority' label and at least monitor those priority review weekly meeting/office hour/or-sometime-in-week can solve such situation | 17:26 |
| gouthamr | meh | 17:27 |
| gouthamr | the RP hasn't really helped :( | 17:27 |
| gouthamr | personal opinion perhaps | 17:27 |
| gouthamr | but, its a binary flag, and you could have too many things prioritized, and hence ignored | 17:28 |
| gmaan | frickler: on core cleanup list, I think it create more confusion to have list out of dated than having review help sometime. But at least we can cleanup who are clearly not active at all. | 17:28 |
| gmaan | Rp can only be helpful when team monitor those weekly basis | 17:28 |
| gmaan | if they do not then yes it does not make sense either you tag them as priority or raise multiple places | 17:29 |
| gmaan | at least it is working fine in QA | 17:30 |
| JayF | I think the path Ironic took of separating approvers/reviewers so we could be more generous with giving newer contributoes a +2 is an option too | 17:30 |
| JayF | especially if we can garner enough trust between projects to make it easier for a primarily project X developer to have some review rights on project Y | 17:30 |
| mnaser | is this akin to the approvers/reviewers that k8s runs? | 17:30 |
| mnaser | lgtm/approve? | 17:30 |
| JayF | I do not know how k8s does anything :) | 17:31 |
| JayF | but in gerrit terms, cid and cardoe have +2 code review votes but not a +1 workflow | 17:31 |
| mnaser | https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md#the-code-review-process | 17:31 |
| gmaan | almost yes, they have same, reviewers can lgtm but not merge. and only few other can approve the things | 17:31 |
| gmaan | even 'member' and 'reviewer' both can lgtm but lgtm from 'reviewer' is helpful for approver | 17:32 |
| JayF | so people in that group are able to contribute reviews, but we ensure that at least one more senior team member has to review something to land | 17:32 |
| mnaser | and that group is very vast, because its anyone part of the k8s (or specific github org) | 17:32 |
| mnaser | aka anyone in kubernete-sigs can lgtm | 17:32 |
| mnaser | and barrier for org joining is low, just need to get sponsored and show impact in the project | 17:33 |
| JayF | it's certainly not that vast for Ironic; but I do think being more open is a part of the path | 17:33 |
| gmaan | yeah but I have seen approver check 'Reviewer' lgtm as key input | 17:33 |
| mnaser | maybe we need to auto close things after 90 days to avoid noise | 17:34 |
| gmaan | so there is distinguish list of 'Reviewer' maintained per repo in k8s which is where it is more helpful | 17:34 |
| gmaan | I am 100% sure auto close thing will not work because core member can have a simple gerrit query to list reviews with zuul+1, no -1, open changes | 17:36 |
| gmaan | it is about many projects does not even check open reviews/state of project for weeks, many might be showing up during release time only | 17:36 |
| JayF | yes | 17:37 |
| gmaan | if core check open reviews weekly basis, it makes situation better | 17:37 |
| JayF | we shouldn't pretend this is about visibility of reviews | 17:37 |
| JayF | it's about people not checking or purposefully prioritizing reviews from people/features they are more engaged with | 17:37 |
| gmaan | yeah | 17:37 |
| JayF | "Wow this project moves slow, but that makes sense it's feature complete" is a different experience than "Why have ten other patches gotten reviewed and landed and I don't even have a comment?" | 17:38 |
| JayF | even /if/ there's a reason for the prioritization, it's a bad experience and not communicated in any visible way in gerrit | 17:38 |
| gmaan | mnaser: if you do not mind pointing those project it will be helpful for TC to keep eyes on that. As you know, we do keep eyes on gate tests/release etc but input about reviews/not merging things can be helpful | 17:40 |
| gmaan | at lest we could bring it during election to PTL candidates/DPL liaisons that this is problem this project facing and what their plan to solve it | 17:41 |
| mnaser | gmaan: for example, https://review.opendev.org/c/openstack/cinder/+/910700 is almsot a year open, and we've been running it locally in atmosphere because we can't wait that long for things to land | 17:42 |
| mnaser | https://review.opendev.org/c/openstack/ovn-octavia-provider/+/945798 minor bug fixes open for a month | 17:43 |
| mnaser | https://review.opendev.org/c/openstack/ovn-bgp-agent/+/947151 backports nearing a month of what should be relatively straightforward.. | 17:44 |
| mnaser | https://review.opendev.org/c/openstack/ovn-octavia-provider/+/942940 sometimes even such trivial stuff makes me wonder if they're even being actively checked | 17:44 |
| mnaser | sometimes we'll push up stuff, get a lot of back and forth - https://review.opendev.org/c/openstack/horizon/+/924729 -- we get distracted with other stuff, only for it to land later with almost the same fix -- https://review.opendev.org/c/openstack/horizon/+/935854 -- but ours was "we need a unit test" and this one shipped right through | 17:46 |
| gmaan | ric replied on cinder one, it seems waiting for one review comment. slaweq ^^ can help to land neutron ones. | 17:47 |
| mnaser | right, rico had to ping a month ago after it sat idle for many months .. and i totally get it, we can find ways to help land them, but i'm trying to dig a bit at the root cause rather than just .. ok for this we can do that | 17:47 |
| gmaan | horizon one is surprise, not sure why one need unit test and other do not :) | 17:48 |
| gmaan | I feel that is where Review Priority worked well in QA but those needs to be open for non-core also. if we see RP label +1 by author we do check and add core RP label also it helps to keep unnecessary/large list of RP to reviews. | 17:50 |
| gmaan | but again project needs to monitor those Rp changes weekly basis | 17:50 |
| gmaan | its a simple way to filter key reviews to look if they missed those due to other work or so | 17:51 |
| gmaan | basically way for author to express how important that change is | 17:51 |
| JayF | But like, even a less important bugfix shouldn't be allowed to sit indefinately | 17:53 |
| JayF | it's not a situation where sometime, 6 months later when someone has time they'll review it | 17:53 |
| JayF | I've never had a review that sat for longer than 2 weeks without a review *get* another review until I pinged someone; maybe holidays excepted | 17:53 |
| gmaan | sure, but without some filter, project things everything goes in least priority list. having priority list does not stop them to review all which they should anyways | 17:55 |
| gouthamr | +1 review priority coordinated through gerrit, or review focus through review-jams or etherpads with weekly check-ins will help sort out what the team can deem "important".. but we wouldn't solve the problem of stagnating patches.. | 17:57 |
| gouthamr | at the PTG, i briefly highlighted the review dashboard from bitergia that PTLs/core reviewers could use.. carloss found this information useful to shepherd languishing changes, and motivate cores | 17:57 |
| gouthamr | for the lack of a legit url shortener: | 17:57 |
| gouthamr | here's a view of how cinder did from Sep 2024: | 17:57 |
| gouthamr | https://openstack.biterg.io/app/dashboards#/view/8c515590-e1de-11e8-8aac-ef7fd4d8cbad?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:'2024-09-16T21:17:53.449Z',to:'2025-04-01T21:18:03.038Z'))&_a=(description:'Efficiency%20panel%20for%20Gerrit%20by%20Bitergia',filters:!(('$state':(store:appState),meta:(alias:'Changesets%20Only',disabled:!f,index:gerrit,key:type,negate:!f,params:(query:changeset),type:phrase),query:(matc | 17:57 |
| gouthamr | h:(type:(query:changeset,type:phrase)))),('$state':(store:appState),meta:(alias:Bots,disabled:!f,index:gerrit,key:author_bot,negate:!t,params:(query:!t),type:phrase),query:(match:(author_bot:(query:!t,type:phrase)))),('$state':(store:appState),meta:(alias:!n,controlledBy:'1541521075121',disabled:!f,index:gerrit,key:repository,negate:!f,params:(query:openstack%2Fcinder),type:phrase),query:(match_phrase:(repository:openstack%2Fcinder)) | 17:57 |
| gouthamr | )),fullScreenMode:!f,options:(darkTheme:!f,hidePanelTitles:!f,useMargins:!t),query:(language:lucene,query:''),timeRestore:!f,title:'Gerrit%20Efficiency',viewMode:view) | 17:57 |
| gouthamr | median open time for a patch: 57.028 days | 17:58 |
| mnaser | well we're gonna need a bit.ly or something for that | 17:59 |
| mnaser | :P | 17:59 |
| gouthamr | lots of other ready dashboards to mine information from there, so i'll not spam you with links | 18:00 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Glossary - remove Linux Bridge https://review.opendev.org/c/openstack/openstack-manuals/+/948653 | 18:06 |
| frickler | JayF: oh, I didn't think of ironic, that might indeed be doing quite good | 18:59 |
| JayF | frickler: I honestly try really hard, it's basically my #1 upstream priority to make sure every change I'm qualified to review (and am core on) gets a review | 19:15 |
| fungi | gouthamr: i've not fully caught up on the discussion, but i do still have https://review.opendev.org/724914 proposed to add the reviewers plugin based on a request from starlingx maintainers 5 years ago | 19:27 |
| fungi | it'll almost certainly need a rebase or rework now, if anyone wants to take it over | 19:28 |
| gouthamr | ah nice | 19:29 |
| fungi | maybe also an example of the problem you're discussing ;) | 19:36 |
| gouthamr | "ironic" | 19:40 |
| gouthamr | i rebased the change :D i can volunteer some repos as guinea-pigs | 19:41 |
| opendevreview | Merged openstack/openstack-manuals master: Add AlmaLinux to Glossary https://review.opendev.org/c/openstack/openstack-manuals/+/949034 | 20:19 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Linux Bridge https://review.opendev.org/c/openstack/openstack-manuals/+/948653 | 20:20 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 20:24 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 20:31 |
| fungi | gouthamr: as noted in my earlier comments, we'll also need a mechanism (in opendev/jeepyb manage-projects would be the most appropriate) to push the reviewers files into gerrit for it too | 21:07 |
| * gouthamr will look | 21:07 | |
| fungi | and anyway, i won't be able to review until next week at the earliest | 21:07 |
| gouthamr | unrelated: i forgot that the proposal bot patches keep getting respun if we let them sit for a while.. these glossary updates are now broken down like we want, except they also spawn a bunch of bot changes | 21:08 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Cloudpipe https://review.opendev.org/c/openstack/openstack-manuals/+/948637 | 21:08 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Glossary - update for term Scoped Token https://review.opendev.org/c/openstack/openstack-manuals/+/948646 | 21:09 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 21:12 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Xen https://review.opendev.org/c/openstack/openstack-manuals/+/948651 | 21:15 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Sahara https://review.opendev.org/c/openstack/openstack-manuals/+/948645 | 21:15 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Panko https://review.opendev.org/c/openstack/openstack-manuals/+/948644 | 21:15 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove nova-network https://review.opendev.org/c/openstack/openstack-manuals/+/948655 | 21:16 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Murano https://review.opendev.org/c/openstack/openstack-manuals/+/948642 | 21:16 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove TripleO https://review.opendev.org/c/openstack/openstack-manuals/+/948648 | 21:16 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Glossary - remove Senlin https://review.opendev.org/c/openstack/openstack-manuals/+/948638 | 21:17 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Glossary - remove Senlin https://review.opendev.org/c/openstack/openstack-manuals/+/948638 | 21:17 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Glossary - remove Senlin https://review.opendev.org/c/openstack/openstack-manuals/+/948638 | 21:19 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 21:22 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 21:41 |
| opendevreview | Merged openstack/openstack-manuals master: Glossary - remove Senlin https://review.opendev.org/c/openstack/openstack-manuals/+/948638 | 21:45 |
| opendevreview | OpenStack Proposal Bot proposed openstack/security-doc master: Updated from openstack-manuals https://review.opendev.org/c/openstack/security-doc/+/948961 | 21:54 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!