PR#61: Separate logic for generating yaml file from UI - copr/copr

		`@@ -1,3 +1,4 @@`
		`+ import os`
		`import time`
		`import base64`
		`import modulemd`
		`@@ -5,6 +6,7 @@`
		`from coprs import models`
		`from coprs import db`
		`from coprs import exceptions`
		`+ from coprs.logic import builds_logic`
		`from wtforms import ValidationError`


		`@@ -61,3 +63,56 @@`

		`db.session.add(module)`
		`return module`
		`+`
		`+`
		`+ class ModulemdGenerator(object):`
		`+ def __init__(self, name="", stream="", version=0, summary="", config=None):`
		`+ self.config = config`
		`+ self.mmd = modulemd.ModuleMetadata()`
		`+ self.mmd.name = name`
		`+ self.mmd.stream = stream`
		`+ self.mmd.version = version`
		`+ self.mmd.summary = summary`
		`+`
		`+ def add_api(self, packages):`
		`+ for package in packages:`
		`+ self.mmd.api.add_rpm(str(package))`
		`+`
		`+ def add_filter(self, packages):`
		`+ for package in packages:`
		`+ self.mmd.filter.add_rpm(str(package))`
		`+`
		`+ def add_profiles(self, profiles):`
		`+ for i, values in profiles:`
		`+ name, packages = values`
		`+ self.mmd.profiles[name] = modulemd.profile.ModuleProfile()`
		`+ for package in packages:`
		`+ self.mmd.profiles[name].add_rpm(str(package))`
		`+`
		`+ def add_components(self, packages, filter_packages, builds):`
		`+ build_ids = sorted(list(set([int(id) for p, id in zip(packages, builds)`
		`+ if p in filter_packages])))`
		`+ for package in filter_packages:`
		`+ build_id = builds[packages.index(package)]`
		`+ build = builds_logic.BuildsLogic.get_by_id(build_id).first()`
		`+ build_chroot = self._build_chroot(build)`
		`+ buildorder = build_ids.index(int(build.id))`
		`+ rationale = "User selected the package as a part of the module"`
		`+ self.add_component(package, build, build_chroot, rationale, buildorder)`
		`+`
		`+ def _build_chroot(self, build):`
		`+ chroot = None`
		`+ for chroot in build.build_chroots:`
		`+ if chroot.name == "custom-1-x86_64":`
		`+ break`
		`+ return chroot`
		`+`
		`+ def add_component(self, package_name, build, chroot, rationale, buildorder=1):`
		`+ ref = str(chroot.git_hash) if chroot else ""`
		`+ url = os.path.join(self.config["DIST_GIT_URL"], build.copr.full_name, "{}.git".format(build.package.name))`
		`+ self.mmd.components.add_rpm(str(package_name), rationale,`
		`+ repository=url, ref=ref,`
		`+ buildorder=buildorder)`
		`+`
		`+ def generate(self):`
		`+ return self.mmd.dumps()`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py

file modified

