PR#1356: Fix 1239/1269 - copr/copr

		`@@ -0,0 +1,120 @@`
		`+ """`
		`+ Add BuildChroot.id and CoprChroot.id`
		`+`
		`+ Revision ID: 4d06318043d3`
		`+ Revises: 6f83ea2ba416`
		`+ Create Date: 2020-04-22 13:37:56.137818`
		`+ """`
		`+`
		`+ import time`
		`+`
		`+ import sqlalchemy as sa`
		`+ from alembic import op`
		`+`
		`+ revision = '4d06318043d3'`
		`+ down_revision = '6f83ea2ba416'`
		`+`
		`+ Base = sa.ext.declarative.declarative_base()`
		`+`
		`+ FIRST_TIME = time.time()`
		`+`
		`+`
		`+ def _info_checkpoint(message):`
		`+ secs = time.time() - FIRST_TIME`
		`+ print("{:6.2f} {}".format(secs, message))`
		`+`
		`+`
		`+ def upgrade():`
		`+ _info_checkpoint("add CoprChroot.id (and index)")`
		`+ op.execute("ALTER TABLE copr_chroot ADD COLUMN id SERIAL")`
		`+ op.create_index('temporary_copr_chroot_id', 'copr_chroot', ['id'],`
		`+ unique=True)`
		`+`
		`+ _info_checkpoint("add BuildChroot.copr_chroot_id column")`
		`+ op.add_column('build_chroot',`
		`+ sa.Column('copr_chroot_id', sa.Integer(), nullable=True))`
		`+`
		`+ _info_checkpoint("creating temporary table with index")`
		`+ op.execute("""`
		`+ CREATE TEMP TABLE temporary_table AS (`
		`+ SELECT`
		`+ bch.build_id as build_id,`
		`+ bch.mock_chroot_id,`
		`+ c.id as copr_id,`
		`+ cch.id as copr_chroot_id`
		`+ FROM`
		`+ build_chroot as bch`
		`+ LEFT JOIN`
		`+ build as b on bch.build_id = b.id`
		`+ LEFT JOIN`
		`+ copr as c on c.id = b.copr_id`
		`+ LEFT JOIN`
		`+ copr_chroot as cch ON cch.copr_id = c.id AND`
		`+ cch.mock_chroot_id = bch.mock_chroot_id`
		`+ )`
		`+ """)`
		`+ op.execute("""`
		`+ CREATE INDEX temporary_index ON temporary_table (build_id, mock_chroot_id)`
		`+ """)`
		`+`
		`+ _info_checkpoint("drop constraints/indexes to speedup update")`
		`+ op.drop_constraint('build_chroot_pkey', 'build_chroot', type_='primary')`
		`+ # drop those temporarily`
		`+ op.drop_index('ix_build_chroot_build_id', table_name='build_chroot')`
		`+ op.drop_index('ix_build_chroot_started_on', table_name='build_chroot')`
		`+ op.drop_index('ix_build_chroot_ended_on', table_name='build_chroot')`
		`+ op.drop_index('build_chroot_status_started_on_idx',`
		`+ table_name='build_chroot')`
		`+`
		`+ _info_checkpoint("add BuildChroot.id")`
		`+ op.execute("ALTER TABLE build_chroot ADD COLUMN id SERIAL")`
		`+`
		`+ _info_checkpoint("starting the expensive query")`
		`+ sql_major_query = """`
		`+ UPDATE`
		`+ build_chroot`
		`+ SET`
		`+ copr_chroot_id = sq.copr_chroot_id`
		`+ FROM`
		`+ temporary_table as sq`
		`+ WHERE`
		`+ build_chroot.mock_chroot_id = sq.mock_chroot_id AND`
		`+ build_chroot.build_id = sq.build_id`
		`+ """`
		`+ op.execute(sql_major_query)`
		`+`
		`+ _info_checkpoint("creating other constraints")`
		`+ op.create_unique_constraint('copr_chroot_mock_chroot_id_copr_id_uniq',`
		`+ 'copr_chroot', ['mock_chroot_id', 'copr_id'])`
		`+`
		`+ _info_checkpoint("drop the temporary stuff")`
		`+ op.drop_table('temporary_table')`
		`+`
		`+ # those were temporarily removed`
		`+ _info_checkpoint("create temporarily removed constraints/indexes")`
		`+ op.create_index('build_chroot_status_started_on_idx', 'build_chroot',`
		`+ ['status', 'started_on'], unique=False)`
		`+ op.create_index(op.f('ix_build_chroot_build_id'), 'build_chroot',`
		`+ ['build_id'], unique=False)`
		`+ op.create_index(op.f('ix_build_chroot_started_on'), 'build_chroot',`
		`+ ['started_on'], unique=False)`
		`+ op.create_index(op.f('ix_build_chroot_ended_on'), 'build_chroot',`
		`+ ['ended_on'], unique=False)`
		`+`
		`+ _info_checkpoint("create changed indexes/constraints")`
		`+ op.create_primary_key('build_chroot_pkey', 'build_chroot', ['id'])`
		`+ op.create_unique_constraint('build_chroot_mock_chroot_id_build_id_uniq',`
		`+ 'build_chroot', ['mock_chroot_id', 'build_id'])`
		`+ op.create_index(op.f('ix_build_chroot_copr_chroot_id'), 'build_chroot',`
		`+ ['copr_chroot_id'], unique=False)`
		`+`
		`+ op.drop_constraint('copr_chroot_pkey', 'copr_chroot', type_='primary')`
		`+ op.create_primary_key('copr_chroot_pkey', 'copr_chroot', ['id'])`
		`+ op.drop_index('temporary_copr_chroot_id', table_name='copr_chroot')`
		`+ op.create_foreign_key(None, 'build_chroot', 'copr_chroot',`
		`+ ['copr_chroot_id'], ['id'], ondelete='SET NULL')`
		`+`
		`+`
		`+ def downgrade():`
		`+ """ not implemented """`
		`+ raise NotImplementedError("Sorry, this migration cannot be undone")`

frontend/coprs_frontend/alembic/versions/b0fd99505e37_fixup_unassigned_copr_chroots.py

file added

+29

		`@@ -0,0 +1,29 @@`
		`+ """`
		`+ fixup unassigned copr chroots`
		`+`
		`+ Revision ID: b0fd99505e37`
		`+ Revises: 4d06318043d3`
		`+ Create Date: 2020-04-29 08:26:46.827633`
		`+ """`
		`+`
		`+ from alembic import op`
		`+`
		`+ revision = 'b0fd99505e37'`
		`+ down_revision = '4d06318043d3'`
		`+`
		`+ def upgrade():`
		`+ # Cancel all build_chroot instances that were submitted before user dropped`
		`+ # corresponding copr_chroot, and didn't have a chance to finish.`
		`+ op.execute("""`
		`+ update build_chroot`
		`+ set status = 2`
		`+ from build where`
		`+ build_chroot.build_id = build.id and`
		`+ build_chroot.status = 4 and`
		`+ build.canceled != true and`
		`+ build_chroot.copr_chroot_id is null`
		`+ """)`
		`+`
		`+`
		`+ def downgrade():`
		`+ """ nothing needed """`

