#2205 Don't duplicate Package model for each CoprDir
Merged 2 years ago by praiskup. Opened 2 years ago by praiskup.

@@ -18,14 +18,14 @@

  keys = root,sqlalchemy,alembic

  

  [handlers]

- keys = console

+ keys = console,alembicfile

  

  [formatters]

- keys = generic

+ keys = generic,logfile

  

  [logger_root]

- level = WARN

- handlers = console

+ level = DEBUG

+ handlers = console,alembicfile

  qualname =

  

  [logger_sqlalchemy]
@@ -41,9 +41,18 @@

  [handler_console]

  class = StreamHandler

  args = (sys.stderr,)

- level = NOTSET

+ level = INFO

  formatter = generic

  

+ [handler_alembicfile]

+ class = FileHandler

+ level = DEBUG

+ formatter = logfile

+ args = ('/var/log/copr-frontend/alembic.log', 'a')

+ 

  [formatter_generic]

  format = %(levelname)-5.5s [%(name)s] %(message)s

  datefmt = %H:%M:%S

+ 

+ [formatter_logfile]

+ format =  [%(asctime)s %(levelname)s %(name)s] %(message)s

@@ -0,0 +1,96 @@

+ """

+ deduplicate packages, issue 617

+ 

+ Revision ID: 484e28958b27

+ Revises: 50e68db97d0a

+ Create Date: 2022-05-25 10:54:34.832625

+ """

+ 

+ import logging

+ 

+ import sqlalchemy as sa

+ from alembic import op

+ 

+ revision = '484e28958b27'

+ down_revision = '50e68db97d0a'

+ 

+ log = logging.getLogger(__name__)

+ 

+ def upgrade():

+     """

+     Before we drop the Package.copr_dir_id argument, use it to "guess" the

+     "main" package within each Copr.  That is to be kept, rest of the Package

+     of the same name are to be removed.

+     """

+     session = sa.orm.sessionmaker(bind=op.get_bind())()

+ 

+     # About 9k items here, 28k packages for de-duplication.

+     duplications = session.execute(

+         """

+         select count(package.name), package.name, copr.id

+         from package

+         join copr on copr.id = package.copr_id

+         group by copr.id, package.name

+         having count(package.name) > 1;

+         """

+     )

+ 

+     for duplication in duplications:

+         _, package_name, copr_id = duplication

+ 

+         fix_package_ids_result = session.execute(

+             """

+             select package.id

+             from package

+             left join copr_dir on package.copr_dir_id = copr_dir.id

+             where package.copr_id = :copr_id and package.name = :package_name

+             order by copr_dir.main = false asc, package.id asc;

+             """,

+             {"copr_id": copr_id, "package_name": package_name},

+         )

+ 

+         fix_ids = []

+         main_package_id = None

+         for row in fix_package_ids_result:

+             package_id = row[0]

+             if main_package_id is None:

+                 main_package_id = package_id

+             else:

+                 fix_ids.append(package_id)

+ 

+         log.debug(

+             "Deduplicating %s=%s in copr_id=%s, affected packages: %s",

+             package_name, main_package_id, copr_id,

+             ", ".join([str(x) for x in fix_ids]),

+         )

+ 

+         session.execute(

+             """

+             update build set package_id = :package_id

+             where package_id in :ids;

+             """,

+             {"package_id": main_package_id, "ids": tuple(fix_ids)},

+         )

+ 

+         session.execute(

+             "delete from package where id in :ids",

+             {"ids": tuple(fix_ids)},

+         )

+ 

+     log.debug("Removing indexes and constraints")

+     op.drop_index('ix_package_copr_dir_id', table_name='package')

+     op.drop_constraint('packages_copr_dir_pkgname', 'package', type_='unique')

+     op.drop_constraint('package_copr_dir_foreign_key', 'package', type_='foreignkey')

+     op.drop_column('package', 'copr_dir_id')

+     # make this unique

+     op.drop_index('package_copr_id_name', table_name='package')

+     op.create_index('package_copr_id_name', 'package', ['copr_id', 'name'], unique=True)

+ 

+ 

+ def downgrade():

+     op.add_column('package', sa.Column('copr_dir_id', sa.INTEGER(), autoincrement=False, nullable=True))

+     op.create_foreign_key('package_copr_dir_foreign_key', 'package', 'copr_dir', ['copr_dir_id'], ['id'])

+     op.create_unique_constraint('packages_copr_dir_pkgname', 'package', ['copr_dir_id', 'name'])

+     op.create_index('ix_package_copr_dir_id', 'package', ['copr_dir_id'], unique=False)

+     op.drop_index('package_copr_id_name', table_name='package')

+     op.create_index('package_copr_id_name', 'package', ['copr_id', 'name'], unique=False)

@@ -77,11 +77,10 @@

              db.session.query(

                  func.max(models.Build.id),

              )

-             .join(models.BuildChroot)

-             .join(models.Package)

+             .join(models.BuildChroot, models.CoprDir, models.Package)

              .filter(models.BuildChroot.mock_chroot_id == mock_rawhide_chroot.id)

              .filter(models.BuildChroot.status == StatusEnum("succeeded"))

-             .filter(models.Package.copr_dir == copr.main_dir)

+             .filter(models.CoprDir.id == copr.main_dir.id)

              .group_by(models.Package.name)

          )

  

@@ -56,10 +56,6 @@

          return PackageFormPyPI

      elif source_type_text == 'rubygems':

          return PackageFormRubyGems

-     elif source_type_text == 'git_and_tito':

-         return PackageFormTito # deprecated

-     elif source_type_text == 'mock_scm':

-         return PackageFormMock # deprecated

      elif source_type_text == "custom":

          return PackageFormCustom

      elif source_type_text == "distgit":
