Closed Vogtinator closed 6 months ago
impressive this did not blow up much more
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
maintainers_get
seems to be doing what one would expect:
In which way?
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
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
See pasted code in https://github.com/openSUSE/openSUSE-release-tools/pull/3041#issuecomment-1852302091
there is no search/owner in there
See pasted code in #3041 (comment)
there is no search/owner in there
There is: root = owner_fallback(apiurl, project, package)
> 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
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.
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 :warning: |
check_source.py | 50.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
To be honest when I working on https://github.com/openSUSE/openSUSE-release-tools/commit/7a088e778459a1a27b3ba035cb5dac07bc43987f 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()
https://github.com/openSUSE/openSUSE-release-tools/pull/3046 can be a rather proper fix here IMO.
osc maintainer PRJ PKG
triggers /source/PRJ/PKG/_meta
, indeed use package_role_expand()
is matches the behavior osc maintainer
command does.
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.
"maintainers_get" is a bit weird and uses /search/owner and not the given prj.
Noticed in https://build.opensuse.org/request/show/1132698.