check_source.py: Use correct function for getting maintainer of the devel pkg

[Vogtinator]

"maintainers_get" is a bit weird and uses /search/owner and not the given prj.

Noticed in https://build.opensuse.org/request/show/1132698.

[DimStar77]

impressive this did not blow up much more

[DimStar77]

maintainers_get seems to be doing what one would expect:

def maintainers_get(apiurl, project, package=None):
    if package is None: 
        meta = ET.fromstringlist(show_project_meta(apiurl, project))
        maintainers = meta.xpath('//person[@role="maintainer"]/@userid')

        groups = meta.xpath('//group[@role="maintainer"]/@groupid')
        maintainers.extend(groups_members(apiurl, groups))

        return maintainers
    root = owner_fallback(apiurl, project, package)
    maintainers = root.xpath('//person[@role="maintainer"]/@name')

    groups = root.xpath('//group[@role="maintainer"]/@name')
    maintainers.extend(groups_members(apiurl, groups))

    return maintainers

[Vogtinator]

maintainers_get seems to be doing what one would expect:

In which way?

[DimStar77]

maintainers_get seems to be doing what one would expect:

In which way?

it reads the meta and finds the maintainer role groupid and userid

[Vogtinator]

maintainers_get seems to be doing what one would expect:

In which way?

it reads the meta and finds the maintainer role groupid and userid

It doesn't, it uses /search/owner

[DimStar77]

See pasted code in #3041 (comment)

there is no search/owner in there

[Vogtinator]

See pasted code in #3041 (comment)

there is no search/owner in there

There is: root = owner_fallback(apiurl, project, package)

[DimStar77]

> grep maintainers_get . -r
./osclib/core.py:def maintainers_get(apiurl, project, package=None):
./check_maintenance_incidents.py:from osclib.core import maintainers_get
./check_maintenance_incidents.py:        maintainers = set(maintainers_get(self.apiurl, project, pkgname))
./devel-project.py:            userids = maintainers_get(apiurl, devel_project, devel_package)
./devel-project.py:def maintainers_get(apiurl, project, package=None):
./devel-project.py:        return maintainers_get(apiurl, project)
./devel-project.py:    userids = sorted(maintainers_get(apiurl, project, package))
./ReviewBot.py:from osclib.core import maintainers_get
./ReviewBot.py:        maintainers = set(maintainers_get(self.apiurl, project, package))
./check_source.py:from osclib.core import maintainers_get
./check_source.py:                maintainers = set(maintainers_get(self.apiurl, devel_project, devel_package))

Let's try to clean stuff up then... we have multiple consumers of mainers_get (and apparently also multiple provides.. yay)
I'd argue that if maintainers_get is wrong, it will likely be equally wrong in all cases and deserves to be fixed properly

[Vogtinator]

Let's try to clean stuff up then... we have multiple consumers of mainers_get (and apparently also multiple provides.. yay) I'd argue that if maintainers_get is wrong, it will likely be equally wrong in all cases and deserves to be fixed properly

I expect that for some cases it's not actually wrong but necessary and meant to get the actual "package maintainer" instead of the "technical devel package maintainer"... I don't really get the logic though, especially for those methods containing "fallback" in their names.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8e4cdc3) 28.31% compared to head (aa1b3d6) 28.31%.

Files	Patch %	Lines
ReviewBot.py	0.00%	1 Missing ⚠️
check_source.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3041   +/-   ##
=======================================
  Coverage   28.31%   28.31%           
=======================================
  Files          86       86           
  Lines       14803    14803           
=======================================
  Hits         4191     4191           
  Misses      10612    10612           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nilxam]

To be honest when I working on 7a088e7 I'm confused with maintainers_get/owner_fallback a bit, it sometimes gave a weird result, I spent time on debugging then move to other errands after have some findings, if my memory is right, the culprit is owner(), the default search mode is binary that means it has binary=livecd-openSUSE as an param of api call, if uses mode=package then it has package=livecd-openSUSE in query params. The right fix should be on maintainers_get() + owner_fallback() to support different mode and pass a package mode to owner(). see https://github.com/openSUSE/osc/blob/master/osc/core.py#L7675

I'm not sure does default has mode=binary makes sense, but as it was there so long probably it works for other scenarios? perhaps maintenance review? if none uses binary mode we can force it to be mode=package in owner_fallback()

[nilxam]

#3046 can be a rather proper fix here IMO.

[nilxam]

osc maintainer PRJ PKG triggers /source/PRJ/PKG/_meta, indeed use package_role_expand() is matches the behavior osc maintainer command does.

[Vogtinator]

I'll have a look at the other uses of maintainer_* and owner_*, maybe I can come up with something more consistent and less misleading.