frontend/coprs_frontend/commands/rawhide_to_release.py

file modified

+6 -3

		`@@ -103,9 +103,12 @@`
		`if not chroot_exists:`
		`# forked chroot may already exists, e.g. from prevoius`
		`# 'rawhide-to-release-run'`
		`- dest_build_chroot = models.BuildChroot(**rbc.to_dict())`
		`- dest_build_chroot.mock_chroot_id = mock_chroot.id`
		`- dest_build_chroot.mock_chroot = mock_chroot`
		`+ dest_build_chroot = builds_logic.BuildChrootsLogic.new(`
		`+ copr=copr,`
		`+ build=rbc.build,`
		`+ mock_chroot=mock_chroot,`
		`+ **rbc.to_dict(),`
		`+ )`
		`dest_build_chroot.status = StatusEnum("forked")`
		`db.session.add(dest_build_chroot)`

frontend/coprs_frontend/coprs/exceptions.py

file modified

+6 -4

		`@@ -33,6 +33,12 @@`
		`_code = 400`


		`+ class ConflictingRequest(CoprHttpException):`
		`+ """ Generic DB conflict """`
		`+ _default = "Conflicting request"`
		`+ _code = 409`
		`+`
		`+`
		`class ApiError(CoprHttpException):`

		`_default = "API error"`
		`@@ -69,10 +75,6 @@`
		`InsufficientRightsException = AccessRestricted`


		`- class RequestCannotBeExecuted(CoprHttpException):`
		`- pass`
		`-`
		`-`
		`class ActionInProgressException(CoprHttpException):`

		`def __init__(self, msg, action):`

frontend/coprs_frontend/coprs/helpers.py

file modified

+22

		`@@ -609,3 +609,25 @@`
		`print(changes)`

		`return changes`
		`+`
		`+`
		`+ def pluralize(what: str, items: list, be_suffix: bool = False) -> str:`
frostyx commented 5 years ago Do we want to start type-hinting our new python3-only functions now? I think it is not a bad idea.
praiskup commented 5 years ago I wouldn't be against. At least on best-effort basis.
		`+ """`
		+ By giving ``what`` string (e.g. "build") and ``items`` array, return string
		`+ in either of those formats:`
		`+ - "builds 1, 2, 3 and 4[ are]"`
		`+ - "builds 1 and 2[ are]"`
		`+ - "build 31[ is]"`
		`+ """`
		`+ if len(items) > 1:`
		`+ return "{}s {} and {}{}".format(`
		`+ what,`
		`+ ', '.join(str(item) for item in items[:-1]),`
		`+ str(items[-1]),`
		`+ " are" if be_suffix else ""`
		`+ )`
		`+ return "{} {}{}".format(`
		`+ what,`
		`+ items[0],`
		`+ " is" if be_suffix else ""`
		`+ )`

frontend/coprs_frontend/coprs/logic/actions_logic.py

file modified

+3 -3

		`@@ -1,14 +1,15 @@`
		`import json`
		`import time`

		`+ from sqlalchemy import and_, or_`
		`+ from sqlalchemy.exc import IntegrityError`
		`+`
		`from copr_common.enums import ActionTypeEnum, BackendResultEnum`
		`from coprs import db`
		`from coprs import models`
		`from coprs import helpers`
		`from coprs import exceptions`
		`-`
		`from .helpers import get_graph_parameters`
		`- from sqlalchemy import and_, or_`

		`class ActionsLogic(object):`

		`@@ -379,7 +380,6 @@`
		`)`
		`db.session.add(cached_data)`
		`db.session.commit() # @FIXME We should not commit here`
		`- return action`
frostyx commented 5 years ago What a rookie mistake. Sorry about that. Weird that tests didn't catch it, indeed.
		`except IntegrityError: # other process already calculated the graph data and cached it`
		`db.session.rollback()`

frontend/coprs_frontend/coprs/logic/builds_logic.py

file modified