@@ -922,84 +918,6 @@

          })

  

  

- class PackageFormTito(BasePackageForm):

-     """

-     @deprecated

-     """

-     git_url = wtforms.StringField(

-         "Git URL",

-         validators=[

-             wtforms.validators.DataRequired(),

-             wtforms.validators.URL()])

- 

-     git_directory = wtforms.StringField(

-         "Git Directory",

-         validators=[

-             wtforms.validators.Optional()])

- 

-     git_branch = wtforms.StringField(

-         "Git Branch",

-         validators=[

-             wtforms.validators.Optional()])

- 

-     tito_test = wtforms.BooleanField(default=False, false_values=FALSE_VALUES)

- 

-     @property

-     def source_json(self):

-         return json.dumps({

-             "type": 'git',

-             "clone_url": self.git_url.data,

-             "committish": self.git_branch.data,

-             "subdirectory": self.git_directory.data,

-             "spec": '',

-             "srpm_build_method": 'tito_test' if self.tito_test.data else 'tito',

-         })

- 

- 

- class PackageFormMock(BasePackageForm):

-     """

-     @deprecated

-     """

-     scm_type = wtforms.SelectField(

-         "SCM Type",

-         choices=[("git", "Git"), ("svn", "SVN")])

- 

-     scm_url = wtforms.StringField(

-         "SCM URL",

-         validators=[

-             wtforms.validators.DataRequired(),

-             wtforms.validators.URL()])

- 

-     scm_branch = wtforms.StringField(

-         "Git Branch",

-         validators=[

-             wtforms.validators.Optional()])

- 

-     scm_subdir = wtforms.StringField(

-         "Subdirectory",

-         validators=[

-             wtforms.validators.Optional()])

- 

-     spec = wtforms.StringField(

-         "Spec File",

-         validators=[

-             wtforms.validators.Optional(),

-             wtforms.validators.Regexp(

-                 r"^.+\.spec$",

-                 message="RPM spec file must end with .spec")])

- 

-     @property

-     def source_json(self):

-         return json.dumps({

-             "type": self.scm_type.data,

-             "clone_url": self.scm_url.data,

-             "committish": self.scm_branch.data,

-             "subdirectory": self.scm_subdir.data,

-             "spec": self.spec.data,

-             "srpm_build_method": 'rpkg',

-         })

- 

- 

  class PackageFormDistGit(BasePackageForm):

      """

      @deprecated
@@ -1343,22 +1261,6 @@

          return _get_build_form(active_chroots, PackageFormScm, package)

  

  

- class BuildFormTitoFactory(object):

-     """

-     @deprecated

-     """

-     def __new__(cls, active_chroots):

-         return _get_build_form(active_chroots, PackageFormTito)

- 

- 

- class BuildFormMockFactory(object):

-     """

-     @deprecated

-     """

-     def __new__(cls, active_chroots):

-         return _get_build_form(active_chroots, PackageFormMock)

- 

- 

  class BuildFormPyPIFactory(object):

      def __new__(cls, active_chroots, package=None):

          return _get_build_form(active_chroots, PackageFormPyPI, package)

@@ -777,7 +777,7 @@

                  raise MalformedArgumentException(

                      "No chroots selected (or automatically selectable) for "

                      "the package \"%s\" in \"%s\""

-                     % (package.name, package.copr_dir.name))

+                     % (package.name, package.copr.name))

  

          elif chroots is None:

              # when package is unknown, we assign the chroots later when the
@@ -968,11 +968,11 @@

          pkg_name = upd_dict.get('pkg_name', None)

          if not build.package and pkg_name:

              # assign the package if it isn't already

-             if not PackagesLogic.get(build.copr_dir.id, pkg_name).first():

+             if not PackagesLogic.get(build.copr.id, pkg_name).first():

                  # create the package if it doesn't exist

                  try:

                      package = PackagesLogic.add(

-                         build.copr.user, build.copr_dir,

+                         build.copr.user, build.copr,

                          pkg_name, build.source_type, build.source_json)

                      db.session.add(package)

                      db.session.commit()
@@ -980,7 +980,7 @@

                      app.logger.exception(e)

                      db.session.rollback()

                      return

-             build.package = PackagesLogic.get(build.copr_dir.id, pkg_name).first()

+             build.package = PackagesLogic.get(build.copr.id, pkg_name).first()

  

          for attr in ["built_packages", "srpm_url", "pkg_version"]:

              value = upd_dict.get(attr, None)
@@ -1626,7 +1626,7 @@

                  BuildChroots (can span multiple Builds!) for the given package

          """

  

-         packages_query = PackagesLogic.get_all_ordered(copr.main_dir.id)

+         packages_query = PackagesLogic.get_all_ordered(copr.id)

          packages_count = packages_query.count()

  

          if packages_count > paginate_if_more_than:

@@ -97,7 +97,7 @@

          srpm_builds_src = []

          srpm_builds_dst = []

  

-         for package in copr.main_dir.packages:

+         for package in copr.packages:

              fpackage = forking.fork_package(package, fcopr)

  

              builds = PackagesLogic.last_successful_build_chroots(package)
@@ -217,13 +217,13 @@

                  message="Package {} does not exist.".format(package_id))

  

      @staticmethod

-     def get_package_safe(copr_dir, package_name):

+     def get_package_safe(copr, package_name):

          try:

-             return PackagesLogic.get(copr_dir.id, package_name).one()

+             return PackagesLogic.get(copr.id, package_name).one()

          except sqlalchemy.orm.exc.NoResultFound:

              raise ObjectNotFound(

-                 message="Package {} in the copr_dir {} does not exist."

-                 .format(package_name, copr_dir))

+                 message="Package {} in the copr {} does not exist."

+                 .format(package_name, copr))

  

      @staticmethod

      def get_group_by_name_safe(group_name):
