#331 [frontend][backend] Pagure integration
Merged a year ago by clime. Opened a year ago by clime.

file modified
+22 -7

@@ -165,13 +165,27 @@ 

              self.log.error(traceback.format_exc())

              result.result = ActionResult.FAILURE

  

-     def handle_delete_copr_project(self):

+     def handle_delete_project(self, result):

          self.log.debug("Action delete copr")

-         project = self.data["old_value"]

-         path = os.path.normpath(self.destdir + '/' + project)

-         if os.path.exists(path):

-             self.log.info("Removing copr %s", path)

-             shutil.rmtree(path)

+         result.result = ActionResult.SUCCESS

+ 

+         ext_data = json.loads(self.data["data"])

+         ownername = ext_data["ownername"]

+         project_dirnames = ext_data["project_dirnames"]

+ 

+         if not ownername:

+             self.log.error("Received empty ownername!")

+             result.result = ActionResult.FAILURE

+             return

+ 

+         for dirname in project_dirnames:

+             if not dirname:

+                 self.log.warning("Received empty dirname!")

+                 continue

+             path = os.path.join(self.destdir, ownername, dirname)

+             if os.path.exists(path):

+                 self.log.info("Removing copr dir {}".format(path))

+                 shutil.rmtree(path)

  

      def handle_comps_update(self, result):

          self.log.debug("Action comps update")

@@ -245,6 +259,7 @@ 

  

          for chroot, builddir in chroot_builddirs.items():

              if not builddir:

+                 self.log.warning("Received empty builddir!")

                  continue

  

              chroot_path = os.path.join(self.destdir, ownername, project_dirname, chroot)

@@ -432,7 +447,7 @@ 

  

          if action_type == ActionType.DELETE:

              if self.data["object_type"] == "copr":

-                 self.handle_delete_copr_project()

+                 self.handle_delete_project(result)

              elif self.data["object_type"] == "build":

                  self.handle_delete_build()

  

@@ -1,14 +1,14 @@ 

  """generate main copr_dirs

  

  Revision ID: 3637b9daf7e4

- Revises: 887cbbd6575e

+ Revises: ac5917e5c4fe

  Create Date: 2018-06-25 23:18:56.969792

  

  """

  

  # revision identifiers, used by Alembic.

  revision = '3637b9daf7e4'

- down_revision = '887cbbd6575e'

+ down_revision = 'ac5917e5c4fe'

  

  from alembic import op

  import sqlalchemy as sa

@@ -16,9 +16,18 @@ 

  

  def upgrade():

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

-     session.execute("""INSERT INTO copr_dir (name,copr_id,main) (SELECT name,id,True FROM copr) ON CONFLICT DO NOTHING;""")

-     session.execute("""UPDATE package SET copr_dir_id=(SELECT id from copr_dir where copr_dir.copr_id=package.copr_id AND copr_dir.main = True) WHERE copr_dir_id = NULL""")

-     session.execute("""UPDATE build SET copr_dir_id=(SELECT id from copr_dir where copr_dir.copr_id=build.copr_id AND copr_dir.main = True) WHERE copr_dir_id = NULL""")

+ 

+     session.execute("""INSERT INTO copr_dir (name,copr_id,ownername,main)

+                     (SELECT copr.name,copr.id,concat('@', "group".name),True FROM copr JOIN "group" ON copr.group_id = "group".id WHERE copr.deleted=False)

+                     ON CONFLICT DO NOTHING;""")

+ 

+     session.execute("""

+                     INSERT INTO copr_dir (name,copr_id,ownername,main)

+                     (SELECT copr.name,copr.id,username,True FROM copr JOIN "user" ON copr.user_id = "user".id WHERE copr.deleted=False AND copr.group_id is NULL)

+                     ON CONFLICT DO NOTHING;""")

+ 

+     session.execute("""UPDATE package SET copr_dir_id=(SELECT id from copr_dir where copr_dir.copr_id=package.copr_id AND copr_dir.main=True) WHERE copr_dir_id is NULL""")

+     session.execute("""UPDATE build SET copr_dir_id=(SELECT id from copr_dir where copr_dir.copr_id=build.copr_id AND copr_dir.main=True) WHERE copr_dir_id is NULL""")

  

  

  def downgrade():

@@ -15,6 +15,7 @@ 

  

  

  def upgrade():