+77 -22

		`@@ -23,8 +23,14 @@`
		`from coprs import db`
		`from coprs import models`
		`from coprs import helpers`
		`- from coprs.exceptions import MalformedArgumentException, ActionInProgressException, InsufficientRightsException, \`
		`- UnrepeatableBuildException, RequestCannotBeExecuted, DuplicateException`
		`+ from coprs.exceptions import (`
		`+ ActionInProgressException,`
		`+ ConflictingRequest,`
		`+ DuplicateException,`
		`+ InsufficientRightsException,`
		`+ MalformedArgumentException,`
		`+ UnrepeatableBuildException,`
		`+ )`

		`from coprs.logic import coprs_logic`
		`from coprs.logic import users_logic`
		`@@ -276,22 +282,31 @@`

		`@classmethod`
		`def get_pending_build_tasks(cls, background=None):`
		`- query = (models.BuildChroot.query`
		`- .outerjoin(models.Build)`
		`- .outerjoin(models.CoprDir)`
		`- .outerjoin(models.Package, models.Package.id == models.Build.package_id)`
		`- .options(joinedload('build').joinedload('copr_dir'),`
		`- joinedload('build').joinedload('package'))`
		`- .filter(models.Build.canceled == false())`
		`- .filter(or_(`
		`- models.BuildChroot.status == StatusEnum("pending"),`
		`- and_(`
		`- models.BuildChroot.status == StatusEnum("running"),`
		`- models.BuildChroot.started_on < int(time.time() - 1.1 * app.config["MAX_BUILD_TIMEOUT"]),`
		`- models.BuildChroot.ended_on.is_(None)`
		`- )`
		`- ))`
		`- .order_by(models.Build.is_background.asc(), models.Build.id.asc()))`
		`+ """`
		`+ Get list of BuildChroot objects that are to be (re)processed.`
		`+ """`
		`+`
		`+ # Also add too-long running tasks, those are probably staled.`
		`+ # TODO: Is this still needed?`
		`+ restart_older = int(time.time() - 1.1 * app.config["MAX_BUILD_TIMEOUT"])`
		`+`
		`+ query = (`
		`+ models.BuildChroot.query`
		`+ .join(models.Build)`
		`+ .join(models.CoprDir)`
		`+ # TODO: BuildChroot objects should be self-standing. The thing is`
		`+ # that this is racy -- Package reference provides some build`
		`+ # configuration which can be changed in the middle of the`
		`+ # BuildChroot processing.`
		`+ .join(models.Package, models.Package.id == models.Build.package_id)`
		`+ .options(joinedload('build').joinedload('copr_dir'),`
		`+ joinedload('build').joinedload('package'))`
		`+ .filter(models.Build.canceled == false())`
		`+ .filter(or_(models.BuildChroot.status == StatusEnum("pending"),`
		`+ and_(models.BuildChroot.status == StatusEnum("running"),`
		`+ models.BuildChroot.started_on < restart_older,`
		`+ models.BuildChroot.ended_on.is_(None))))`
		`+ .order_by(models.Build.is_background.asc(), models.Build.id.asc()))`
		`if background is not None:`
		`query = query.filter(models.Build.is_background == (true() if background else false()))`
		`return query`
		`@@ -629,7 +644,7 @@`
		`git_hash = None`
		`if git_hashes:`
		`git_hash = git_hashes.get(chroot.name)`
		`- buildchroot = models.BuildChroot(`
		`+ buildchroot = BuildChrootsLogic.new(`
		`build=build,`
		`status=chroot_status,`
		`mock_chroot=chroot,`
		`@@ -674,7 +689,7 @@`

		`status = StatusEnum("waiting")`
		`for chroot in package.chroots:`
		`- buildchroot = models.BuildChroot(`
		`+ buildchroot = BuildChrootsLogic.new(`
		`build=build,`
		`status=status,`
		`mock_chroot=chroot,`
		`@@ -777,7 +792,7 @@`
		`# create the BuildChroots from Package setting, if not`
		`# already set explicitly for concrete build`
		`for chroot in build.package.chroots:`
		`- buildchroot = models.BuildChroot(`
		`+ buildchroot = BuildChrootsLogic.new(`
		`build=build,`
		`status=chroot_status,`
		`mock_chroot=chroot,`
		`@@ -903,7 +918,7 @@`
		`err_msg = "Cannot cancel build {} in state 'starting'".format(build.id)`
		`else:`
		`err_msg = "Cannot cancel build {}".format(build.id)`
		`- raise RequestCannotBeExecuted(err_msg)`
		`+ raise ConflictingRequest(err_msg)`

		`if build.status == StatusEnum("running"): # otherwise the build is just in frontend`
		`ActionsLogic.send_cancel_build(build)`
		`@@ -1069,6 +1084,25 @@`

		`class BuildChrootsLogic(object):`
		`@classmethod`
		`+ def new(cls, build, mock_chroot, **kwargs):`
		`+ """`
		`+ Create new instance of BuildChroot`
		`+ (which is not assigned to any session)`
		`+`
		`+ Each freshly created instance of BuildChroot has to be assigned to`
		`+ pre-existing Build and MockChroot, hence the mandatory arguments.`
		`+ """`
		`+ copr_chroot = coprs_logic.CoprChrootsLogic.get_by_mock_chroot_id(`
		`+ build.copr, mock_chroot.id`
		`+ ).one()`
		`+ return models.BuildChroot(`
		`+ mock_chroot=mock_chroot,`
		`+ copr_chroot=copr_chroot,`
		`+ build=build,`
		`+ **kwargs,`
		`+ )`
		`+`
		`+ @classmethod`
		`def get_by_build_id_and_name(cls, build_id, name):`
		`mc = MockChrootsLogic.get_from_name(name).one()`

		`@@ -1110,6 +1144,27 @@`
		`def filter_by_group_name(cls, query, group_name):`
		`return query.filter(models.Group.name == group_name)`

		`+ @classmethod`
		`+ def filter_by_copr_and_mock_chroot(cls, query, copr, mock_chroot):`
		`+ """`
		`+ Filter BuildChroot query so it returns only instances related to`
		`+ particular Copr and MockChroot.`
		`+ """`
		`+ return (`
		`+ query.join(models.BuildChroot.build)`
		`+ .filter(models.BuildChroot.mock_chroot_id == mock_chroot.id)`
		`+ .filter(models.Build.copr_id == copr.id)`
		`+ )`
		`+`
		`+ @classmethod`
		`+ def by_copr_and_mock_chroot(cls, copr, mock_chroot):`
		`+ """`
		`+ Given Copr and MockChroot instances, return query object which provides`
		`+ a list of related BuildChroots.`
		`+ """`
		`+ return cls.filter_by_copr_and_mock_chroot(BuildChroot.query, copr,`
		`+ mock_chroot)`
		`+`

		`class BuildsMonitorLogic(object):`
		`@classmethod`

frontend/coprs_frontend/coprs/logic/complex_logic.py

file modified

+12 -1

		`@@ -338,10 +338,21 @@`
		`db.session.flush()`

		`fbuild.result_dir = '{:08}'.format(fbuild.id)`
		`- fbuild.build_chroots = [self.create_object(models.BuildChroot, c, exclude=["id", "build_id", "result_dir"]) for c in build_chroots]`
		`+ fbuild.build_chroots = [`
		`+ self.create_object(models.BuildChroot, c,`
		`+ exclude=["id_", "build_id", "result_dir",`
		`+ "copr_chroot_id"])`
		`+ for c in build_chroots`
		`+ ]`
		`for chroot in fbuild.build_chroots:`
		`chroot.result_dir = '{:08}-{}'.format(fbuild.id, fpackage.name)`
		`chroot.status = StatusEnum("forked")`
		`+ # the CoprChroot could be disabled in project (when we fork directly`
		`+ # by fork_build(), without parent fork_copr(), hence one_or_none()`
		`+ chroot.copr_chroot = CoprChrootsLogic.get_by_mock_chroot_id(`
		`+ fcopr,`
		`+ chroot.mock_chroot_id,`
		`+ ).one_or_none()`
		`db.session.add(fbuild)`
		`return fbuild`

frontend/coprs_frontend/coprs/logic/coprs_logic.py

file modified