@@ -343,11 +343,10 @@

          return fcopr

  

      def fork_package(self, package, fcopr):

-         fpackage = PackagesLogic.get(fcopr.main_dir.id, package.name).first()

+         fpackage = PackagesLogic.get(fcopr.id, package.name).first()

          if not fpackage:

-             fpackage = self.create_object(models.Package, package, exclude=["id", "copr_id", "copr_dir_id", "webhook_rebuild"])

+             fpackage = self.create_object(models.Package, package, exclude=["id", "copr_id", "webhook_rebuild"])

              fpackage.copr = fcopr

-             fpackage.copr_dir = fcopr.main_dir

              db.session.add(fpackage)

          return fpackage

  

@@ -25,34 +25,29 @@

          return models.Package.query.filter(models.Package.id == package_id)

  

      @classmethod

-     def get_all(cls, copr_dir_id):

+     def get_all(cls, copr_id):

          """

-         Get all packages assigned to 'copr_dir_id'

+         Get all packages assigned to given project ID.

          """

          return (models.Package.query

-                 .filter(models.Package.copr_dir_id == copr_dir_id))

+                 .filter(models.Package.copr_id == copr_id))

  

      @classmethod

-     def get_all_ordered(cls, copr_dir_id):

+     def get_all_ordered(cls, copr_id):

          """

-         Get all packages in 'copr_dir_id' ordered by package name

+         Get all packages in given project ID.

          """

-         return cls.get_all(copr_dir_id).order_by(models.Package.name)

- 

-     @classmethod

-     def get_all_in_copr(cls, copr_id):

-         return (models.Package.query

-                 .filter(models.Package.copr_id == copr_id))

+         return cls.get_all(copr_id).order_by(models.Package.name)

  

      @classmethod

      def get_packages_with_latest_builds_for_dir(

-             cls, copr_dir_id, small_build=True, packages=None):

+             cls, copr_dir, small_build=True, packages=None):

          """

-         Obtain the list of package objects for given copr_dir_id, with the

+         Obtain the list of package objects for given copr_dir, with the

          latest build assigned.

          Parameters:

  

-         :param copr_dir_id: CoprDir ID (int)

+         :param copr_dir: CoprDir ID (int)

          :param small_build: Don't assign full Build objects, but only a limited

              objects with necessary info.

          :param packages: Don't query the list of Package objects from DB, but
@@ -60,13 +55,16 @@

          :return: array of Package objects, with assigned latest Build object

          """

          if packages is None:

-             packages = cls.get_all_ordered(copr_dir_id).all()

+             packages = cls.get_all_ordered(copr_dir.copr.id).all()

  

          pkg_ids = [package.id for package in packages]

-         builds_ids = models.Build.query \

-             .filter(models.Build.package_id.in_(pkg_ids)) \

-             .with_entities(func.max(models.Build.id)) \

+         builds_ids = (

+             models.Build.query.join(models.CoprDir)

+             .filter(models.Build.package_id.in_(pkg_ids))

+             .filter(models.CoprDir.id==copr_dir.id)

+             .with_entities(func.max(models.Build.id))

              .group_by(models.Build.package_id)

+         )

  

          # map package.id => package object in packages array

          packages_map = {package.id: package for package in packages}
@@ -104,8 +102,8 @@

                                             models.Package.name == package_name)

  

      @classmethod

-     def get(cls, copr_dir_id, package_name):

-         return models.Package.query.filter(models.Package.copr_dir_id == copr_dir_id,

+     def get(cls, copr_id, package_name):

+         return models.Package.query.filter(models.Package.copr_id == copr_id,

                                             models.Package.name == package_name)

  

      @classmethod
@@ -186,20 +184,19 @@

          return False

  

      @classmethod

-     def add(cls, user, copr_dir, package_name, source_type=helpers.BuildSourceEnum("unset"), source_json=json.dumps({})):

+     def add(cls, user, copr, package_name, source_type=helpers.BuildSourceEnum("unset"), source_json=json.dumps({})):

          users_logic.UsersLogic.raise_if_cant_build_in_copr(

-             user, copr_dir.copr,

+             user, copr,

              "You don't have permissions to build in this copr.")

  

-         if cls.exists(copr_dir.id, package_name).all():

+         if cls.exists(copr, package_name).all():

              raise exceptions.DuplicateException(

-                 "Project dir {} already has a package '{}'"

-                 .format(copr_dir.full_name, package_name))

+                 "Project {} already has a package '{}'"

+                 .format(copr.full_name, package_name))

  

          package = models.Package(

              name=package_name,

-             copr=copr_dir.copr,

-             copr_dir=copr_dir,

+             copr=copr,

              source_type=source_type,

              source_json=source_json,

          )
@@ -208,9 +205,9 @@

          return package

  

      @classmethod

-     def exists(cls, copr_dir_id, package_name):

+     def exists(cls, copr, package_name):

          return (models.Package.query

-                 .filter(models.Package.copr_dir_id == copr_dir_id)

+                 .filter(models.Package.copr_id == copr.id)

                  .filter(models.Package.name == package_name))

  

  

