Fix association of unwanted modular rpms [RHELDST-6453] #147
Conversation
rbikar
force-pushed
the
unwanted-modular-pkgs
branch
from
564f70d
to
fefe7ed
Compare
|
This waits for #148 |
| filename="tomcatjss-7.3.6-1.el8+1944+b6c8e16f.noarch.rpm", | ||
| )] | ||
|
|
||
| mock_ubipop_runner._filter_pkgs_from_modules() |
There was a problem hiding this comment.
This is OK, but it'd be nice if there was at least one test which tested this at a higher level too in order to show the overall impact. The test here may protect against bugs in _filter_pkgs_from_modules, but it doesn't help ensure that the call to the method is wired up correctly. e.g. a refactor or incorrect resolution of a git conflict which accidentally removes the call to self._filter_pkgs_from_modules(), would not be caught by any test if I understand right.
There was a problem hiding this comment.
Yeah, no having high level test also lowers the code coverage.
This fixes a situation when modular packages are selected for output set of ubi repo but they shouldn't be there because respective modulemd unit is not selected as a content of ubi repo. This lead to situations when a repo contained modular rpm from a modulemd that wasn't associated with repo. This is wrong and could lead to problems with packages installation from ubi repos.
Co-authored-by: Rohan McGovern <rohan@mcgovern.id.au>
rbikar
force-pushed
the
unwanted-modular-pkgs
branch
from
1e894bd
to
5239083
Compare
ubipop/__init__.py
Outdated
| @@ -604,6 +604,18 @@ def _diff_lists_by_attr(self, list_1, list_2, attr): | |||
|
|
|||
| return diff | |||
|
|
|||
| def _filter_pkgs_from_modules(self): | |||
| regex = r'^\d+:' | |||
There was a problem hiding this comment.
@rohanpm
Actually the epoch is not the leading thing there, but it's name of the package.
E.g. tomcatjss-0:7.3.6-1.el8+1944+b6c8e16f.noarch
So the anchor breaks the expected behavior now :(
There was a problem hiding this comment.
Oops, my mistake. Adding one other minor suggestion about count=1 in re.sub then (because anchor would have ensured it only matched once, now it's not guaranteed).
This reverts commit 5239083.
| # name_stream, packages == str, list of _pulp_client.Package | ||
|
|
||
| wanted_packages = list(chain.from_iterable([m.packages for m in self.repos.modules[name_stream]])) | ||
| wanted_packages = [reg.sub('', rpm_nevra) + '.rpm' for rpm_nevra in wanted_packages] |
There was a problem hiding this comment.
One other minor suggestion - this reg.sub would be a little safer if you gave it a count of 1 (do max 1 substitution). Since we now remembered that the regex shouldn't be anchored at the beginning.
There was a problem hiding this comment.
Yep, but I'll do it in another PR - the same substitution happens also in another place in code, I guess it would be better to fix both and ideally do it at the time when self.repos.modules is populated.
I'll merge this PR, we need to test it and deploy it ASAP, it looks it creates a lot of customer problems.
Travis seems to fail only on pylint issues, I'll fix those also in another PR.
This fixes a situation when modular packages are selected for output set
of ubi repo but they shouldn't be there because respective modulemd unit
is not selected as a content of ubi repo.
This lead to situations when a repo contained modular rpm from a
modulemd that wasn't associated with repo. This is wrong and could lead
to problems with packages installation from ubi repos.