+10 -32

		`@@ -33,7 +33,7 @@`
		`from coprs.logic.packages_logic import PackagesLogic`
		`from coprs.logic.stat_logic import CounterStatLogic`
		`from coprs.logic.users_logic import UsersLogic`
		`- from coprs.logic.modules_logic import ModulesLogic`
		`+ from coprs.logic.modules_logic import ModulesLogic, ModulemdGenerator`
		`from coprs.rmodels import TimedStatEvents`

		`from coprs.logic.complex_logic import ComplexLogic`
		`@@ -970,38 +970,16 @@`
		`form.profile_pkgs.append_entry()`
		`return render_create_module(copr, form, profiles=len(form.profile_names))`

		`- mmd = modulemd.ModuleMetadata()`
		`- mmd.name = str(copr.name)`
		`- mmd.stream = str(form.stream.data)`
		`- mmd.version = form.version.data`
		`- mmd.summary = "Module from Copr repository: {}".format(copr.full_name)`
		`+ summary = "Module from Copr repository: {}".format(copr.full_name)`
		`+ generator = ModulemdGenerator(str(copr.name), str(form.stream.data),`
		`+ form.version.data, summary, app.config)`
		`+ generator.add_filter(form.filter.data)`
		`+ generator.add_api(form.api.data)`
		`+ generator.add_profiles(enumerate(zip(form.profile_names.data, form.profile_pkgs.data)))`
		`+ generator.add_components(form.packages.data, form.filter.data, form.builds.data)`
		`+ yml = generator.generate()`

		`- for package in form.filter.data:`
		`- mmd.filter.add_rpm(str(package))`
		`-`
		`- for package in form.api.data:`
		`- mmd.api.add_rpm(str(package))`
		`-`
		`- for i, values in enumerate(zip(form.profile_names.data, form.profile_pkgs.data)):`
		`- name, packages = values`
		`- mmd.profiles[name] = modulemd.profile.ModuleProfile()`
		`- for package in packages:`
		`- mmd.profiles[name].add_rpm(str(package))`
		`-`
		`- build_ids = sorted(list(set([int(id) for p, id in zip(form.packages.data, form.builds.data)`
		`- if p in form.filter.data])))`
		`- for package in form.filter.data:`
		`- build_id = form.builds.data[form.packages.data.index(package)]`
		`- build = builds_logic.BuildsLogic.get_by_id(build_id).first()`
		`-`
		`- chroot = builds_logic.BuildChrootsLogic.get_by_build_id_and_name(build.id, "custom-1-x86_64").first()`
		`- url = os.path.join(app.config["DIST_GIT_URL"], build.copr.full_name, "{}.git".format(build.package.name))`
		`-`
		`- mmd.components.add_rpm(str(package), "User selected the package as a part of the module",`
		`- repository=url, ref=str(chroot.git_hash),`
		`- buildorder=build_ids.index(int(build.id)))`
		`-`
		`- module = ModulesLogic.add(flask.g.user, copr, ModulesLogic.from_modulemd(mmd.dumps()))`
		`+ module = ModulesLogic.add(flask.g.user, copr, ModulesLogic.from_modulemd(yml))`
		`db.session.flush()`
		`actions_logic.ActionsLogic.send_build_module(flask.g.user, copr, module)`
		`db.session.commit()`

frontend/coprs_frontend/tests/coprs_test_case.py

file modified

+3 -1

		`@@ -215,7 +215,9 @@`
		`self.p1 = models.Package(`
		`copr=self.c1, name="hello-world", source_type=0)`
		`self.p2 = models.Package(`
		`- copr=self.c2, name="hello-world", source_type=0)`
		`+ copr=self.c2, name="whatsupthere-world", source_type=0)`
		`+ self.p3 = models.Package(`
		`+ copr=self.c2, name="goodbye-world", source_type=0)`

		`self.b1 = models.Build(`
		`copr=self.c1, package=self.p1, user=self.u1, submitted_on=50)`

frontend/coprs_frontend/tests/test_logic/test_modules_logic.py

file added