@@ -786,16 +786,10 @@

      """

  

      __table_args__ = (

-         db.UniqueConstraint('copr_dir_id', 'name', name='packages_copr_dir_pkgname'),

-         db.Index('package_copr_id_name', 'copr_id', 'name'),

+         db.Index('package_copr_id_name', 'copr_id', 'name', unique=True),

          db.Index('package_webhook_sourcetype', 'webhook_rebuild', 'source_type'),

      )

  

-     def __init__(self, *args, **kwargs):

-         if kwargs.get('copr') and not kwargs.get('copr_dir'):

-             kwargs['copr_dir'] = kwargs.get('copr').main_dir

-         super(Package, self).__init__(*args, **kwargs)

- 

      id = db.Column(db.Integer, primary_key=True)

      name = db.Column(db.String(100), nullable=False)

      # Source of the build: type identifier
@@ -820,16 +814,13 @@

      copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"), index=True)

      copr = db.relationship("Copr", backref=db.backref("packages"))

  

-     copr_dir_id = db.Column(db.Integer, db.ForeignKey("copr_dir.id"), index=True)

-     copr_dir = db.relationship("CoprDir", backref=db.backref("packages"))

- 

      # comma-separated list of wildcards of chroot names that this package should

      # not be built against, e.g. "fedora-*, epel-*-i386"

      chroot_denylist_raw = db.Column(db.Text)

  

      @property

      def dist_git_repo(self):

-         return "{}/{}".format(self.copr_dir.full_name, self.name)

+         return "{}/{}".format(self.copr.full_name, self.name)

  

      @property

      def source_json_dict(self):
@@ -930,10 +921,7 @@

      def chroots(self):

          chroots = list(self.copr.active_chroots)

          if not self.chroot_denylist_raw:

-             # no specific denylist

-             if self.copr_dir.main:

-                 return chroots

-             return self.main_pkg.chroots

+             return chroots

  

          filtered = [c for c in chroots if not self.matched_chroot(c, self.chroot_denylist)]

          # We never want to filter everything, this is a misconfiguration.
@@ -2006,9 +1994,9 @@

      @property

      def distgit_clone_url(self):

          """

-         Considering self.build.copr_dir and self.build.package is correctly set,

-         return the dist git clone url for that package.  We can not use

-         self.build.package.dist_git_clone_url because of issue#617.

+         Return a CoprDir specific DistGit clone URL.  We can not just use

+         self.build.package.dist_git_clone_url because that one is not