+61 -16

		`@@ -15,6 +15,7 @@`
		`from coprs import exceptions`
		`from coprs import helpers`
		`from coprs import models`
		`+ from coprs import logic`
		`from coprs.exceptions import MalformedArgumentException, BadRequest`
		`from coprs.logic import users_logic`
		`from coprs.whoosheers import CoprWhoosheer`
		`@@ -588,7 +589,11 @@`

		`@classmethod`
		`def mock_chroots_from_names(cls, names):`
		`-`
		`+ """`
		`+ Return a list of MockChroot objects (not a query object!) which are`
		+ named by one of the ``names`` list.
		`+ """`
		`+ # TODO: this should be moved to MockChrootsLogic`
		`db_chroots = models.MockChroot.query.all()`
		`mock_chroots = []`
		`for ch in db_chroots:`
		`@@ -598,14 +603,20 @@`
		`return mock_chroots`

		`@classmethod`
		`- def get_by_name(cls, copr, chroot_name, active_only=True):`
		`- mc = MockChrootsLogic.get_from_name(chroot_name, active_only=active_only).one()`
		`- query = (`
		`- models.CoprChroot.query.join(models.MockChroot)`
		`+ def get_by_mock_chroot_id(cls, copr, mock_chroot_id):`
		`+ """`
		`+ Query CoprChroot(s) in Copr with MockChroot.id`
		`+ """`
		`+ return (`
		`+ models.CoprChroot.query`
		`.filter(models.CoprChroot.copr_id == copr.id)`
		`- .filter(models.MockChroot.id == mc.id)`
		`+ .filter(models.CoprChroot.mock_chroot_id == mock_chroot_id)`
		`)`
		`- return query`
		`+`
		`+ @classmethod`
		`+ def get_by_name(cls, copr, chroot_name, active_only=True):`
		`+ mc = MockChrootsLogic.get_from_name(chroot_name, active_only=active_only).one()`
		`+ return cls.get_by_mock_chroot_id(copr, mc.id)`

		`@classmethod`
		`def get_by_name_safe(cls, copr, chroot_name):`
		`@@ -649,6 +660,12 @@`
		`chroot = models.CoprChroot(copr=copr, mock_chroot=mock_chroot)`
		`cls._update_chroot(buildroot_pkgs, repos, comps, comps_name, chroot,`
		`with_opts, without_opts, delete_after, delete_notify, module_toggle)`
		`+`
		`+ # reassign old build_chroots, if the chroot is re-created`
		`+ get_old = logic.builds_logic.BuildChrootsLogic.by_copr_and_mock_chroot`
		`+ for old_bch in get_old(copr, mock_chroot):`
		`+ old_bch.copr_chroot = chroot`
		`+`
		`return chroot`

		`@classmethod`
		`@@ -699,36 +716,64 @@`

		`@classmethod`
		`def update_from_names(cls, user, copr, names):`
		`+ """`
		+ Update list of CoprChroots assigned to ``copr`` from chroot ``names``
		+ array. The chroots not present in ``names`` are disabled.
		`+`
		`+ :param user: The user who does the change.`
		`+ :type user: models.User`
		`+ """`
		`+`
		`UsersLogic.raise_if_cant_update_copr(`
		`user, copr,`
		`"Only owners and admins may update their projects.")`
		`- current_chroots = copr.mock_chroots`
		`- new_chroots = cls.mock_chroots_from_names(names)`
		`+`
		`+ current_copr_chroots = copr.copr_chroots`
		`+ chroot_map = {cch.mock_chroot: cch for cch in current_copr_chroots}`
		`+ new_mock_chroots = cls.mock_chroots_from_names(names)`
		`+`
		`# add non-existing`
		`run_createrepo = False`
		`- for mock_chroot in new_chroots:`
		`- if mock_chroot not in current_chroots:`
		`- db.session.add(`
		`- models.CoprChroot(copr=copr, mock_chroot=mock_chroot))`
		`+ for mock_chroot in new_mock_chroots:`
		`+ if mock_chroot not in chroot_map:`
		`+ db.session.add(CoprChrootsLogic.create_chroot(`
		`+ user=user,`
		`+ copr=copr,`
		`+ mock_chroot=mock_chroot,`
		`+ ))`
		`run_createrepo = True`

		`if run_createrepo:`
		`ActionsLogic.send_createrepo(copr)`

		`- # delete no more present`
		`to_remove = []`
		`- for mock_chroot in current_chroots:`
		`- if mock_chroot in new_chroots:`
		`+ for mock_chroot in chroot_map:`
		`+ if mock_chroot in new_mock_chroots:`
		`continue`
		`if not mock_chroot.is_active:`
		`+ # we don't remove EOLed variants here`
		`continue`
		`# can't delete here, it would change current_chroots and break`
		`# iteration`
		`to_remove.append(mock_chroot)`

		`+ running_builds = set()`
		`for mc in to_remove:`
		`+ for bch in chroot_map[mc].build_chroots:`
		`+ if not bch.finished:`
		`+ running_builds.add(bch.build_id)`
		`+ continue`
		`copr.mock_chroots.remove(mc)`

		`+ # reject the request when some build_chroots are not yet finished`
		`+ if running_builds:`
		`+ raise exceptions.ConflictingRequest(`
		`+ "Can't drop chroot from project, related "`
		`+ "{} still in progress".format(`
		`+ helpers.pluralize("build", list(running_builds),`
		`+ be_suffix=True)))`
		`+`
		`+`
		`@classmethod`
		`def remove_comps(cls, user, copr_chroot):`
		`UsersLogic.raise_if_cant_update_copr(`

frontend/coprs_frontend/coprs/models.py

file modified