+88

		`@@ -0,0 +1,88 @@`
		`+ import yaml`
		`+ from mock import patch, ANY`
		`+ from tests.coprs_test_case import CoprsTestCase`
		`+ from coprs.logic.modules_logic import ModulemdGenerator`
		`+`
		`+`
		`+ class TestModulemdGenerator(CoprsTestCase):`
		`+ config = {"DIST_GIT_URL": "http://distgiturl.org"}`
		`+`
		`+ def test_basic_mmd_attrs(self):`
		`+ generator = ModulemdGenerator("testmodule", "master", 123, "Some testmodule summary", None)`
		`+ assert generator.mmd.name == "testmodule"`
		`+ assert generator.mmd.stream == "master"`
		`+ assert generator.mmd.version == 123`
		`+ assert generator.mmd.summary == "Some testmodule summary"`
		`+`
		`+ def test_api(self):`
		`+ packages = {"foo", "bar", "baz"}`
		`+ generator = ModulemdGenerator()`
		`+ generator.add_api(packages)`
		`+ assert generator.mmd.api.rpms == packages`
		`+`
		`+ def test_filter(self):`
		`+ packages = {"foo", "bar", "baz"}`
		`+ generator = ModulemdGenerator()`
		`+ generator.add_filter(packages)`
		`+ assert generator.mmd.filter.rpms == packages`
		`+`
		`+ def test_profiles(self):`
		`+ profile_names = ["default", "debug"]`
		`+ profile_pkgs = [["pkg1", "pkg2"], ["pkg3"]]`
		`+ profiles = enumerate(zip(profile_names, profile_pkgs))`
		`+ generator = ModulemdGenerator()`
		`+ generator.add_profiles(profiles)`
		`+ assert set(generator.mmd.profiles.keys()) == set(profile_names)`
		`+ assert generator.mmd.profiles["default"].rpms == {"pkg1", "pkg2"}`
		`+ assert generator.mmd.profiles["debug"].rpms == {"pkg3"}`
		`+ assert len(generator.mmd.profiles) == 2`
		`+`
		`+ def test_add_component(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):`
		`+ chroot = self.b1.build_chroots[0]`
		`+ generator = ModulemdGenerator(config=self.config)`
		`+ generator.add_component(self.b1.package_name, self.b1, chroot,`
		`+ "A reason why package is in the module", buildorder=1)`
		`+ assert len(generator.mmd.components.rpms) == 1`
		`+`
		`+ component = generator.mmd.components.rpms[self.b1.package_name]`
		`+ assert component.buildorder == 1`
		`+ assert component.name == self.b1.package_name`
		`+ assert component.rationale == "A reason why package is in the module"`
		`+`
		`+ with patch("coprs.app.config", self.config):`
		`+ assert component.repository.startswith("http://distgiturl.org")`
		`+ assert component.repository.endswith(".git")`
		`+ assert chroot.dist_git_url.startswith(component.repository)`
		`+ assert component.ref == chroot.git_hash`
		`+`
		`+ def test_components(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):`
		`+ packages = [self.p1.name, self.p2.name, self.p3.name]`
		`+ filter_packages = [self.p1.name, self.p2.name]`
		`+ builds = [self.b1.id, self.b3.id]`
		`+ generator = ModulemdGenerator(config=self.config)`
		`+`
		`+ with patch("coprs.logic.modules_logic.ModulemdGenerator.add_component") as add_component:`
		`+ generator.add_components(packages, filter_packages, builds)`
		`+ add_component.assert_called_with(self.p2.name, self.b3, self.b3.build_chroots[-1], ANY, 1)`
		`+ assert add_component.call_count == 2`
		`+`
		`+ def test_components_different_chroots(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):`
		`+ # https://bugzilla.redhat.com/show_bug.cgi?id=1444433`
		`+ # Test that we can pass a list of builds to the add_components,`
		`+ # that have no common chroots`
		`+ packages = [self.p1.name, self.p2.name, self.p3.name]`
		`+ filter_packages = [self.p1.name, self.p2.name]`
		`+ builds = [self.b1.id, self.b3.id]`
		`+ generator = ModulemdGenerator(config=self.config)`
		`+ generator.add_components(packages, filter_packages, builds)`
		`+`
		`+ def test_add_component_none_build_chroot(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):`
		`+ # https://bugzilla.redhat.com/show_bug.cgi?id=1444433`
		`+ generator = ModulemdGenerator(config=self.config)`
		`+ generator.add_component(self.p1.name, self.b1, None, "Some reason")`
		`+`
		`+ def test_generate(self):`
		`+ generator = ModulemdGenerator("testmodule", "master", 123, "Some testmodule summary", self.config)`
		`+ generator.add_api(["foo", "bar", "baz"])`
		`+ generator.add_filter(["foo", "bar"])`
		`+ yaml.load(generator.generate())`