+         CoprDir-specific.

          """

          dirname = self.build.copr_dir.full_name

          package = self.build.package.name

@@ -37,7 +37,6 @@

          </a>

          {% for copr_dir_info in copr_dirs %}

            {% set copr_dir = copr_dir_info.copr_dir %}

-           {% if copr_dir.main or (copr_dir.packages | count) > 0 %}

              <a href="{{ copr_url('coprs_ns.copr_builds', copr) }}?dirname={{ copr_dir.name }}" class="btn btn-default btn-sm {% if current_dirname == copr_dir.name %}active{% endif %}"

                  {% if copr_dir_info.removal_candidate %}

                      title="Will be removed in {{ copr_dir_info.remaining_days }} days if no other build is done here"
@@ -50,7 +49,6 @@

              >

                  {{ copr_dir.name }}

              </a>

-           {% endif %}

          {% endfor %}

        </div>

      </div>

@@ -106,13 +106,13 @@

  

      copr = get_copr(ownername, projectname)

      try:

-         package = PackagesLogic.get(copr.main_dir.id, packagename)[0]

+         package = PackagesLogic.get(copr.id, packagename)[0]

      except IndexError:

          raise ObjectNotFound("No package with name {name} in copr {copr}".format(name=packagename, copr=copr.name))

      return flask.jsonify(to_dict(package, with_latest_build, with_latest_succeeded_build))

  

  

- @apiv3_ns.route("/package/list/", methods=GET)

+ @apiv3_ns.route("/package/list", methods=GET)

  @pagination()

  @query_params()

  def get_package_list(ownername, projectname, with_latest_build=False,
@@ -122,7 +122,7 @@

      with_latest_succeeded_build = get_arg_to_bool(with_latest_succeeded_build)

  

      copr = get_copr(ownername, projectname)

-     query = PackagesLogic.get_all(copr.main_dir.id)

+     query = PackagesLogic.get_all(copr.id)

      paginator = Paginator(query, models.Package, **kwargs)

      packages = paginator.get().all()

  
@@ -135,7 +135,7 @@

      # for querying latest successfull builds, so that will be a little slower

      if with_latest_build:

          packages = PackagesLogic.get_packages_with_latest_builds_for_dir(

-             copr.main_dir.id,

+             copr.main_dir,

              small_build=False,

              packages=packages)

  
@@ -150,7 +150,7 @@

      copr = get_copr(ownername, projectname)

      data = rename_fields(get_form_compatible_data(preserve=["python_versions"]))

      process_package_add_or_edit(copr, source_type_text, data=data)

-     package = PackagesLogic.get(copr.main_dir.id, package_name).first()

+     package = PackagesLogic.get(copr.id, package_name).first()

      return flask.jsonify(to_dict(package))

  

  
@@ -160,7 +160,7 @@

      copr = get_copr(ownername, projectname)

      data = rename_fields(get_form_compatible_data(preserve=["python_versions"]))

      try:

-         package = PackagesLogic.get(copr.main_dir.id, package_name)[0]

+         package = PackagesLogic.get(copr.id, package_name)[0]

          source_type_text = source_type_text or package.source_type_text

      except IndexError:

          raise ObjectNotFound("Package {name} does not exists in copr {copr}."
@@ -176,7 +176,7 @@

      copr = get_copr()

      form = forms.BasePackageForm()

      try:

-         package = PackagesLogic.get(copr.main_dir.id, form.package_name.data)[0]

+         package = PackagesLogic.get(copr.id, form.package_name.data)[0]

      except IndexError:

          raise ObjectNotFound("No package with name {name} in copr {copr}"

                               .format(name=form.package_name.data, copr=copr.name))
@@ -194,7 +194,7 @@

          preserve=["python_versions", "chroots", "exclude_chroots"]))

      form = forms.RebuildPackageFactory.create_form_cls(copr.active_chroots)(data, meta={'csrf': False})

      try:

-         package = PackagesLogic.get(copr.main_dir.id, form.package_name.data)[0]

+         package = PackagesLogic.get(copr.id, form.package_name.data)[0]

      except IndexError:

          raise ObjectNotFound("No package with name {name} in copr {copr}"

                               .format(name=form.package_name.data, copr=copr.name))
@@ -218,7 +218,7 @@

      copr = get_copr()

      form = forms.BasePackageForm()

      try:

-         package = PackagesLogic.get(copr.main_dir.id, form.package_name.data)[0]

+         package = PackagesLogic.get(copr.id, form.package_name.data)[0]

      except IndexError:

          raise ObjectNotFound("No package with name {name} in copr {copr}"

                               .format(name=form.package_name.data, copr=copr.name))
@@ -248,7 +248,7 @@

      if form.validate_on_submit():

          if not package:

              try:

-                 package = PackagesLogic.add(flask.app.g.user, copr.main_dir, form.package_name.data)

+                 package = PackagesLogic.add(flask.app.g.user, copr, form.package_name.data)

              except InsufficientRightsException:

                  raise ApiError("Insufficient permissions.")

              except DuplicateException:

@@ -34,8 +34,7 @@

  def copr_packages(copr, page=1):

      flashes = flask.session.pop('_flashes', [])

  

-     copr_dir_id = copr.main_dir.id

-     query_packages = PackagesLogic.get_all_ordered(copr_dir_id)

+     query_packages = PackagesLogic.get_all_ordered(copr.id)

  

      pagination = None

      if query_packages.count() > 1000:
@@ -46,7 +45,7 @@

  

      # Assign the latest builds to the package array set.

      packages = PackagesLogic.get_packages_with_latest_builds_for_dir(

-         copr.main_dir.id, packages=packages)

+         copr.main_dir, packages=packages)

  

      response = flask.Response(

          stream_with_context(helpers.stream_template(
@@ -63,7 +62,7 @@

  @coprs_ns.route("/g/<group_name>/<coprname>/package/<package_name>/")

  @req_with_copr

  def copr_package(copr, package_name):

-     package = ComplexLogic.get_package_safe(copr.main_dir, package_name)

+     package = ComplexLogic.get_package_safe(copr, package_name)

      return flask.render_template("coprs/detail/package.html", package=package, copr=copr)

  

  @coprs_ns.route("/<username>/<coprname>/package/<package_name>/status_image/last_build.png")
@@ -71,7 +70,7 @@

  @req_with_copr

  def copr_package_icon(copr, package_name):

      try:

-         package = ComplexLogic.get_package_safe(copr.main_dir, package_name)

+         package = ComplexLogic.get_package_safe(copr, package_name)

      except ObjectNotFound:

          return send_file("static/status_images/bad_url.png", mimetype='image/png')

  
@@ -82,14 +81,15 @@

  @coprs_ns.route("/g/<group_name>/<coprname>/packages/rebuild-all/", methods=["GET", "POST"])

  @req_with_copr

  def copr_rebuild_all_packages(copr):

+     # pylint: disable=not-callable

      form = forms.RebuildAllPackagesFormFactory(

-         copr.active_chroots, [package.name for package in copr.main_dir.packages])()

+         copr.active_chroots, [package.name for package in copr.packages])()

  

      if flask.request.method == "POST" and form.validate_on_submit():

          try:

              packages = []

              for package_name in form.packages.data:

-                 packages.append(ComplexLogic.get_package_safe(copr.main_dir, package_name))

+                 packages.append(ComplexLogic.get_package_safe(copr, package_name))

  

              PackagesLogic.batch_build(

                  flask.g.user,
@@ -119,7 +119,7 @@

  @coprs_ns.route("/g/<group_name>/<coprname>/package/<package_name>/rebuild")

  @req_with_copr

  def copr_rebuild_package(copr, package_name):

-     package = ComplexLogic.get_package_safe(copr.main_dir, package_name)

+     package = ComplexLogic.get_package_safe(copr, package_name)

      data = package.source_json_dict

  

      if package.source_type_text == "scm":
@@ -193,7 +193,7 @@

  @coprs_ns.route("/g/<group_name>/<coprname>/package/<package_name>/edit/<source_type_text>")

  @req_with_copr

  def copr_edit_package(copr, package_name, source_type_text=None, **kwargs):

-     package = ComplexLogic.get_package_safe(copr.main_dir, package_name)

+     package = ComplexLogic.get_package_safe(copr, package_name)

      data = package.source_json_dict

      data["webhook_rebuild"] = package.webhook_rebuild

      data["chroot_denylist"] = package.chroot_denylist_raw
@@ -265,7 +265,7 @@

              if package_name:

                  package = PackagesLogic.get(copr.main_dir.id, package_name)[0]

              else:

-                 package = PackagesLogic.add(flask.app.g.user, copr.main_dir, form.package_name.data)

+                 package = PackagesLogic.add(flask.app.g.user, copr, form.package_name.data)

  

              package.source_type = helpers.BuildSourceEnum(source_type_text)

              package.webhook_rebuild = form.webhook_rebuild.data

@@ -63,7 +63,7 @@

  

          package_name = kwargs.pop('package_name')

          try:

-             package = ComplexLogic.get_package_safe(copr.main_dir, package_name)

+             package = ComplexLogic.get_package_safe(copr, package_name)

          except ObjectNotFound:

              return "PACKAGE_NOT_FOUND\n", 404

  

@@ -18,7 +18,6 @@

  from coprs.logic.coprs_logic import CoprDirsLogic

  from coprs.logic.builds_logic import BuildsLogic

  from coprs.logic.complex_logic import ComplexLogic

- from coprs.logic.packages_logic import PackagesLogic

  from coprs import helpers

  

  from urllib.parse import urlparse
@@ -60,25 +59,18 @@

      def build(self, source_dict_update, copr_dir, update_callback,

                scm_object_type, scm_object_id, scm_object_url, agent_url):

  

-         if self.package.copr_dir.name != copr_dir.name:

-             package = PackagesLogic.get_or_create(copr_dir, self.package.name, self.package)

-         else:

-             package = self.package

- 

          if db.engine.url.drivername != 'sqlite':

              db.session.execute('LOCK TABLE build IN EXCLUSIVE MODE')

  

          return BuildsLogic.rebuild_package(

-             package, source_dict_update, copr_dir, update_callback,

+             self.package, source_dict_update, copr_dir, update_callback,

              scm_object_type, scm_object_id, scm_object_url, submitted_by=agent_url)

  

      @classmethod

      def get_candidates_for_rebuild(cls, clone_url):

          rows = models.Package.query \

-             .join(models.CoprDir) \

              .filter(models.Package.source_type.in_(SUPPORTED_SOURCE_TYPES)) \

              .filter(models.Package.webhook_rebuild) \

-             .filter(models.CoprDir.main) \

              .filter(models.Package.source_json.ilike('%' + clone_url + '%'))

  

          return [ScmPackage(row) for row in rows]
@@ -298,7 +290,7 @@

                      if build:

                          log.info('\t -> {}'.format(build.to_dict()))

                  except Exception as e:

-                     log.error(str(e))

+                     log.exception(str(e))

                      db.session.rollback()

                  else:

                      db.session.commit()

@@ -360,11 +360,11 @@

      @pytest.fixture

      def f_builds(self):

          self.p1 = models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=0)

+             copr=self.c1, name="hello-world", source_type=0)

          self.p2 = models.Package(

-             copr=self.c2, copr_dir=self.c2_dir, name="whatsupthere-world", source_type=0)

+             copr=self.c2, name="whatsupthere-world", source_type=0)

          self.p3 = models.Package(

-             copr=self.c3, copr_dir=self.c3_dir, name="goodbye-world", source_type=0)

+             copr=self.c3, name="goodbye-world", source_type=0)

  

          self.b1 = models.Build(

              copr=self.c1, copr_dir=self.c1_dir, package=self.p1,
@@ -416,7 +416,7 @@

      def f_fork_prepare(self, f_coprs, f_mock_chroots, f_builds):

  

          self.p4 = models.Package(

-             copr=self.c2, copr_dir=self.c2_dir, name="hello-world", source_type=0)

+             copr=self.c2, name="hello-world", source_type=0)

          self.b5 = models.Build(

              copr=self.c2, copr_dir=self.c2_dir, package=self.p4,

              user=self.u1, submitted_on=50, srpm_url="http://somesrpm",
@@ -460,7 +460,7 @@

          self.db.session.add_all([self.b5, self.b6, self.b7, self.b8])

  

          self.p5 = models.Package(

-             copr=self.c2, copr_dir=self.c2_dir, name="new-package", source_type=0)

+             copr=self.c2, name="new-package", source_type=0)

          self.b9 = models.Build(

              copr=self.c2, copr_dir=self.c2_dir, package=self.p5,

              user=self.u1, submitted_on=100, srpm_url="http://somesrpm",
@@ -531,7 +531,6 @@

          self.db.session.add(self.c1)

          self.pHook = models.Package(

              copr=self.c1,

-             copr_dir=self.c1_dir,

              name="hook-package",

              source_type=helpers.BuildSourceEnum('scm'))

  
@@ -691,9 +690,6 @@

      def f_pr_dir(self):

          self.c4_dir = models.CoprDir(name=u"foocopr:PR", copr=self.c1,

                  main=False)

-         self.p4 = models.Package(

-             copr=self.c1, copr_dir=self.c4_dir, name="hello-world",

-             source_type=0)

  

      @pytest.fixture

      def f_pr_build(self, f_mock_chroots, f_builds, f_pr_dir):

@@ -12,17 +12,8 @@

  from pagure_events import build_on_fedmsg_loop

  

  

- class _Message:

-     def __init__(self, topic, body):

-         self.topic = topic

-         self.body = body

- 

- 

- class PullRequestTrigger:

-     """ Trigger builds using "Pagure-like" message """

-     test_object = None

- 

-     data = {

+ def get_pagure_pr_event():

+     return copy.deepcopy({

          "msg": {

              "agent": "test",

              "pullrequest": {
@@ -49,7 +40,19 @@

                  },

              }

          }

-     }

+     })

+ 

+ 

+ class _Message:

+     def __init__(self, topic, body):

+         self.topic = topic

+         self.body = body

+ 

+ 

+ class PullRequestTrigger:

+     """ Trigger builds using "Pagure-like" message """

+     test_object = None

+     data = get_pagure_pr_event()

  

      def __init__(self, test_object):

          self.test_object = test_object
@@ -57,10 +60,10 @@

  

      @staticmethod

      def _get_the_package(project, pkgname):

-         return models.Package.query.join(models.CoprDir).filter(

+         return models.Package.query.filter(

              models.Package.copr_id==project.id,

              models.Package.name==pkgname,

-             models.CoprDir.main).one_or_none()

+         ).one_or_none()

  

      @mock.patch('pagure_events.get_repeatedly', mock.Mock())

      def build_package(self, project_name, pkgname, pr_id):

@@ -55,7 +55,8 @@

          """ Modify CoprChroot """

          raise NotImplementedError

  

-     def create_distgit_package(self, project, pkgname, options=None):

+     def create_distgit_package(self, project, pkgname, options=None,

+                                expected_status_code=None):

          """ Modify CoprChroot """

          raise NotImplementedError

  
@@ -120,7 +121,8 @@

              assert resp.status_code == 302

          return resp

  

-     def create_distgit_package(self, project, pkgname, options=None):

+     def create_distgit_package(self, project, pkgname, options=None,

+                                expected_status_code=None):

          if options:

              raise NotImplementedError

          data = {
@@ -131,7 +133,7 @@

              project=project,

          )

          resp = self.client.post(route, data=data)

-         assert resp.status_code == 302

+         assert resp.status_code == (expected_status_code or 302)

          return resp

  

      @staticmethod
@@ -166,7 +168,7 @@

          """ There's a button "rebuild-all" in web-UI, hit that button """

          copr = models.Copr.query.get(project_id)

          if not package_names:

-             packages = copr.main_dir.packages

+             packages = copr.packages

              package_names = [p.name for p in packages]

  

          chroots = [mch.name for mch in copr.mock_chroots]
@@ -280,7 +282,8 @@

          resp = self.post(route, data)

          return resp

  

-     def create_distgit_package(self, project, pkgname, options=None):

+     def create_distgit_package(self, project, pkgname, options=None,

+                                expected_status_code=None):

          route = "/api_3/package/add/{}/{}/{}/distgit".format(

              self.transaction_username, project, pkgname)

          data = {"package_name": pkgname}
@@ -289,13 +292,15 @@

          resp = self.post(route, data)

          return resp

  

-     def rebuild_package(self, project, pkgname, build_options=None):

+     def rebuild_package(self, project_dirname, pkgname, build_options=None):

          """ Rebuild one package in a given project using API """

+         project_name = project_dirname.split(":")[0]

          route = "/api_3/package/build"

          rebuild_data = {

              "ownername": self.transaction_username,

-             "projectname": project,

+             "projectname": project_name,

              "package_name": pkgname,

+             "project_dirname": project_dirname,

          }

          rebuild_data.update(self._form_data_from_build_options(build_options))

          return self.post(route, rebuild_data)

@@ -0,0 +1,83 @@

+ """

+ Test the custom-directory (CoprDir) functionality

+ """