+     # new copr_dir table

      op.create_table('copr_dir',

      sa.Column('id', sa.Integer(), nullable=False),

      sa.Column('name', sa.Text(), nullable=True),

@@ -27,6 +28,8 @@ 

      op.create_index(op.f('ix_copr_dir_copr_id'), 'copr_dir', ['copr_id'], unique=False)

      op.create_index(op.f('ix_copr_dir_name'), 'copr_dir', ['name'], unique=False)

      op.create_index('only_one_main_copr_dir', 'copr_dir', ['copr_id', 'main'], unique=True, postgresql_where=sa.text(u'main = true'))

+ 

+     # scm integration properties for Build + copr_dir relation

      op.add_column(u'build', sa.Column('copr_dir_id', sa.Integer(), nullable=True))

      op.add_column(u'build', sa.Column('scm_object_id', sa.Text(), nullable=True))

      op.add_column(u'build', sa.Column('scm_object_type', sa.Text(), nullable=True))

@@ -34,9 +37,13 @@ 

      op.add_column(u'build', sa.Column('update_callback', sa.Text(), nullable=True))

      op.create_index(op.f('ix_build_copr_dir_id'), 'build', ['copr_dir_id'], unique=False)

      op.create_foreign_key('build_copr_dir_foreign_key', 'build', 'copr_dir', ['copr_dir_id'], ['id'])

+ 

+     # scm integration properties for Copr

      op.add_column(u'copr', sa.Column('scm_api_auth_json', sa.Text(), nullable=True))

      op.add_column(u'copr', sa.Column('scm_api_type', sa.Text(), nullable=True))

      op.add_column(u'copr', sa.Column('scm_repo_url', sa.Text(), nullable=True))

+ 

+     # package constraint changes + copr_dir relation

      op.add_column(u'package', sa.Column('copr_dir_id', sa.Integer(), nullable=True))

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

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

@@ -0,0 +1,28 @@ 

+ """make copr_dirs unique by ownername

+ 

+ Revision ID: ac5917e5c4fe

+ Revises: 887cbbd6575e

+ Create Date: 2018-07-19 11:45:57.228628

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = 'ac5917e5c4fe'

+ down_revision = '887cbbd6575e'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ def upgrade():

+     op.add_column('copr_dir', sa.Column('ownername', sa.Text(), nullable=False))

+     op.create_index(op.f('ix_copr_dir_ownername'), 'copr_dir', ['ownername'], unique=False)

+     op.create_unique_constraint('ownername_copr_dir_uniq', 'copr_dir', ['ownername', 'name'])

+     op.drop_constraint(u'copr_dir_copr_id_name_uniq', 'copr_dir', type_='unique')

+ 

+ 

+ def downgrade():

+     op.create_unique_constraint(u'copr_dir_copr_id_name_uniq', 'copr_dir', ['copr_id', 'name'])

+     op.drop_constraint('ownername_copr_dir_uniq', 'copr_dir', type_='unique')

+     op.drop_index(op.f('ix_copr_dir_ownername'), table_name='copr_dir')

+     op.drop_column('copr_dir', 'ownername')

@@ -657,10 +657,14 @@ 

  

  def trim_git_url(url):

      if not url:

-         return False

+         return None

+ 

      return re.sub(r'(\.git)?/*$', '', url)

  

  

  def get_parsed_git_url(url):

+     if not url:

+         return False

+ 

      url = trim_git_url(url)

      return urlparse(url)

@@ -89,6 +89,19 @@ 

          db.session.add(action)

  

      @classmethod

+     def send_delete_copr(cls, copr):

+         data_dict = {

+             "ownername": copr.owner_name,

+             "project_dirnames": [copr_dir.name for copr_dir in copr.dirs],

+         }

+         action = models.Action(action_type=helpers.ActionTypeEnum("delete"),

+                                object_type="copr",

+                                object_id=copr.id,

+                                data=json.dumps(data_dict),

+                                created_on=int(time.time()))

+         db.session.add(action)

+ 

+     @classmethod

      def send_delete_build(cls, build):

          """

          Schedules build delete action

@@ -329,7 +329,7 @@ 

          db.engine.execute(order_to_status)

  

      @classmethod

-     def get_copr_builds_list(cls, copr, dirname):

+     def get_copr_builds_list(cls, copr, dirname=''):

          query_select = """

  SELECT build.id, build.source_status, MAX(package.name) AS pkg_name, build.pkg_version, build.submitted_on,

      MIN(statuses.started_on) AS started_on, MAX(statuses.ended_on) AS ended_on, order_to_status(MIN(statuses.st)) AS status,

@@ -842,6 +842,7 @@ 

              if new_status == StatusEnum("failed"):

                  build.fail_type = helpers.FailTypeEnum("srpm_build_error")

  

+             cls.process_update_callback(build)

              db.session.add(build)

              return

  

@@ -928,8 +929,9 @@ 

              'uid': str(build.id),

          }

  

+         log.info('Sending data to Pagure API: %s', pprint.pformat(data))

          response = requests.post(api_url, data=data, headers=headers)

-         log.info('Pagure API response: '+response.text)

+         log.info('Pagure API response: %s', response.text)

  

      @classmethod

      def cancel_build(cls, user, build):

@@ -948,6 +950,8 @@ 

              ActionsLogic.send_cancel_build(build)

  

          build.canceled = True

+         cls.process_update_callback(build)

+ 

          for chroot in build.build_chroots:

              chroot.status = 2  # canceled

              if chroot.ended_on is not None:

@@ -985,6 +989,7 @@ 

          chroots = filter(lambda x: x.status != helpers.StatusEnum("succeeded"), build.build_chroots)

          for chroot in chroots:

              chroot.status = helpers.StatusEnum("failed")

+         cls.process_update_callback(build)

          return build

  

      @classmethod

@@ -75,32 +75,6 @@ 

          return fcopr, created

  

      @staticmethod

-     def get_group_copr_dir_safe(group_name, copr_dirname, **kwargs):

-         """ Get group project_dir by group_name and copr_dirname."""

-         try:

-             return CoprDirsLogic.get_by_groupname(

-                 group_name, copr_dirname, **kwargs).one()

-         except sqlalchemy.orm.exc.NoResultFound:

-             raise ObjectNotFound(

-                 message="Project directory @{}/{} does not exist."

-                         .format(group_name, copr_dirname))

- 

-     @staticmethod

-     def get_copr_dir_safe(user_name, copr_dirname, **kwargs):

-         """ Get project_dir by user_name and copr_dirname.

- 

-         This always returns a personal directory.

-         For group project directories, see get_group_copr_dir_safe().

-         """

-         try:

-             return CoprDirsLogic.get_by_username(

-                 user_name, copr_dirname, **kwargs).filter(Copr.group_id.is_(None)).one()

-         except sqlalchemy.orm.exc.NoResultFound:

-             raise ObjectNotFound(

-                 message="Project directory {}/{} does not exist."

-                         .format(user_name, copr_dirname))

- 

-     @staticmethod

      def get_group_copr_safe(group_name, copr_name, **kwargs):

          group = ComplexLogic.get_group_by_name_safe(group_name)

          try:

@@ -131,10 +105,12 @@ 

          return ComplexLogic.get_copr_safe(owner_name, copr_name, **kwargs)

  

      @staticmethod

-     def get_copr_dir_by_owner_safe(owner_name, copr_dirname, **kwargs):

-         if owner_name[0] == "@":

-             return ComplexLogic.get_group_copr_dir_safe(owner_name[1:], copr_dirname, **kwargs)

-         return ComplexLogic.get_copr_dir_safe(owner_name, copr_dirname, **kwargs)

+     def get_copr_dir_safe(ownername, copr_dirname, **kwargs):

+         try:

+             return CoprDirsLogic.get_by_ownername(ownername, copr_dirname).one()

+         except sqlalchemy.orm.exc.NoResultFound:

+             raise ObjectNotFound(message="copr dir {}/{} does not exist."

+                         .format(ownername, copr_dirname))

  

      @staticmethod

      def get_copr_by_id_safe(copr_id):

@@ -217,9 +217,12 @@ 

          if not user.admin and not auto_prune:

              raise exceptions.NonAdminCannotDisableAutoPrunning()

  

+         # form validation checks for duplicates

+         cls.new(user, name, group, check_for_duplicates=check_for_duplicates)

+ 

          copr = models.Copr(name=name,

                             repos=repos or u"",

-                            user_id=user.id,

+                            user=user,

                             description=description or u"",

                             instructions=instructions or u"",

                             created_on=int(time.time()),

@@ -234,14 +237,13 @@ 

              UsersLogic.raise_if_not_in_group(user, group)

              copr.group = group

  

-         # form validation checks for duplicates

-         cls.new(user, copr, check_for_duplicates=check_for_duplicates)

- 

          copr_dir = models.CoprDir(

              main=True,

              name=name,

              copr=copr)

+ 

          db.session.add(copr_dir)

+         db.session.add(copr)

  

          CoprChrootsLogic.new_from_names(

              copr, selected_chroots)

@@ -252,18 +254,16 @@ 

          return copr

  

      @classmethod

-     def new(cls, user, copr, check_for_duplicates=True):

+     def new(cls, user, copr_name, group=None, check_for_duplicates=True):

          if check_for_duplicates:

-             if copr.group is None and cls.exists_for_user(user, copr.name).all():

+             if group is None and cls.exists_for_user(user, copr_name).all():

                  raise exceptions.DuplicateException(

-                     "Copr: '{0}/{1}' already exists".format(user.name, copr.name))

-             elif copr.group:

-                 db.session.flush()  # otherwise copr.id is not set from sequence

-                 if cls.exists_for_group(copr.group, copr.name).filter(models.Copr.id != copr.id).all():

+                     "Copr: '{0}/{1}' already exists".format(user.name, copr_name))

+             elif group:

+                 if cls.exists_for_group(group, copr_name).all():

                      db.session.rollback()

                      raise exceptions.DuplicateException(

-                         "Copr: '@{0}/{1}' already exists".format(copr.group.name, copr.name))

-         db.session.add(copr)

+                         "Copr: '@{0}/{1}' already exists".format(group.name, copr_name))

  

      @classmethod

      def update(cls, user, copr):

@@ -292,23 +292,13 @@ 

              copr, "Can't delete this project,"

                    " another operation is in progress: {action}")

  

-         cls.create_delete_action(copr)

-         copr.deleted = True

+         ActionsLogic.send_delete_copr(copr)

+         CoprDirsLogic.delete_all_by_copr(copr)

  

+         copr.deleted = True

          return copr

  

      @classmethod

-     def create_delete_action(cls, copr):

-         action = models.Action(action_type=helpers.ActionTypeEnum("delete"),

-                                object_type="copr",

-                                object_id=copr.id,

-                                old_value=copr.full_name,

-                                new_value="",

-                                created_on=int(time.time()))

-         db.session.add(action)

-         return action

- 

-     @classmethod

      def exists_for_user(cls, user, coprname, incl_deleted=False):

          existing = (models.Copr.query

                      .filter(models.Copr.name == coprname)

@@ -442,20 +432,6 @@ 

  

  class CoprDirsLogic(object):

      @classmethod

-     def get_copr(cls, username, dirname, **kwargs):

-         query = CoprsLogic.get_multiple_by_username(username, **kwargs)

-         query = query.join(models.CoprDir).filter(

-             models.CoprDir.name == dirname)

-         return query

- 

-     @classmethod

-     def get_copr_by_group_id(cls, group_id, dirname, **kwargs):

-         query = CoprsLogic.get_multiple_by_group_id(group_id, **kwargs)

-         query = query.join(models.CoprDir).filter(

-             models.CoprDir.name == dirname)

-         return query

- 

-     @classmethod

      def get_or_create(cls, copr, dirname, main=False):

          copr_dir = cls.get_by_copr(copr, dirname).first()

  

@@ -470,24 +446,25 @@ 

  

      @classmethod

      def get_by_copr(cls, copr, dirname):

-         return (db.session.query(models.CoprDir).join(models.Copr)

-                 .filter(models.Copr.id==copr.id).filter(models.CoprDir.name==dirname))

+         return (db.session.query(models.CoprDir)

+                 .join(models.Copr)

+                 .filter(models.Copr.id==copr.id)

+                 .filter(models.CoprDir.name==dirname))

  

      @classmethod

      def get_by_ownername(cls, ownername, dirname):

-         if ownername.startswith('@'):

-             return cls.get_by_groupname(ownername[1:], dirname)

-         return cls.get_by_username(ownername, dirname)

+         return (db.session.query(models.CoprDir)

+                 .filter(models.CoprDir.name==dirname)

+                 .filter(models.CoprDir.ownername==ownername))

  

      @classmethod

-     def get_by_username(cls, username, dirname):

-         return (db.session.query(models.CoprDir).join(models.Copr).join(models.User)

-                 .filter(models.CoprDir.name==dirname).filter(models.User.username==username))

+     def delete(cls, copr_dir):

+         db.session.delete(copr_dir)

  

      @classmethod

-     def get_by_groupname(cls, groupname, dirname):

-         return (db.session.query(models.CoprDir).join(models.Copr).join(models.Group)

-                 .filter(models.CoprDir.name==dirname).filter(models.Group.name==groupname))

+     def delete_all_by_copr(cls, copr):

+         for copr_dir in copr.dirs:

+             db.session.delete(copr_dir)

  

  

  def on_auto_createrepo_change(target_copr, value_acr, old_value_acr, initiator):

@@ -262,7 +262,7 @@ 

          """

          Return True if copr belongs to a group

          """

-         return self.group_id is not None

+         return self.group is not None

  

      @property

      def owner(self):

@@ -433,16 +433,24 @@ 

      name = db.Column(db.Text, index=True)

      main = db.Column(db.Boolean, default=False, server_default="0", nullable=False)

  

+     ownername = db.Column(db.Text, index=True, nullable=False)

+ 

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

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

  

      __table_args__ = (

          db.Index('only_one_main_copr_dir', copr_id, main,

                   unique=True, postgresql_where=(main==True)),

-         db.UniqueConstraint('copr_id', 'name',

-                             name='copr_dir_copr_id_name_uniq'),

+ 

+         db.UniqueConstraint('ownername', 'name',

+                             name='ownername_copr_dir_uniq'),

      )

  

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

+         if kwargs.get('copr') and not kwargs.get('ownername'):

+             kwargs['ownername'] = kwargs.get('copr').owner_name

+         super(CoprDir, self).__init__(*args, **kwargs)

+ 

      @property

      def full_name(self):

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

@@ -486,7 +494,7 @@ 

      # Source of the build: description in json, example: git link, srpm url, etc.

      source_json = db.Column(db.Text)

      # True if the package is built automatically via webhooks

-     webhook_rebuild = db.Column(db.Boolean, default=True)

+     webhook_rebuild = db.Column(db.Boolean, default=False)

      # enable networking during a build process

      enable_net = db.Column(db.Boolean, default=False, server_default="0", nullable=False)

  

@@ -1090,7 +1098,7 @@ 

      def dist_git_url(self):

          if app.config["DIST_GIT_URL"]:

              if self.state == "forked":

-                 copr_dirname = self.build.copr.main_dir.full_name

+                 copr_dirname = self.build.copr.forked_from.main_dir.full_name

              else:

                  copr_dirname = self.build.copr_dir.full_name

              return "{}/{}/{}.git/commit/?id={}".format(app.config["DIST_GIT_URL"],

@@ -390,13 +390,23 @@ 

  

  

  {% macro repo_file_href(copr, repo) %}

-   {{- copr_url('coprs_ns.generate_repo_file',

-       copr,

+   {% if copr.is_a_group_project: %}

Can't we just use copr_url macro instead of conditioning whether it is a group project? This is exactly what the macro does :-). And it is not obscurely workarounded if that was your main concern :-)

... it is not obscurely workarounded anymore

clime commented a year ago

The copr_url macro generated some garbage into query params because it always tries to put coprname into url but in this case, there is no coprname in the URL path because that param got replaced by copr_dirname.

+   {{- url_for('coprs_ns.generate_repo_file',

+       group_name=copr.group.name,

        copr_dirname=copr.main_dir.name,

        name_release=repo.name_release,

        repofile=repo.repo_file,

        _external=True

      )|fix_url_https_frontend -}}

+   {% else %}

+   {{- url_for('coprs_ns.generate_repo_file',

+       username=copr.user.name,

+       copr_dirname=copr.main_dir.name,

+       name_release=repo.name_release,

+       repofile=repo.repo_file,

+       _external=True

+     )|fix_url_https_frontend -}}

+   {% endif %}

  {% endmacro %}

  

  

@@ -19,29 +19,28 @@ 

  {% endif %}

  

  <h2 class="page-title">Project Builds</h2>

-   {% if builds %}

- 

-     {% if copr.dirs|length > 1 %}

-     <div class="panel panel-default">

-       <div class="panel-body">

-         <div class="btn-group" role="group">

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

-             all builds

+   {% if copr.dirs|length > 1 %}

+   <div class="panel panel-default">

+     <div class="panel-body">

+       <div class="btn-group" role="group">

+         <a href="{{ copr_url('coprs_ns.copr_builds', copr) }}" class="btn btn-default btn-sm {% if not current_dirname %}active{% endif %}">

+           all builds

+         </a>

+         {% for copr_dir in copr.dirs|sort(attribute='name') %}

+             <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 %}">

+             {{ copr_dir.name }}

            </a>

-           {% for copr_dir in copr.dirs %}

-               <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 %}">

-               {{ copr_dir.name }}

-             </a>

-           {% endfor %}

-         </div>

+         {% endfor %}

        </div>

      </div>

-     {% endif %}

- 

+   </div>

+   {% endif %}

+   {% if builds %}

      {{ builds_table(builds) }}

      {{ build_states() }}

    {% else %}

    <div class="blank-slate-pf">

+     {% if not current_dirname %}

      <div class="blank-slate-pf-icon">

        <span class="pficon pficon pficon-add-circle-o"></span>

      </div>

@@ -59,6 +58,11 @@ 

        <a class="btn btn-primary btn-lg" href="{{ copr_url('coprs_ns.copr_add_build', copr) }}"> Submit a New Build </a>

      </div>

      {% endif %}

+     {% else %}

+     <h1>

I know, that we already have this in the code, but IMHO we should change the <h1> to something else. It doesn't make sense to have <h1> below <h2>

clime commented a year ago

That's probably true but I think this is just a minor css issue.

+       No builds in this project directory.

+     </h1>

+     {% endif %}

    </div>

    {% endif %}

  {% endblock %}

@@ -50,7 +50,7 @@ 

  def get_package(ownername, projectname, packagename):

      copr = get_copr(ownername, projectname)

      try:

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

+         package = PackagesLogic.get(copr.main_dir.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))

@@ -61,7 +61,7 @@ 

  @query_params()

  def get_package_list(ownername, projectname, **kwargs):

      copr = get_copr(ownername, projectname)

-     paginator = Paginator(PackagesLogic.get_all(copr.id), models.Package, **kwargs)

+     paginator = Paginator(PackagesLogic.get_all(copr.main_dir.id), models.Package, **kwargs)

      packages = paginator.map(to_dict)

      return flask.jsonify(items=packages, meta=paginator.meta)

  

@@ -72,7 +72,7 @@ 

      copr = get_copr(ownername, projectname)

      data = rename_fields(get_input())

      process_package_add_or_edit(copr, source_type_text, data=data)

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

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

      return flask.jsonify(to_dict(package))

  

  

@@ -82,7 +82,7 @@ 

      copr = get_copr(ownername, projectname)

      data = rename_fields(get_input())

      try:

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

+         package = PackagesLogic.get(copr.main_dir.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}."

@@ -98,7 +98,7 @@ 

      copr = get_copr()

      form = forms.BasePackageForm()

      try:

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

+         package = PackagesLogic.get(copr.main_dir.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))

@@ -115,7 +115,7 @@ 

      data = rename_fields(get_form_compatible_data())

      form = forms.RebuildPackageFactory.create_form_cls(copr.active_chroots)(data, csrf_enabled=False)

      try:

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

+         package = PackagesLogic.get(copr.main_dir.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))

@@ -134,7 +134,7 @@ 

      copr = get_copr()

      form = forms.BasePackageForm()

      try:

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

+         package = PackagesLogic.get(copr.main_dir.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))

@@ -732,14 +732,8 @@ 

      """ Generate repo file for a given repo name.

          Reponame = username-coprname """

  

-     if username and username.startswith("@"):

-         group_name=username[1:]

- 

-     if group_name:

-         copr_dir = ComplexLogic.get_group_copr_dir_safe(group_name, copr_dirname)

-     else:

-         copr_dir = ComplexLogic.get_copr_dir_safe(username, copr_dirname)

- 

+     ownername = username if username else ('@'+group_name)

+     copr_dir = ComplexLogic.get_copr_dir_safe(ownername, copr_dirname)

      return render_generate_repo_file(copr_dir, name_release)

  

  

@@ -26,7 +26,7 @@ 

  SCM_SOURCE_TYPE = helpers.BuildSourceEnum("scm")

  

  logging.basicConfig(

-     filename='{0}/build_on_pagure_commit.log'.format(app.config.get('LOG_DIR')),

+     filename='{0}/pagure-events.log'.format(app.config.get('LOG_DIR')),

      format='[%(asctime)s][%(levelname)6s]: %(message)s',

      level=logging.DEBUG)

  

@@ -190,8 +190,8 @@ 

  

  

  def git_compare_urls(url1, url2):

-     url1 = re.sub(r'(\.git)?/*$', '', url1)

-     url2 = re.sub(r'(\.git)?/*$', '', url2)

+     url1 = re.sub(r'(\.git)?/*$', '', str(url1))

+     url2 = re.sub(r'(\.git)?/*$', '', str(url2))

      o1 = urlparse(url1)

      o2 = urlparse(url2)

      return (o1.netloc == o2.netloc and o1.path == o2.path)

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

  from marshmallow import pprint

  

  from coprs.helpers import BuildSourceEnum, StatusEnum

+ from coprs.logic.actions_logic import ActionsLogic

  from coprs.logic.coprs_logic import CoprsLogic

  from coprs.logic.builds_logic import BuildsLogic

  from tests.coprs_test_case import CoprsTestCase

@@ -225,7 +226,7 @@ 

              f_mock_chroots_many, f_build_many_chroots,

              f_users_api):

  

-         CoprsLogic.create_delete_action(self.c1)

+         ActionsLogic.send_delete_copr(self.c1)

          chroot_name_list = [c.name for c in self.c1.active_chroots]

          metadata = {

              "project_id": 1,

@@ -271,7 +272,7 @@ 

              f_mock_chroots_many, f_build_many_chroots,

              f_users_api):

  

-         CoprsLogic.create_delete_action(self.c1)

+         ActionsLogic.send_delete_copr(self.c1)

          chroot_name_list = [c.name for c in self.c1.active_chroots]

          metadata = {

              "project_id": 1,

@@ -10,6 +10,7 @@ 

  import sqlalchemy

  from coprs.logic.builds_logic import BuildsLogic

  

+ from coprs.logic.actions_logic import ActionsLogic

  from coprs.logic.users_logic import UsersLogic

  from coprs.logic.coprs_logic import CoprsLogic

  from coprs.models import Copr

@@ -340,7 +341,7 @@ 

              self, f_users, f_mock_chroots,

              f_coprs, f_users_api, f_db):

  

-         CoprsLogic.create_delete_action(self.c1)

+         ActionsLogic.send_delete_copr(self.c1)

          self.db.session.commit()

          href = "/api_2/projects/{}".format(self.c1.id)

          r0 = self.request_rest_api_with_auth(

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

  from coprs import models

  from coprs.helpers import ActionTypeEnum

  

- from coprs.logic.coprs_logic import CoprsLogic

+ from coprs.logic.coprs_logic import CoprsLogic, CoprDirsLogic

  from coprs.logic.actions_logic import ActionsLogic

  

  from tests.coprs_test_case import CoprsTestCase, TransactionDecorator

@@ -176,8 +176,10 @@ 

              self, f_users, f_coprs, f_mock_chroots, f_db):

  

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

+         self.db.session.add(self.c1_dir)

          self.c1.deleted = True

          self.c1.user = self.u1

+         CoprDirsLogic.delete_all_by_copr(self.c1)

          self.db.session.commit()

  

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

SCM Integration with Pagure for flagging pull request (and commits).

At the moment, the code is not finished but the basics has been done so I believe it can be reviewed as it is. I will finish it after somebody looks at it.

This is pretty complicated, and potentially complicating part to keep sync between frontend/backend "databases". I'd much rather vote for avoiding this at all, and having a consistent layout as we have so far --- project dedicated for PRs, and having set-up the pagure integration per-package.

The main advantage of this concrete implementation is that you can maintain your project settings on a single place only. If you had an additional PR-dedicated copr, you would always need to make sure the settings of the coprs are synced - i.e. when adding an additional repo into main copr, you would need to add it in the PR-copr as well. Also, this implementation offers better integration with . We could e.g. delete a copr_dir once the respective pull request is closed/merged, which is the most suitable time to be deleting the PR builds&packages. I would like to create really tight integration between an SCM repo and copr so that people can really easily "jump in" if they know how a Git-hosting site work. That's another advantage that having PRs "integrated" into main repo directly is more self-explanatory. I think the idea here is quite simple, on backend there are "copr_dirs" having N:1 association with a copr. One of the copr_dirs is the main one - the once accessible directly under <ownername>/<projectname>.

The main advantage of this concrete implementation is that you can maintain your project settings on a single place only. If you had an additional PR-dedicated copr, you would always need to make sure the settings of the coprs are synced

It depends, I would rather never mix such different things into one project - and keeping these separated (with different config, set of chroots, build deps) is expected for my real life use-cases.

To me it is almost zero benefit, for a lot of maintenance and potential desync; so I vote against <project>:PR:<PR_ID> layout to keep our life simpler and have more future-flexible design.

For me the biggest bottleneck is that I can not build from multiple project-related git repositories into one PR-related copr project. This is IMO clear sign that we should configure this per-package, not per copr project, how expensive that would be?

Also, I'd prefer dnf copr enable copr/copr-pr instead of

dnf copr enable copr/dist-git:pr:XX
dnf copr enable copr/copr:pr:YY

Anyways, I consider installing software from PRs to be really risky :-) so even though it is nice -- this feature should be less prioritized if it goes against the general "convenience" of setting the nice "green CI" tasks. I guess that most of our users is going to be interested to see if the buidl and build-time test-site passed, and if we supported some integration testing on PRs -- it would be anyways trivial to setup via copr API (download the built packages by the build ID).

Can you change this to lazy evaluation?
...log.exception("foo {}", ownername, ...

name should have separate index as you use it often in filter() conditions.

The main advantage of this concrete implementation is that you can maintain your project settings on a single place only. If you had an additional PR-dedicated copr, you would always need to make sure the settings of the coprs are synced

It depends, I would rather never mix such different things into one project - and keeping these separated (with different config, set of chroots, build deps) is expected for my real life use-cases.
To me it is almost zero benefit, for a lot of maintenance and potential desync; so I vote against <project>:PR:<pr_id> layout to keep our life simpler and have more future-flexible design.
For me the biggest bottleneck is that I can not build from multiple project-related git repositories into one PR-related copr project. This is IMO clear sign that we should configure this per-package, not per copr project, how expensive that would be?

I don't see that as a clear sign. I see that as a sign that we can't do everything for everyone. I don't know what exactly you mean by 'per-package', if you mean by it that you would need to setup an extra PR repo where everything gets build on one pile, then I already described why this is not advantageous. You typically want to have the same package and chroot settings for PRs as well as for the main repo, because those PRs are supposed to be merged into your code and that's why they should be built against the same build settings as there are for the main repo. And you cannot delete the PR builds once they are closed, which is a feature we might want to implement one day.

Also, it's really a simple aproach picked here - for a Git repo, you have a development copr repo. 1:1 relation here. If you want something else like production/release copr - you can have that as well. Setup a copr where e.g. only tagged commits will be built and you have it. In this copr, no PRs will be built.

Also, I'd prefer dnf copr enable copr/copr-pr instead of
dnf copr enable copr/dist-git🇵🇷XX
dnf copr enable copr/copr🇵🇷YY

: was chosen to avoid collisions between the main copr dir names and the pull request dir names. I also think the character chosen is pretty much irrelevant as long as it works.

Anyways, I consider installing software from PRs to be really risky :-) so even though it is nice -- this feature should be less prioritized if it goes against the general "convenience" of setting the nice "green CI" tasks. I guess that most of our users is going to be interested to see if the buidl and build-time test-site passed, and if we supported some integration testing on PRs -- it would be anyways trivial to setup via copr API (download the built packages by the build ID).

Sorry, I don't exactly understand what you are saying here. This feature has, however, pretty big priority as it could potentially attract more developers to us.

-1 as long as only one git repo can be associated with one copr project.
-1 as long as there's the random new syntax
'<project><separator><pr><separator><pr_id>'

This deserves better design and integration, and taking it "generally"
across other providers (e.g. gitlab/github), so it should be taken
seriously. E.g. note that there aren't pull-requests in gitlab, etc.

Sorry, I don't exactly understand what you are saying here. This feature has,
however, pretty big priority as it could potentially attract more developers
to us.

I'm saying that much important is to make the CI setup easier, convenient
and natural, to atract more developers. Having one copr for max one git
repo is far from what I'd expect. Neither @copr/copr builds packages only from
one repo...

Also the separated copr repo for PRs avoids code bloat (we already have
everything set-up), and thus maintenance costs (and also admin costs).

rebased onto 22d0dbecd2a131db7c8ca27762e7319f8bea6c01

a year ago

rebased onto 4538954d348e278abe00b11466f35d8edaed4cc1

a year ago

rebased onto e4d5e1067978c9ea1416ea326fc34d88a2c3374e

a year ago

1 new commit added

  • [backend] add dir support into copr_prune_results.py
a year ago

I have:

  • added GUI filtering in Build list by a copr dir.
  • added support into copr_prune_results.py scripts
  • finished pagure-events (former build_on_pagure_commit) script with support for PR updates and commit flagging
  • changed Build.end_callback to Build.update_callback
  • fixed frontend unit tests
  • fixed forking with respect to copr dirs
  • moved scm integration properties from CoprDir to Build and regenerated alembic revision

Things left:
- bug hunting

rebased onto 79971bc83f9672ce765246dcf57c34d486f5e947

a year ago

rebased onto 42a5f21b96d4094bb886dc97426742e2387f3cd2

a year ago

Ping for review. Pagure does not correctly show the diff on all files here so, please paste any comments below here.

According to the PR#330 it should be log.info('Sending data to Pagure API: %s', pprint.pformat(data))

Shouldn't we rather introduce user_id and group_id columns, use it for the UniqueConstraint below and if needed, add also a property for ownername that will return an appropriate user/group name?

Can't we just use copr_url macro instead of conditioning whether it is a group project? This is exactly what the macro does :-). And it is not obscurely workarounded if that was your main concern :-)

I know, that we already have this in the code, but IMHO we should change the <h1> to something else. It doesn't make sense to have <h1> below <h2>

Ah man, I am sorry. My browser somehow stopped loading the page when not all comments were showed yet and I didn't see the comment about wrong diff.

I am going to look on https://pagure.io/copr/copr/c/87a4af6be8e2c964d919aa9aa2b00f992b5e3c2d?branch=pagure_integration_new

I think it's fine like this. The unique constraint would be complicated with both user_id and group_id ( group_id, copr_dir.name must be unique only if group_id is not NULL, otherwise user_id, copr_dir.name must be unique). This is more direct approach to assure that you can't get the same copr_dir.name for the same ownername on copr-backend.

The copr_url macro generated some garbage into query params because it always tries to put coprname into url but in this case, there is no coprname in the URL path because that param got replaced by copr_dirname.

That's probably true but I think this is just a minor css issue.

rebased onto a62472ba6d5ac0921ef4ba94f642070bcf910803

a year ago

I removed the alembic comment, used the log.info interpolation (note that the interpolation actually only works in frontend, not in backend where we implement a custom logger), and fixed some additional issues that I discovered and had in stack to push. I don't think there should be more (apart from the review ones).

I've reviewed the https://pagure.io/copr/copr/c/87a4af6be8e2c964d919aa9aa2b00f992b5e3c2d?branch=pagure_integration_new

52 + self.log.info("Creating repo for: {}/{}/{}"
53 + .format(ownername, project_dirname, chroot))

So we can't use %s and parameters here? PR#330 did it also on backend.

4 + Revision ID: 887cbbd6575e
5 + Revises: acac8d3ae868
6 + Create Date: 2018-07-16 11:32:26.554253

This migration script looks quite messy. It would be nice to split it into multiple small migrations. However, it would probably be a waste of time doing it now. Can you please put some blank spaces there to separate it into multiple logical pieces and add very short comments to them?

15 + def trim_git_url(url):
16 + if not url:
17 + return False

This should IMHO return None because we don't expect a boolean response from it. Or even return the unmodified url, which can be None or "", etc.

32 @classmethod
33 - def get_copr_builds_list(cls, copr):
34 + def get_copr_builds_list(cls, copr, dirname):100:

Can the dirname parameter be optional?

88 @classmethod
89 - def rebuild_package(cls, package, source_dict_update={}):
90 + def rebuild_package(cls, package, source_dict_update={}, copr_dir=None, update_callback=None,
91 + scm_object_type=None, scm_object_id=None, scm_object_url=None):

I probably don't understand the purpose of those parametters, sorry. Package can have scm source defined, so it may make sense there, but what are they for, when the package source is pypi or something? Can you explain, please?

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

Why do we want to query package by ID of a main_dir instead of a project ID?

@classmethod
    def get(cls, copr_id, package_name):

... Aha, further down in the diff, there is also a change of this the PackagesLogic.get method. To be honest, it doesn't make sense to me. When I want to query a package, I think of (copr_id, package_name) combination. If I want to filter only packages from some dir, I would prefer doing it via an optional parameter. When not specified, it would query a package from the main dir.

Same for add and exists, etc methods.

0 + @property
151 + def full_name(self):
152 + return "{}/{}".format(self.copr.owner_name, self.name)

Shouldn't a self.copr.name figure in it? Or it is a part of self.name? In that case, it would be storing unnecessary duplicities.

21 + <dt> Directory:
22 + <dd>
23 + <a href="{{ build.copr_dir.repo_url }}">
24 + {{ build.copr_dir.name }}
25 + </a>
26 + </dd>

IMHO this part should be also conditional. e.g. {% if build.scm_object_type %} ... Because when it is not auto-rebuilt from a pull request, it won't have its own directory.

Any of the issues is not a deal breaker though ...

I've reviewed the https://pagure.io/copr/copr/c/87a4af6be8e2c964d919aa9aa2b00f992b5e3c2d?branch=pagure_integration_new

52 + self.log.info("Creating repo for: {}/{}/{}"
53 + .format(ownername, project_dirname, chroot))

So we can't use %s and parameters here? PR#330 did it also on backend.

That PR did not, in fact, work. We will need to fix logging in backend before next release.

4 + Revision ID: 887cbbd6575e
5 + Revises: acac8d3ae868
6 + Create Date: 2018-07-16 11:32:26.554253

This migration script looks quite messy. It would be nice to split it into multiple small migrations. However, it would probably be a waste of time doing it now. Can you please put some blank spaces there to separate it into multiple logical pieces and add very short comments to them?

yes, I can but this is mostly an auto-generated migration. I don't think migrations need to be commented because they are not going to be changed in future by another programmer. They just need to work.

15 + def trim_git_url(url):
16 + if not url:
17 + return False

This should IMHO return None because we don't expect a boolean response from it. Or even return the unmodified url, which can be None or "", etc.

I can return None e.g. there, that should be ok.

32 @classmethod
33 - def get_copr_builds_list(cls, copr):
34 + def get_copr_builds_list(cls, copr, dirname)💯

Can the dirname parameter be optional?

Yup, sure.

88 @classmethod
89 - def rebuild_package(cls, package, source_dict_update={}):
90 + def rebuild_package(cls, package, source_dict_update={}, copr_dir=None, update_callback=None,
91 + scm_object_type=None, scm_object_id=None, scm_object_url=None):

I probably don't understand the purpose of those parametters, sorry. Package can have scm source defined, so it may make sense there, but what are they for, when the package source is pypi or something? Can you explain, please?

From these properties, commit hash or pull request ID is taken and displayed in a build detail page, the url enables you to easily jump to the respective commit or PR on a Git hosting site that triggered the build.

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

Why do we want to query package by ID of a main_dir instead of a project ID?

Because currently almost everywhere in our logic, you want to get only the packages that are associated with the main dir. Not the packages that are associated with the other dirs. Querying only by copr is ambiguous - you would probably expect that all packages associated with the copr_id are returned, which would not work for us (e.g. in the case of forking), we usually only want packages associated with the main dir (even in the frotend UI it's like that).

@classmethod
def get(cls, copr_id, package_name):

... Aha, further down in the diff, there is also a change of this the PackagesLogic.get method. To be honest, it doesn't make sense to me. When I want to query a package, I think of (copr_id, package_name) combination. If I want to filter only packages from some dir, I would prefer doing it via an optional parameter. When not specified, it would query a package from the main dir.

You can have several packages with the same name for the same copr now given that they all belong to different dirs. The copr_dir, package relation is now important. The copr_id param for a package does not play so much of a role.

Same for add and exists, etc methods.

0 + @property
151 + def full_name(self):
152 + return "{}/{}".format(self.copr.owner_name, self.name)

Shouldn't a self.copr.name figure in it? Or it is a part of self.name? In that case, it would be storing unnecessary duplicities.

full_name corresponds to what is being stored in backend under results. You are right that self.name will usually contain self.copr.name in it but it does not necessarily need to be like. The main thing here that the name of copr_dir is the name of the directory under the owner directory on copr-backend.

21 +

Directory:
22 +

23 +
24 + {{ build.copr_dir.name }}
25 +

26 +

IMHO this part should be also conditional. e.g. {% if build.scm_object_type %} ... Because when it is not auto-rebuilt from a pull request, it won't have its own directory.

Build will always have its own directory. For normal builds it is the main_dir. For auto-rebuilds it might be something else. CoprDirs directly corresponds to directory structure on copr-backend.

Any of the issues is not a deal breaker though ...

rebased onto 168e78cb5f381b08152074831e67d2bc5678a952

a year ago

I added the following commits:

[frontend] make copr_dir param for get_copr_builds_list optional
[frontend] return None from trim_git_url

Do you want the comments added into the migration? I can do it here but long term I don't think we should be doing it (or rather that it should be required to do).

Btw. if https://pagure.io/copr/copr/pull-request/278 is merged first, then a part of this PR should be fix for apiv3 with respect to the package logic change but I think I can do that quite easily.

Btw. if https://pagure.io/copr/copr/pull-request/278 is merged first, then a part of this PR should be fix for apiv3 with respect to the package logic change but I think I can do that quite easily.

Also note that the fix will only affect the copr-frontend part. As far as the python API goes, we will just keep the current interface that does not know about copr_dirs.

Do you want the comments added into the migration? I can do it here but long term I don't think we should be doing it (or rather that it should be required to do).

I know that migrations are usually write-only thing, but sometimes you go back and read some of them and this one is quite messy and takes a long time to orient in. I don't want you to spend an hour on writing comments in it. I think that grouping copr/build/package together and separating them with a blank line will be good enough.

rebased onto 344ef6185059ae7f13902e1001e2cb8992eb7565

a year ago

I've added the comments into the alembic migration. I've also updated apiv3_packages.py with respect to copr_dirs introduction.

Thank you, it looks much more clear now.

88 @classmethod
89 - def rebuild_package(cls, package, source_dict_update={}):
90 + def rebuild_package(cls, package, source_dict_update={}, copr_dir=None, update_callback=None,
91 + scm_object_type=None, scm_object_id=None, scm_object_url=None):

I probably don't understand the purpose of those parametters, sorry. Package can have scm source defined, so it may make sense there, but what are they for, when the package source is pypi or something? Can you explain, please?

From these properties, commit hash or pull request ID is taken and displayed in a build detail page, the url enables you to easily jump to the respective commit or PR on a Git hosting site that triggered the build.

Aha, so it makes sense for all source types.

Because currently almost everywhere in our logic, you want to get only the packages that are associated with the main dir. Not the packages that are associated with the other dirs. Querying only by copr is ambiguous - you would probably expect that all packages associated with the copr_id are returned, which would not work for us (e.g. in the case of forking), we usually only want packages associated with the main dir (even in the frotend UI it's like that).
You can have several packages with the same name for the same copr now given that they all belong to different dirs. The copr_dir, package relation is now important. The copr_id param for a package does not play so much of a role.

This is the only thing that I disagree with. Imho all the package methods should be changed to take (copr, package_name) instead of (copr_id, package_name). This way they would be consistent with the most of other logic code, which takes the whole copr object. Then an additional copr_dir=None should be added. If not copr_dir then copr_dir=copr.main_dir.

The current implementation should be equivalent with the suggested one and work as well. You could object, that it is doing the same thing, but it requires more parameters and thus it is worse. I disagree because, in a code where we don't care about the special directories feature (i.e. want to get things from the main dir), we wouldn't have to.

I won't -1 the current implementation though. I just want to suggest my idea. Can anyone else share his opinion on this?

Aha, so it makes sense for all source types.

It may not be used for some source types, that why the proprties are nullable.

Because currently almost everywhere in our logic, you want to get only the packages that are associated with the main dir. Not the packages that are associated with the other dirs. Querying only by copr is ambiguous - you would probably expect that all packages associated with the copr_id are returned, which would not work for us (e.g. in the case of forking), we usually only want packages associated with the main dir (even in the frotend UI it's like that).
You can have several packages with the same name for the same copr now given that they all belong to different dirs. The copr_dir, package relation is now important. The copr_id param for a package does not play so much of a role.

This is the only thing that I disagree with. Imho all the package methods should be changed to take (copr, package_name) instead of (copr_id, package_name). This way they would be consistent with the most of other logic code, which takes the whole copr object. Then an additional copr_dir=None should be added. If not copr_dir then copr_dir=copr.main_dir.
The current implementation should be equivalent with the suggested one and work as well. You could object, that it is doing the same thing, but it requires more parameters and thus it is worse. I disagree because, in a code where we don't care about the special directories feature (i.e. want to get things from the main dir), we wouldn't have to.
I won't -1 the current implementation though. I just want to suggest my idea. Can anyone else share his opinion on this?

I don't understand those objections very much. You can have several packages of the same name across multiple dirs, that's why if you want to get a single well-identified package, you should specify dir id and the package name. If you just specified copr (or copr_id) and package name, then e.g. PackagesLogic.get method would return multiple results, which is not what we expect. If you added If not copr_dir then copr_dir=copr.main_dir into that method, then yes, you would probably get something equivalent but also something quite strange because why then not pass the directory that you want to get package(s) from directly as the method param. And also just from seeing e.g. PackagesLogic.get(copr, package_name) you wouldn't then guess the internal assumption that if copr_dir is missing then it will be put equal to copr.main_dir.

If that was done, yes, we could have saved a few lines of code where I wouldn't need to change copr.id for copr.main_dir.id for price of possible future refactoring needed because of quite strange "double-default" values in PackagesLogic methods. I basically did the refactoring immediately while writing it the code. I think we are talking here about minor implementation detail where the way I did that is probably cleaner.

I think we are talking here about minor implementation detail

Don't get confused, also I see it this way, so I am not forcing you to reimplement it or something else. I just disagree that it is cleaner. Its the only thing that I would change in this PR, but you have my +1 regardless.

rebased onto 6c7696a

a year ago

rebased onto 21cf39b

a year ago

I think we are talking here about minor implementation detail

Don't get confused, also I see it this way, so I am not forcing you to reimplement it or something else. I just disagree that it is cleaner. Its the only thing that I would change in this PR, but you have my +1 regardless.

Ok, I think we can merge it then.

Pull-Request has been merged by clime

a year ago