frostyx commented 7 years ago

There was too much logic in the view for generating modulemd yaml from web UI, so I decided to separate it.

I have few reasons for this

The logic shouldn't really be there in the first place. When it started, the logic was simpler and it was all kinda proof of concept, but times changed
I could properly test it with unit tests

Since it is properly tested now, I fixed the https://bugzilla.redhat.com/show_bug.cgi?id=1444433

clime commented 7 years ago

Is there any particular "reason" why you couldn't make the bug fix separate from refactoring? This way we cannot very well comment on the way you fixed the bug. Also, in case we would like to deploy a frontend version with the bugfix we now need to deploy it with this load of not-asked-for changes that might even introduce other bugs.

clime commented on line 58 of frontend/coprs_frontend/coprs/logic/modules_logic.py 7 years ago

Typo here. It should say "custom-1-x86_64".

frostyx commented 7 years ago

Well changing something in too-long and untested method might introduce new bugs too.

This adds no additional functionality, just splits that long function into few smaller ones so the code is much clearer now, and mainly adds bunch of tests, so it was safer to change it.

Typo here. It should say "custom-1-x86_64".

Thanks, fixed

1 new commit added

[frontend] fix typo in chroot name

7 years ago

clime commented 7 years ago

What about commentary about the bug? What did you do to fix it? That's not obvious. There's no comment even here: https://bugzilla.redhat.com/show_bug.cgi?id=1444433

frostyx commented 7 years ago

oh, sorry @clime, I didn't realize that. Sure I will be glad to describe it.

There are basically three commits

54eeab9 [frontend] use ModulemdGenerator for construing the yaml file
d27699e [frontend] fix generating module issue with git_hash on NoneType (RhBug: 1444433)
765449d [frontend] separate logic for generating module yamls

See, the first one 765449d does not actually modify anything. It just duplicates the code from coprs_general.py into ModulemdGenerator class and refactors it, so it is not one ugly method, but rather a class with several much simpler methods. This commit also adds tests for this newly created class.

Then d27699e fixes the issue within the ModulemdGenerator class. Not in the original ugly code in coprs_general.py. A root of the issue is that there was a hardcoded chroot name from which a git_hash to our distgit is taken. Since there was a "custom-1-x86_64" it was fine when you tried to generate a yaml from a project that was created by MBS. The problem occurred when you picked some project without that chroot. The problem is not with custom vs other chroots though. In the UI for generating yaml, there are listed all packages with various kinds of chroots and you don't exactly have to find a common chroot that all of them have.

My solution was to not hardcode the chroot. I just see whether the build was built in custom-1-x86_64 chroot and if yes, then use it. If not, just use any of build.build_chroots - particularly I chose the last one from the array, because the code could be more elegant. The check for custom chroot is not required, but I would like to use the custom chroot for when it is possible, so I did it.

There are two tests for the solution. It is tested that add_component method can be called with None chroot and not tracebacks (the original code would), and it is tested that you can call add_components for test builds self.b1 and self.b3 and it not tracebacks (the original code would). The special thing on those two particular builds is that they have no common chroot - ['fedora-18-x86_64'] vs ['fedora-17-x86_64', 'fedora-17-i386']. It worth noting in some comment, I will do that.

Lastly 54eeab9 throws away an original code and uses ModulemdGenerator

1 new commit added

[frontend] describe the purpose of test_components_different_chroots

7 years ago

clime commented 7 years ago

Alright, thanks for the description. I would perhaps prefer if there was one PR that fixes the bug and another one that fixes the bug and refactors (both with the same commit for the bug fix so that they can be merged one after another easily). You at least separated the bug fix into a different commit. It would be better if that commit came before refactoring and if it was pointed out in the description but okay, these are just things that hugely simplify and speed up the review process.

Anyway, LGTM.

frostyx commented 6 years ago

With @clime's blessing, I am going to merge this.

Pull-Request has been merged by frostyx

6 years ago