+ 

+ import pytest

+ from tests.coprs_test_case import CoprsTestCase, TransactionDecorator

+ 

+ 

+ class TestCoprDir(CoprsTestCase):

+     @TransactionDecorator("u1")

+     @pytest.mark.usefixtures("f_users", "f_users_api", "f_coprs", "f_builds",

+                              "f_mock_chroots", "f_other_distgit", "f_db")

+     def test_custom_copr_dir(self):

+         # De-synchronize the CoprDir.id, Copr.id and Package.id.

+         self.web_ui.create_distgit_package("foocopr", "unused-package")

+         self.api3.rebuild_package("foocopr:unused", "unused-package")

+ 

+         self.web_ui.new_project("test", ["fedora-rawhide-i386"])

+ 

+         self.web_ui.create_distgit_package("test", "copr-cli")

+         self.web_ui.create_distgit_package("test", "copr-frontend")

+ 

+         # should fail, we can not create sub-dir through "new package" request

+         self.web_ui.create_distgit_package("test:asdf", "copr-cli",

+                                            expected_status_code=404)

+ 

+         out = self.api3.rebuild_package("test", "copr-cli")

+         build_id_1 = out.json["id"]

+         out = self.api3.rebuild_package("test:custom:subdir", "copr-frontend")

+         build_id_2 = out.json["id"]

+         self.backend.finish_build(build_id_1)

+         self.backend.finish_build(build_id_2)

+ 

+         params = {

+             "ownername": "user1",

+             "projectname": "test",

+             "project_dirname": "test:custom:subdir",

+         }

+         res = self.tc.get("/api_3/monitor", query_string=params)

+         assert len(res.json["packages"]) == 1

+         assert res.json["packages"][0]["name"] == "copr-frontend"

+ 

+         params = {

+             "ownername": "user1",

+             "projectname": "test",

+         }

+         res = self.tc.get("/api_3/monitor", query_string=params)

+         assert len(res.json["packages"]) == 1

+         assert res.json["packages"][0]["name"] == "copr-cli"

+ 

+         # TODO: The rest of this test-case is actually testing a bad behavior.

+         # The default "package/list" queries should be restricted to the main

+         # directory, but they aren't.  They are "mixed-up", see the api3 route

+         # /package/list, and how it is "dir agostic" while it still uses the

+         # "get_packages_with_latest_builds_for_dir" method.

+         params = {

+             "ownername": "user1",

+             "projectname": "test",

+             "with_latest_succeeded_build": True,

+         }

+         res = self.tc.get("/api_3/package/list", query_string=params)

+ 

+         # Two packages in this project

+         packages = res.json["items"]

+         assert len(packages) == 2

+         latest_build = packages[0]["builds"]["latest_succeeded"]

+         assert latest_build["project_dirname"] == "test"

+         latest_build = packages[1]["builds"]["latest_succeeded"]

+         assert latest_build["project_dirname"] == "test:custom:subdir"

+ 

+         params = {

+             "ownername": "user1",

+             "projectname": "test",

+             "with_latest_build": True,

+         }

+         res = self.tc.get("/api_3/package/list", query_string=params)

+ 

+         packages = res.json["items"]

+         assert len(packages) == 2

+         latest_build = packages[0]["builds"]["latest"]

+         assert latest_build["project_dirname"] == "test"

+         latest_build = packages[1]["builds"]["latest"]

+         assert latest_build["project_dirname"] == "test:custom:subdir"

@@ -34,14 +34,6 @@

          assert len(self.p1.chroot_denylist) == 2

          assert len(list(self.p1.chroots)) == 8

  

-         # non-main package inherits from main package by default

-         assert len(list(self.p4.chroots)) == 8

- 

-         # but if we set the denylist here, too, it get's precedence

-         self.p4.chroot_denylist_raw = 'epel*'

-         assert len(self.p4.chroot_denylist) == 1

-         assert len(list(self.p4.chroots)) == 10