+66 -9

		`@@ -1110,6 +1110,22 @@`
		`return self.source_status == StatusEnum("succeeded")`

		`@property`
		`+ def finished_early(self):`
		`+ """`
		`+ Check if the build has finished, and if that happened prematurely`
		`+ because:`
		`+ - it was canceled`
		`+ - it failed to generate/download sources).`
		`+ That said, whether it's clear that the build has finished and we don't`
		`+ have to do additional SQL query to check corresponding BuildChroots.`
		`+ """`
		`+ if self.canceled:`
		`+ return True`
		`+ if self.source_status in [StatusEnum("failed"), StatusEnum("canceled")]:`
		`+ return True`
		`+ return False`
		`+`
		`+ @property`
		`def finished(self):`
		`"""`
		`Find out if this build is in finished state.`
		`@@ -1117,7 +1133,7 @@`
		`Build is finished only if all its build_chroots are in finished state or`
		`the build was canceled.`
		`"""`
		`- if self.canceled:`
		`+ if self.finished_early:`
		`return True`
		`if not self.build_chroots:`
		`return StatusEnum(self.source_status) in helpers.FINISHED_STATUSES`
		`@@ -1286,16 +1302,36 @@`

		`class CoprChroot(db.Model, helpers.Serializer):`
		`"""`
		`- Representation of Copr<->MockChroot relation`
		`+ Representation of Copr<->MockChroot M:N relation.`
		`+`
		`+ This table basically determines what chroots are enabled in what projects.`
		`+ But it also contains configuration for assigned Copr/MockChroot pairs.`
		`+`
		`+ We create/delete instances of this class when user enables/disables the`
		`+ chroots in his project. That said, we don't keep history of changes here`
		`+ which means that there's only one configuration at any time.`
		`"""`

		`+ id_ = db.Column('id', db.Integer, primary_key=True)`
		`+`
		`+ __table_args__ = (`
		`+ # For now we don't allow adding multiple CoprChroots having the same`
		`+ # assigned MockChroot into the same project, but we could allow this`
		`+ # in future (e.g. two chroots for 'fedora-rawhide-x86_64', both with`
		`+ # slightly different configuration).`
		`+ db.UniqueConstraint("mock_chroot_id", "copr_id",`
		`+ name="copr_chroot_mock_chroot_id_copr_id_uniq"),`
		`+ )`
		`+`
		`+ copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"))`
		`+`
		`buildroot_pkgs = db.Column(db.Text)`
		`repos = db.Column(db.Text, default="", server_default="", nullable=False)`
		`- mock_chroot_id = db.Column(`
		`- db.Integer, db.ForeignKey("mock_chroot.id"), primary_key=True)`
		`+ mock_chroot_id = db.Column(db.Integer, db.ForeignKey("mock_chroot.id"),`
		`+ nullable=False)`
		`mock_chroot = db.relationship(`
		`"MockChroot", backref=db.backref("copr_chroots"))`
		`- copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"), primary_key=True,`
		`+ copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"), nullable=False,`
		`index=True)`
		`copr = db.relationship("Copr",`
		`backref=db.backref(`
		`@@ -1383,13 +1419,32 @@`
		`Representation of Build<->MockChroot relation`
		`"""`

		`- __table_args__ = (db.Index('build_chroot_status_started_on_idx', "status", "started_on"),)`
		`+ __table_args__ = (`
		`+ db.Index("build_chroot_status_started_on_idx", "status", "started_on"),`
		`+ db.UniqueConstraint("mock_chroot_id", "build_id",`
		`+ name="build_chroot_mock_chroot_id_build_id_uniq"),`
		`+ )`
		`+`
		`+ id_ = db.Column('id', db.Integer, primary_key=True)`

		`+ # The copr_chrot field needs to be nullable because we don't remove`
		`+ # BuildChroot when we delete CoprChroot.`
		`+ copr_chroot_id = db.Column(`
		`+ db.Integer,`
		`+ db.ForeignKey("copr_chroot.id", ondelete="SET NULL"),`
		`+ nullable=True, index=True,`
		`+ )`
		`+ copr_chroot = db.relationship("CoprChroot",`
		`+ backref=db.backref("build_chroots"))`
		`+`
		`+ # The mock_chroot reference is not redundant! We need it because reference`
		`+ # through copr_chroot.mock_chroot is disposable.`
		`mock_chroot_id = db.Column(db.Integer, db.ForeignKey("mock_chroot.id"),`
		`- primary_key=True)`
		`+ nullable=False)`
		`mock_chroot = db.relationship("MockChroot", backref=db.backref("builds"))`
		`- build_id = db.Column(db.Integer, db.ForeignKey("build.id", ondelete="CASCADE"),`
		`- primary_key=True, index=True)`
		`+ build_id = db.Column(db.Integer,`
		`+ db.ForeignKey("build.id", ondelete="CASCADE"),`
		`+ index=True, nullable=False)`
		`build = db.relationship("Build", backref=db.backref("build_chroots", cascade="all, delete-orphan",`
		`passive_deletes=True))`
		`git_hash = db.Column(db.String(40))`
		`@@ -1421,6 +1476,8 @@`

		`@property`
		`def finished(self):`
		`+ if self.build.finished_early:`
		`+ return True`
		`return self.state in helpers.FINISHED_STATUSES`

		`@property`

frontend/coprs_frontend/coprs/rest_api/resources/build.py

file modified

+6 -2

		`@@ -5,7 +5,11 @@`
		`from flask_restful import Resource`

		`from ... import db`
		`- from ...exceptions import ActionInProgressException, InsufficientRightsException, RequestCannotBeExecuted`
		`+ from ...exceptions import (`
		`+ ActionInProgressException,`
		`+ ConflictingRequest,`
		`+ InsufficientRightsException,`
		`+ )`
		`from ...logic.builds_logic import BuildsLogic`
		`from ..common import get_project_safe`
		`from ..exceptions import MalformedRequest, CannotProcessRequest, AccessForbidden`
		`@@ -212,7 +216,7 @@`
		`if not build.canceled and build_dict["state"] == "canceled":`
		`BuildsLogic.cancel_build(flask.g.user, build)`
		`db.session.commit()`
		`- except (RequestCannotBeExecuted, ActionInProgressException) as err:`
		`+ except (ActionInProgressException, ConflictingRequest) as err:`
		`db.session.rollback()`
		`raise CannotProcessRequest("Cannot update build due to: {}"`
		`.format(err))`

frontend/coprs_frontend/coprs/rest_api/resources/project_chroot.py

file modified

+1 -1

		`@@ -51,8 +51,8 @@`
		`except (MalformedArgumentException, ObjectNotFoundError) as err:`
		`raise MalformedRequest("Bad mock chroot name: {}. Error: {}".format(name, err))`

		`- CoprChrootsLogic.create_chroot(flask.g.user, project, mock_chroot, **req)`
		`try:`
		`+ CoprChrootsLogic.create_chroot(flask.g.user, project, mock_chroot, **req)`
		`db.session.commit()`
		`except IntegrityError as err:`
		`# assuming conflict with existing chroot`

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

file modified

+5 -3

		`@@ -484,13 +484,15 @@`
		`copr.auto_prune = form.auto_prune.data`
		`else:`
		`copr.auto_prune = True`
		`- coprs_logic.CoprChrootsLogic.update_from_names(`
		`- flask.g.user, copr, form.selected_chroots)`
		`+`
		`try:`
		`+ coprs_logic.CoprChrootsLogic.update_from_names(`
		`+ flask.g.user, copr, form.selected_chroots)`
		`# form validation checks for duplicates`
		`coprs_logic.CoprsLogic.update(flask.g.user, copr)`
		`except (exceptions.ActionInProgressException,`
		`- exceptions.InsufficientRightsException) as e:`
		`+ exceptions.InsufficientRightsException,`
		`+ exceptions.ConflictingRequest) as e:`

		`flask.flash(str(e), "error")`
		`db.session.rollback()`

frontend/coprs_frontend/tests/coprs_test_case.py

file modified

+27 -1

		`@@ -16,7 +16,7 @@`
		`from coprs import helpers`
		`from coprs import models`
		`from coprs import cache`
		`- from coprs.logic.coprs_logic import BranchesLogic`
		`+ from coprs.logic.coprs_logic import BranchesLogic, CoprChrootsLogic`
		`from coprs.logic.dist_git_logic import DistGitLogic`

		`from unittest import mock`
		`@@ -708,6 +708,32 @@`
		`)`
		`self.db.session.add_all([self.dg1, self.dg2])`

		`+ @pytest.fixture`
		`+ def f_copr_chroots_assigned(self, f_users, f_coprs, f_mock_chroots,`
		`+ f_builds):`
		`+ """`
		`+ Make sure that users/coprs/builds are created, and that some build`
		`+ chroots have correcponding copr_chroot instsances.`
		`+ """`
		`+ _side_effects = (self, f_users, f_coprs, f_mock_chroots, f_builds)`
		`+ for bch in models.BuildChroot.query.all():`
		`+ bch.copr_chroot = CoprChrootsLogic.get_by_mock_chroot_id(`
		`+ bch.build.copr,`
		`+ bch.mock_chroot_id,`
		`+ ).one_or_none()`
		`+`
		`+ @pytest.fixture`
		`+ def f_copr_chroots_assigned_finished(self, f_copr_chroots_assigned):`
		`+ """`
		`+ Same as f_copr_chroots_assigned, but _some_ BuildChroots are flipped`
		`+ from non-finished statuses to finished.`
		`+ """`
		`+ _side_effects = (self, f_copr_chroots_assigned)`
		`+ for bch in self.b3_bc:`
		`+ bch.status = StatusEnum("failed")`
		`+ for bch in self.b4_bc:`
		`+ bch.status = StatusEnum("succeeded")`
		`+`
		`def request_rest_api_with_auth(self, url,`
		`login=None, token=None,`
		`content=None, method="GET",`

