PR#959: Make repo_include_all setting configurable in xmd - fm-orchestrator

fm-orchestrator

#959 Make repo_include_all setting configurable in xmd

Merged 5 years ago by ralph. Opened 5 years ago by mizdebsk.

mizdebsk/fm-orchestrator fix-957 into master

Make repo_include_all setting configurable in xmd

Mikolaj Izdebski • 5 years ago

0d29633

module_build_service/builder/KojiModuleBuilder.py

file modified

		`@@ -838,6 +838,10 @@`
		`'repo_include_all': True,`
		`}`

		`+ xmd = self.mmd.get_xmd()`
		`+ if "mbs_options" in xmd.keys() and "repo_include_all" in xmd["mbs_options"].keys():`
		`+ opts['extra']['repo_include_all'] = xmd["mbs_options"]["repo_include_all"]`
		`+`
		`# edit tag with opts`
		`self.koji_session.editTag2(tag_name, **opts)`
		`return self._get_tag(tag_name) # Return up2date taginfo`

tests/test_builder/test_koji.py

file modified

+19 -1

		`@@ -414,7 +414,8 @@`

		`@pytest.mark.parametrize('blocklist', [False, True])`
		`@pytest.mark.parametrize('custom_whitelist', [False, True])`
		`- def test_buildroot_connect(self, custom_whitelist, blocklist):`
		`+ @pytest.mark.parametrize('repo_include_all', [False, True])`
		`+ def test_buildroot_connect(self, custom_whitelist, blocklist, repo_include_all):`
		`if blocklist:`
		`mmd = self.module.mmd()`
		`xmd = glib.from_variant_dict(mmd.get_xmd())`
		`@@ -429,6 +430,15 @@`
		`mmd.set_buildopts(opts)`
		`self.module.modulemd = mmd.dumps()`

		`+ if repo_include_all is False:`
		`+ mmd = self.module.mmd()`
		`+ xmd = glib.from_variant_dict(mmd.get_xmd())`
		`+ mbs_options = xmd["mbs_options"] if "mbs_options" in xmd.keys() else {}`
		`+ mbs_options["repo_include_all"] = False`
		`+ xmd["mbs_options"] = mbs_options`
		`+ mmd.set_xmd(glib.dict_values(xmd))`
		`+ self.module.modulemd = mmd.dumps()`
		`+`
		`builder = FakeKojiModuleBuilder(`
		`owner=self.module.owner, module=self.module, config=conf, tag_name='module-foo',`
		`components=["nginx"])`
		`@@ -466,6 +476,14 @@`
		`expected_calls = []`
		`assert session.packageListBlock.mock_calls == expected_calls`

		`+ expected_calls = [mock.call('module-foo', arches='i686 armv7hl x86_64',`
		`+ extra={'mock.package_manager': 'dnf',`
		`+ 'repo_include_all': repo_include_all}),`
		`+ mock.call('module-foo-build', arches='i686 armv7hl x86_64',`
		`+ extra={'mock.package_manager': 'dnf',`
		`+ 'repo_include_all': repo_include_all})]`
		`+ assert session.editTag2.mock_calls == expected_calls`
		`+`
		`@pytest.mark.parametrize('blocklist', [False, True])`
		`def test_buildroot_connect_create_tag(self, blocklist):`
		`if blocklist:`

mizdebsk commented 5 years ago

Fixes #957

mprahl commented 5 years ago

I'm fine with this but I'll leave it open to leave others time to review it.

ralph commented 5 years ago

Hm, this should probably affect component re-use. i.e., if it changes from False to True or vice versa, no components are re-usable.

ralph commented 5 years ago

FWIW, seems fine here too, generally speaking. @mizdebsk - wdyt about the component re-use question?

ralph commented 5 years ago

@mizdebsk, we've got some merge conflicts here now. Do you have time to look into it?

mizdebsk commented 5 years ago

Sorry for delayed reply

I would not be concerned about component reuse in this case:
1. First, this option only helps with some corner cases, but generally for most packages buildroot should be the same with the option set to either value. So for most packages it does not matter.
2. I think that packager toggling advanced and undocumented options like this can be expected to be aware about possible component reuse problems and submit module build with reuse disabled when necessary.
3. Component reuse is not perfect, and there are more important cases in which component reuse could be a problem, like when something in platform changes and component is reused anyway.

I will rebase this PR to fix merge conflicts.

rebased onto c549690981d0ecb887a1d278ef36ae058a4abfe6

5 years ago

ralph commented 5 years ago

Makes sense, and I agree.

ralph commented on line 18 of tests/test_builder/test_koji.py 5 years ago

Here, I get

___________________________________________ TestKojiBuilder.test_buildroot_connect[False-False-False] ____________________________________________

self = <test_koji.TestKojiBuilder instance at 0x7fccf4194a28>, custom_whitelist = False, blocklist = False, repo_include_all = False

    @pytest.mark.parametrize('blocklist', [False, True])
    @pytest.mark.parametrize('custom_whitelist', [False, True])
    @pytest.mark.parametrize('repo_include_all', [False, True])
    def test_buildroot_connect(self, custom_whitelist, blocklist, repo_include_all):
    if blocklist:
        mmd = self.module.mmd()
        xmd = glib.from_variant_dict(mmd.get_xmd())
        xmd["mbs_options"] = {"blocked_packages": ["foo", "nginx"]}
        mmd.set_xmd(glib.dict_values(xmd))
        self.module.modulemd = mmd.dumps()

    if custom_whitelist:
        mmd = self.module.mmd()
        opts = mmd.get_buildopts()
        opts.set_rpm_whitelist(['custom1', 'custom2'])
        mmd.set_buildopts(opts)
        self.module.modulemd = mmd.dumps()

    if repo_include_all is False:
        mmd = self.module.mmd()
        xmd = glib.from_variant_dict(mmd.get_xmd())
>           xmd.getdefault("mbs_options", {})["repo_include_all"] = "false"
E           AttributeError: 'dict' object has no attribute 'getdefault'

tests/test_builder/test_koji.py:436: AttributeError

ralph commented 5 years ago

@mizdebsk, should we change that getdefault to just a plain old get?

rebased onto 0d29633

5 years ago

ralph commented 5 years ago

Tests pass here now. Thanks @mizdebsk!