- 

      @pytest.mark.usefixtures("f_users", "f_coprs", "f_builds",

                               "f_mock_chroots_many", "f_pr_dir", "f_db")

      def test_chroot_denylist_all(self):

@@ -3,6 +3,7 @@

  from pagure_events import (event_info_from_pr_comment, event_info_from_push,

                             event_info_from_pr, ScmPackage, build_on_fedmsg_loop,

                             TOPICS)

+ from tests.lib.pagure_pull_requests import get_pagure_pr_event

  from tests.coprs_test_case import CoprsTestCase

  

  
@@ -16,34 +17,7 @@

      def setup_method(self, method):

          super().setup_method(method)

          # pylint: disable=attribute-defined-outside-init

-         self.data = {

-             "msg": {

-                 "agent": "test",

-                 "pullrequest": {

-                     "branch": "master",

-                     "branch_from": "test_PR",

-                     "id": 1,

-                     "commit_start": "78a74b02771506daf8927b3391a669cbc32ccf10",

-                     "commit_stop": "da3d120f2ff24fa730067735c19a0b52c8bc1a44",

-                     "repo_from": {

-                         "fullname": "test/copr/copr",

-                         "url_path": "test/copr/copr",

-                     },

-                     "project": {

-                         "fullname": "test/copr/copr",

-                         "url_path": "test/copr/copr",

-                     },

-                     'status': 'Open',

-                     "comments": [],

-                     'user': {

-                         "fullname": "John Doe",

-                         "url_path": "user/jdoe",

-                         "full_url": "https://src.fedoraproject.org/user/jdoe",

-                         "name": "jdoe"

-                     },

-                 }

-             }

-         }

+         self.data = get_pagure_pr_event()

          self.base_url = "https://pagure.io/"

  

      def _setup_push_msg(self):
@@ -61,7 +35,7 @@

  

      def test_negative_is_dir_in_commit(self, f_users, f_coprs):

          self.p2 = self.models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=8, webhook_rebuild=True,

+             copr=self.c1, name="hello-world", source_type=8, webhook_rebuild=True,

              source_json='{"clone_url": "https://pagure.io/test"}'

          )

          candidates = ScmPackage.get_candidates_for_rebuild("https://pagure.io/test")
@@ -76,7 +50,7 @@

              'tests/integration/conftest.py b/tests/integration/conftest.py index '

              '1747874..a2b81f6 100644 --- a/tests/integration/conftest.py +++'}

          self.p2 = self.models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=8, webhook_rebuild=True,

+             copr=self.c1, name="hello-world", source_type=8, webhook_rebuild=True,

              source_json='{"clone_url": "https://pagure.io/test"}'

          )

          candidates = ScmPackage.get_candidates_for_rebuild("https://pagure.io/test")
@@ -94,7 +68,7 @@

              'tests/integration/conftest.py b/tests/integration/conftest.py index '

              '1747874..a2b81f6 100644 --- a/tests/integration/conftest.py +++'}

          self.p1 = self.models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=8, webhook_rebuild=True,

+             copr=self.c1, name="hello-world", source_type=8, webhook_rebuild=True,

              source_json='{"clone_url": "https://pagure.io/test/copr/copr"}'

          )

          build = build_on_fedmsg_loop()
@@ -176,7 +150,7 @@

              'tests/integration/conftest.py b/tests/integration/conftest.py index '

              '1747874..a2b81f6 100644 --- a/tests/integration/conftest.py +++'}

          self.p1 = self.models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=8, webhook_rebuild=True,

+             copr=self.c1, name="hello-world", source_type=8, webhook_rebuild=True,

              source_json='{"clone_url": "https://pagure.io/test"}'

          )

          build = build_on_fedmsg_loop()
@@ -195,7 +169,7 @@

          self._setup_push_msg()

          f_raw_commit_changes.return_value = {''}

          self.p1 = self.models.Package(

-             copr=self.c1, copr_dir=self.c1_dir, name="hello-world", source_type=8, webhook_rebuild=True,

+             copr=self.c1, name="hello-world", source_type=8, webhook_rebuild=True,

              source_json='{"clone_url": "https://pagure.io/test"}',

          )

          build = build_on_fedmsg_loop()

no initial comment

Build succeeded.

rebased onto 5510cd8f4d627042d7a1b48fdf1f04d8824bf64e

2 years ago

Build succeeded.

4 new commits added

  • frontend: don't hide CoprDir buttons in Builds web-ui
  • frontend: deduplicate the testing Pagure PR event data
  • frontend: de-duplicate Packages for each CoprDirectory
  • frontend: dump DEBUG+ log entries alembic.log
2 years ago

Build succeeded.

1 new commit added

  • fix list-packages
2 years ago

Build succeeded.

5 new commits added

  • fix list-packages
  • frontend: don't hide CoprDir buttons in Builds web-ui
  • frontend: deduplicate the testing Pagure PR event data
  • frontend: de-duplicate Packages for each CoprDirectory
  • frontend: dump DEBUG+ log entries alembic.log
2 years ago

Build succeeded.

5 new commits added

  • fix list-packages
  • frontend: don't hide CoprDir buttons in Builds web-ui
  • frontend: deduplicate the testing Pagure PR event data
  • frontend: de-duplicate Packages for each CoprDirectory
  • frontend: dump DEBUG+ log entries alembic.log
2 years ago

Build succeeded.

Thank you for the PR,

The last commit should probably be squashed into something, and also it would be probably a good idea to run beaker tests on this. But ... LGTM

Yes, beaker tests run twice (and are clean). I just wanted to add a unit test because it is really too weird the bug fixed by the last commit was not caught by unit tests.

rebased onto 3051963

2 years ago

Build succeeded.

Pull-Request has been merged by praiskup

2 years ago