frontend/coprs_frontend/tests/test_api/test_project_chroot_r.py

file modified

-1

		`@@ -290,7 +290,6 @@`
		`}`
		`data.update(base_data)`
		`self.db.session.commit()`
		`- self.db.session.commit()`
		`r0 = self.tc.get("/api_2/projects/1/chroots")`
		`assert r0.status_code == 200`
		`assert len(json.loads(r0.data.decode("utf-8"))["chroots"]) == 1`

frontend/coprs_frontend/tests/test_helpers.py

file modified

+15 -1

		`@@ -1,13 +1,15 @@`
		`import os`
		`from copy import deepcopy`
		`from unittest import mock`
		`+`
		`from flask import Flask`
		`+ import pytest`

		`from coprs import app`
		`from coprs.helpers import parse_package_name, generate_repo_url, \`
		`fix_protocol_for_frontend, fix_protocol_for_backend, pre_process_repo_url, \`
		`parse_repo_params, pagure_html_diff_changed, SubdirMatch, \`
		`- raw_commit_changes, WorkList`
		`+ raw_commit_changes, WorkList, pluralize`

		`from tests.coprs_test_case import CoprsTestCase`

		`@@ -255,3 +257,15 @@`
		`assert _get_list(task_a) == ["a", "c", "b"]`
		`assert _get_list(task_b) == ["b", "a", "c"]`
		`assert _get_list(task_c) == ["c", "b", "a"]`
		`+`
		`+`
		`+ def test_pluralize():`
		`+ """ test generic I/O for helpers.pluralize """`
		`+ # we don't explicitly re-order`
		`+ assert pluralize("build", [2, 1, 3], be_suffix=True) == "builds 2, 1 and 3 are"`
		`+ assert pluralize("action", [1], be_suffix=True) == "action 1 is"`
		`+ assert pluralize("sth", [1, 2], be_suffix=False) == "sths 1 and 2"`
		`+ assert pluralize("a", [2], be_suffix=False) == "a 2"`
		`+`
		`+ with pytest.raises(IndexError):`
		`+ pluralize("a", [])`

frontend/coprs_frontend/tests/test_logic/test_complex_logic.py

file modified

+29

		`@@ -2,10 +2,13 @@`
		`import json`
		`from unittest import mock`

		`+ import pytest`
		`+`
		`from coprs import models, helpers, app`
		`from copr_common.enums import ActionTypeEnum`
		`from coprs.logic.actions_logic import ActionsLogic`
		`from coprs.logic.complex_logic import ComplexLogic, ProjectForking`
		`+ from coprs.logic.coprs_logic import CoprChrootsLogic`
		`from tests.coprs_test_case import CoprsTestCase, new_app_context`


		`@@ -112,6 +115,32 @@`
		`assert ch.ended_on == fch.ended_on`
		`assert ch.mock_chroot_id == fch.mock_chroot_id`

		`+ @pytest.mark.usefixtures("f_copr_chroots_assigned_finished")`
		`+ def test_fork_check_assigned_copr_chroot(self):`
		`+ """`
		`+ When old build with old set of CoprChroots is forked, only the`
		`+ "still-enabled" copr_chroots get the new forked BuildChroot.`
		`+ """`
		`+ _side_effects = (self)`
		`+ assert len(self.c2.copr_chroots) == 2`
		`+ assert self.b1.copr != self.c2 # fork from different copr`
		`+`
		`+ # enable f18-x86_64, that's where b1 was build into`
		`+ new_cch = CoprChrootsLogic.create_chroot(`
		`+ user=self.u1,`
		`+ copr=self.c2,`
		`+ mock_chroot=self.b1_bc[0].mock_chroot,`
		`+ )`
		`+ self.db.session.add(new_cch)`
		`+`
		`+ forking = ProjectForking(self.u1)`
		`+ fork_b = forking.fork_build(self.b1, self.c2, self.p2,`
		`+ self.b1.build_chroots)`
		`+`
		`+ # check the forked build_chroot has assigned copr_chroot`
		`+ assert len(new_cch.build_chroots) == 1`
		`+ assert fork_b.build_chroots == new_cch.build_chroots`
		`+`
		`def test_fork_package(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):`
		`forking = ProjectForking(self.u1)`
		`fp1 = forking.fork_package(self.p1, self.c2)`

frontend/coprs_frontend/tests/test_logic/test_coprs_logic.py

file modified

+45 -2

		`@@ -6,7 +6,7 @@`

		`from sqlalchemy import desc`

		`- from copr_common.enums import ActionTypeEnum`
		`+ from copr_common.enums import ActionTypeEnum, StatusEnum`
		`from coprs import app`
		`from coprs.forms import PinnedCoprsForm, ChrootForm, ModuleEnableNameValidator`
		`from coprs.logic.actions_logic import ActionsLogic`
		`@@ -17,7 +17,10 @@`
		`from coprs import models`
		`from coprs.whoosheers import CoprWhoosheer`
		`from tests.coprs_test_case import CoprsTestCase`
		`- from coprs.exceptions import InsufficientRightsException`
		`+ from coprs.exceptions import (`
		`+ ConflictingRequest,`
		`+ InsufficientRightsException,`
		`+ )`


		`class TestCoprsLogic(CoprsTestCase):`
		`@@ -170,6 +173,46 @@`
		`self.c2.copr_chroots[0].delete_after = datetime.today() - timedelta(days=1)`
		`assert outdated.all() == [self.c2.copr_chroots[0]]`

		`+ @pytest.mark.usefixtures("f_copr_chroots_assigned")`
		`+ def test_disabling_disallowed_when_build_runs(self):`
		`+ """`
		`+ We disallow removing chroots from project when some BuildChroot(s) are`
		`+ still in progress.`
		`+ """`
		`+ chroot_names = ["fedora-17-x86_64", "fedora-17-i386"]`
		`+ assert [ch.name for ch in self.c2.copr_chroots] == chroot_names`
		`+`
		`+ with pytest.raises(ConflictingRequest) as exc:`
		`+ CoprChrootsLogic.update_from_names(self.c2.user, self.c2, ["fedora-17-x86_64"])`
		`+ assert "builds 3 and 4 are still in progress" in exc.value.message`
		`+ for bch in self.b3_bc:`
		`+ bch.status = StatusEnum("failed")`
		`+ for bch in self.b4_bc:`
		`+ bch.status = StatusEnum("succeeded")`
		`+ CoprChrootsLogic.update_from_names(self.c2.user, self.c2, ["fedora-17-x86_64"])`
		`+`
		`+ @pytest.mark.usefixtures("f_copr_chroots_assigned_finished")`
		`+ def test_chroot_reenable(self):`
		`+ """`
		`+ We re-assign old unassigned BuildChroots to newly created`
		`+ CoprChroot instances if they match the corresponding MockChroot`
		`+ """`
		`+ assert len(self.c2.copr_chroots) == 2`
		`+ assert self.mc3 in self.c2.mock_chroots`
		`+ old_copr_chroot = self.c2.copr_chroots[1]`
		`+ old_bch_ids = [bch.id_ for bch in old_copr_chroot.build_chroots]`
		`+ CoprChrootsLogic.update_from_names(self.c2.user, self.c2, ["fedora-17-x86_64"])`
		`+ assert len(self.c2.copr_chroots) == 1`
		`+ assert self.mc3 not in self.c2.mock_chroots`
		`+`
		`+ # re-enable`
		`+ CoprChrootsLogic.update_from_names(`
		`+ self.c2.user, self.c2, ["fedora-17-x86_64", "fedora-17-i386"])`
		`+`
		`+ new_copr_chroot = self.c2.copr_chroots[1]`
		`+ assert old_copr_chroot != new_copr_chroot`
		`+ assert old_bch_ids == [bch.id_ for bch in new_copr_chroot.build_chroots]`
		`+`

		`class TestPinnedCoprsLogic(CoprsTestCase):`

frontend/coprs_frontend/tests/test_models.py

file modified

+19

		`@@ -1,3 +1,5 @@`
		`+ import pytest`
		`+`
		`from copr_common.enums import StatusEnum`
		`from tests.coprs_test_case import CoprsTestCase`

		`@@ -62,9 +64,26 @@`

		`self.b1.source_status = StatusEnum("running")`
		`assert not self.b1.finished`
		`+ assert not self.b1.finished_early`

		`self.b1.source_status = StatusEnum("canceled")`
		`assert self.b1.finished`
		`+ assert self.b1.finished_early`

		`self.b1.source_status = StatusEnum("failed")`
		`assert self.b1.finished`
		`+ assert self.b1.finished_early`
		`+`
		`+ @pytest.mark.usefixtures('f_users', 'f_coprs', 'f_mock_chroots', 'f_builds',`
		`+ 'f_db')`
		`+ def test_canceled(self):`
		`+ """ test that build.cancel affects build.build_chroots[*].finished """`
		`+ bch = self.b1.build_chroots[0]`
		`+ bch.status = StatusEnum("pending")`
		`+ assert not self.b1.finished`
		`+ assert not bch.finished`
		`+ self.b1.canceled = True`
		`+ assert bch.finished`
		`+ self.b1.canceled = False`
		`+ self.b1.source_status = StatusEnum("canceled")`
		`+ assert bch.finished`

no initial comment

Metadata Update from @praiskup:
- Pull-request tagged with: wip

5 years ago

praiskup commented 5 years ago

Depends on https://pagure.io/copr/copr/pull-request/1353

praiskup commented 5 years ago

Per last test on production data:

  0.50 add CoprChroot.id (and index)
  1.15 add BuildChroot.copr_chroot_id column
  1.15 creating temporary table with index
  6.25 drop constraints/indexes to speedup update
  6.26 add BuildChroot.id
 46.22 starting the expensive query
185.86 createing other constraints
185.97 drop the temporary stuff
187.43 create temporarily removed constraints/indexes
193.67 create changed indexes/constraints

7 new commits added

frontend: don't queue build chroots with incomplete info
frontend: link BuildChroot with corresponding CoprChroot
docs: fix typos
build_aux: linter: rewrite pylint wrapper to python
build_aux: use pylint --output-format=json
frontend: sync model with migrations
frontend: drop the unnecessary split to schema/fedora

5 years ago

rebased onto 93a9e5572ec9b81339f7749861ff5be8584e9caf

5 years ago

rebased onto a750222248b55fa38e754edf3d3cecec33ef3d36

5 years ago

8 new commits added

frontend: drop duplicate commit() in test
frontend: re-assign BuidlChroots to re-enabled CoprChroot
frontend: not finished build_chroots to disallow copr_chroot removal
frontend: don't queue build chroots with incomplete info
frontend: link BuildChroot(s) with corresponding CoprChroot(s)
frontend: fixes for two weird bugs in actions_logic
frontend: sync model with migrations
frontend: drop the unnecessary split to schema/fedora

5 years ago

praiskup commented 5 years ago

I dropped WIP flag, it's ready for review - but built on top of PR#1353 (two first commits).
Please review patches one-by-one.

Metadata Update from @praiskup:
- Pull-request untagged with: wip

5 years ago

8 new commits added

frontend: drop duplicate commit() in test
frontend: re-assign BuildChroots to re-enabled CoprChroot
frontend: not finished build_chroots to disallow copr_chroot removal
frontend: don't queue build chroots with incomplete info
frontend: link BuildChroot(s) with corresponding CoprChroot(s)
frontend: fixes for two weird bugs in actions_logic
frontend: sync model with migrations
frontend: drop the unnecessary split to schema/fedora

5 years ago

rebased onto bfb3d0b49fbf76287922a39ba233c292826594d0

5 years ago

praiskup commented 5 years ago

Just a ping here, since #1353 is merged, I rebased this yesterday and this is ready for review.

praiskup commented 5 years ago

Tested on staging:

INFO  [alembic.runtime.migration] Running upgrade 6d0a02dc7de4 -> 4d06318043d3, Add BuildChroot.id and CoprChroot.id
  1.78 add CoprChroot.id (and index)
  2.33 add BuildChroot.copr_chroot_id column
  2.33 creating temporary table with index
  8.50 drop constraints/indexes to speedup update
  8.50 add BuildChroot.id
 16.75 starting the expensive query
 86.97 creating other constraints
 87.05 drop the temporary stuff
 87.05 create temporarily removed constraints/indexes
 94.93 create changed indexes/constraints
INFO  [alembic.runtime.migration] Running upgrade 4d06318043d3 -> b0fd99505e37, fixup unassigned copr chroot

frostyx commented on line 23 of frontend/coprs_frontend/coprs/logic/actions_logic.py 5 years ago

What a rookie mistake. Sorry about that. Weird that tests didn't catch it, indeed.

frostyx commented on line 31 of frontend/coprs_frontend/alembic/versions/4d06318043d3_add_buildchroot_id_and_coprchroot_id.py 5 years ago

We have a different naming convention here than it was in the previous PR#1353 (e.g. ix_build_copr_id)

frostyx commented on line 120 of frontend/coprs_frontend/alembic/versions/4d06318043d3_add_buildchroot_id_and_coprchroot_id.py 5 years ago

The upgrade function does a lot of things (which is understandable, there was a lot of workarounding required because of the reasons explained in the commit message).

But if I understand it correctly, the end goal of the migration is to add this column

op.add_column('build_chroot', sa.Column('copr_chroot_id', sa.Integer(), nullable=True))

everything else is temporary changes that are undone within the migration. In that case, implementing downgrade function should be trivial - just one simple op.drop_column, right?

In that case I would vote for implementing it. If I am mistaken, can we please have a better error than assert False. For example, a simple NotImplementedError("Sorry, this migration cannot be undone") would be just fine.

praiskup commented on line 31 of frontend/coprs_frontend/alembic/versions/4d06318043d3_add_buildchroot_id_and_coprchroot_id.py 5 years ago

this is weird :-/ either I was completely wrong in 1353, or I just made this up ... good catch, I'll do appropriate steps here

frostyx commented on line 5 of frontend/coprs_frontend/coprs/exceptions.py 5 years ago

Seems like this is a duplicate of RequestCannotBeExecuted. Do you think we can merge them together?

praiskup commented on line 120 of frontend/coprs_frontend/alembic/versions/4d06318043d3_add_buildchroot_id_and_coprchroot_id.py 5 years ago

implementing downgrade function should be trivial - just one simple op.drop_column, right?

Not really, this upgrade changes primary keys - so we'd have to fiddle with them as well in downgrade.

can we please have a better error than assert False.

Absolutely, this is just an inappropriate use of assert. Thanks for the catch.

praiskup commented on line 5 of frontend/coprs_frontend/coprs/exceptions.py 5 years ago

I wanted to return 409 error specifically, that's for conflicts.

The spelling of this exception sounds a bit more generic, but perhaps we
can drop the RequestCannotBeExecuted exception and replace it with this
one (the RequestCannotBeExecuted is raised only once in our code if I am
not wrong).

frostyx commented on line 7 of frontend/coprs_frontend/coprs/helpers.py 5 years ago

Do we want to start type-hinting our new python3-only functions now? I think it is not a bad idea.

praiskup commented 5 years ago

Let me know explicitly please once you are done with the review; I'll
update the PR then (perhaps this evening, I hope). I don't want to waste
our review cycles.

frostyx commented on line 118 of frontend/coprs_frontend/coprs/logic/coprs_logic.py 5 years ago

We have models.Build.finished boolean property. I think it will allow you to obtain the set of running_builds much more conveniently.

frostyx commented on line 5 of frontend/coprs_frontend/coprs/exceptions.py 5 years ago

but perhaps we can drop the RequestCannotBeExecuted exception and replace it with this one

That's what I meant. I grepped it and it found only few results.

frostyx commented 5 years ago

This was a hell of a PR, /me salutes

Also, this was substantially more complicated than we originally expected, shoutout to @schlupov for having a lot of patience when figuring out the initial groundwork for this PR.

Let me know explicitly please once you are done with the review; I'll
update the PR then (perhaps this evening, I hope). I don't want to waste
our review cycles.

I am done with the first review. Overall it looks good to me, I just pointed out some minor things. We can now focus only on them.

Thank you!

praiskup commented on line 118 of frontend/coprs_frontend/coprs/logic/coprs_logic.py 5 years ago

I want to have Build.id related to all BuildChroot(s) related to all
disabled CoprChroots here. Problem of Build.finished property is that
it is pretty expensive (all the related BuildChroots need to be lazy
loaded). And it isn't cheap to ask for all running builds ... (that's the
pending-jobs query....

The CoprChroot -> BuildChroot is pretty cheap query, even though there
might be many build chroots. So I could optimize this to specific query
with filter(BuildChroot.status.in_(...)) when we needed.

I don't know, perhaps you have concrete idea in head so please elaborate.

praiskup commented 5 years ago

Yeah @frostyx is right, @schlupov I'm sorry for the underestimation of
this task. We'll have a precedent for the next time so I believe it
won't happen again.

praiskup commented on line 7 of frontend/coprs_frontend/coprs/helpers.py 5 years ago

I wouldn't be against. At least on best-effort basis.

frostyx commented 5 years ago

I wasn't sure on whose side of the net the ball is right now, so I went through the comments and I want to summarize what we agreed that should be fixed

a different naming convention of indexes
dropping RequestCannotBeExecuted exception and replacing it with the new one
better error message when downgrading the 4d06318043d3 migration

praiskup commented 5 years ago

Sure, sorry ... I just didn't have proper time frame to concentrate on fixes, yet.

rebased onto b85037533b85542b7a90b36cbf8272b3cf1abe70

5 years ago

praiskup commented 5 years ago

I updated the alembic migration path so it continues from #1265 which I plan
to merge first.

praiskup commented 5 years ago

a different naming convention of indexes

That was a temporary index in the end, needed to make the migration faster - and
then dropped. I renamed it to make it clear.

dropping RequestCannotBeExecuted exception and replacing it with the new one

Ok, this deserved a new commit.

I decided to drop also the new BuildInProgressException, since the
ConflictingRequest is just enough ... but I am bit lazy to re-rebase everything
so the last commit only drops the old exception. A bit off add-remove
ping-pong, sorry. Let me know if you want me to do the work..

better error message when downgrading the 4d06318043d3 migration

I used your wording here.

PTAL

frostyx commented 5 years ago

Thank you for the update, LGTM.

I decided to drop also the new BuildInProgressException, since the
ConflictingRequest is just enough.

I am not sure about this one though. I know that it didn't define anything special and different that ConflictingRequest, but it was a good specific-enough exception for a specific scenario. It IMHO made the code clearer because it was obvious what is going on. Now you get abstract ConflictingRequest and you need to guess that it was probably caused because of some builds aren't finished yet. I vote for putting the BuildInProgressException back.

But feel free to merge as is.

praiskup commented 5 years ago

I don't know. I can put it back ... but I was thinking what is the difference between
those two usecases we discuss here (BuildInProgress vs. BuildNotCancellable).
And I came to conclusion that having two more exceptions defined, each raised
only once in our whole codebase is just overkill which complicates potential
catch specificiations.

Anyway, if you think it should be re-added, we should also invent the
BuildNotCancelableException as well :-(

frostyx commented 5 years ago

Okay, then let's go with just ConflictingRequest. We can always refactor and reinvent more specific exceptions once they are necessary.

rebased onto 45d96656f09cdaed8ffaa6244ff34e3672fbddc7

5 years ago

praiskup commented 5 years ago

thank you gor the review

rebased onto 966b377

5 years ago

praiskup commented 5 years ago

Commit 7a465a1 fixes this pull-request

Pull-Request has been merged by praiskup