#2262 Packit allowed forge projects
Merged 2 years ago by praiskup. Opened 2 years ago by lbarczio.
copr/ lbarczio/copr packit-forge-projects  into  main

file modified
+10
@@ -467,6 +467,7 @@ 

              fedora_review=args.fedora_review,

              appstream=ON_OFF_MAP[args.appstream],

              runtime_dependencies=args.runtime_dependencies,

+             packit_forge_projects_allowed=args.packit_forge_projects_allowed,

          )

  

          owner_part = username.replace('@', "g/")
@@ -498,6 +499,7 @@ 

              fedora_review=ON_OFF_MAP[args.fedora_review],

              appstream=ON_OFF_MAP[args.appstream],

              runtime_dependencies=args.runtime_dependencies,

+             packit_forge_projects_allowed=args.packit_forge_projects_allowed,

          )

  

      @requires_api_auth
@@ -983,6 +985,14 @@ 

              "This can be specified multiple times."

      ))

  

+     parser.add_argument(

+         "--packit-forge-project-allowed", dest="packit_forge_projects_allowed",

+         metavar="FORGE_PROJECT", action="append", help=(

+             "Forge project that will be allowed to build in this project "

+             "via Packit in format github.com/packit/ogr. "

+             "Can be specified multiple times."

+     ))

+ 

  

  def setup_parser():

      """

file modified
+2
@@ -477,6 +477,7 @@ 

          "fedora_review": False,

          "appstream": True,

          "runtime_dependencies": None,

+         "packit_forge_projects_allowed": None,

      }

      assert stdout == "New project was successfully created: http://copr/coprs/jdoe/foo/\n"

  
@@ -571,6 +572,7 @@ 

          "fedora_review": False,

          "appstream": True,

          "runtime_dependencies": None,

+         "packit_forge_projects_allowed": None,

      }

      assert stdout == "New project was successfully created: http://copr/coprs/jdoe/foo/\n"

  

file modified
+9 -13
@@ -15,7 +15,7 @@ 

  

  We support docker-compose to spawn the whole docker stack, including builder, on a local machine:

  

- - `Read the original blog post about COPR docker stack <https://frostyx.cz/posts/copr-stack-dockerized>`_

+ - `Read the blog post about COPR docker stack <https://frostyx.cz/posts/copr-docker-compose-without-supervisord>`_

  - `Learn more about docker on Fedora <https://developer.fedoraproject.org/tools/docker/about.html>`_

  

  
@@ -32,6 +32,10 @@ 

  

      $ docker-compose up -d

  

+     $ docker exec -it copr_frontend_1 bash

+ 

+     [copr-fe@frontend /]$ init-database.sh

+ 

  The ``docker-compose up`` command will take several minutes to complete.

  

  You should be now able to access the frontend on http://localhost:5000.
@@ -40,11 +44,9 @@ 

  Testing your changes

  --------------------

  

- To test your in frontend or backend, you should restart the respective container for the changes to take effect, e.g.:

- 

- ::

- 

-      $ docker-compose restart frontend

+ To test your changes in frontend or backend, you should restart the respective container for the changes to take effect.

+ You can read more details on how to do it and tips for troubleshooting in the previously mentioned

+ `blog post <https://frostyx.cz/posts/copr-docker-compose-without-supervisord#running-services-from-git>`_.

  

  

  Stopping the machines
@@ -75,12 +77,6 @@ 

      $ docker exec -it docker_backend_1 /bin/bash

      # tail -f /var/log/copr-backend/backend.log

  

- If something went wrong, you can also try to restart all the backend services and see if that helped

- 

- ::

- 

-     # supervisorctl restart all

- 

  

  Unit tests

  ^^^^^^^^^^
@@ -89,7 +85,7 @@ 

  

      $ cd copr/frontend

  

-     $ vim coprs_frontend/manage.py      (and make some change)

+     $ dnf builddep copr-frontend.spec     (or other equivalent command to install the needed dependencies)

  

      $ ./run_tests.sh

  

@@ -0,0 +1,22 @@ 

+ """

+ add packit_forge_projects_allowed for Copr projects

+ 

+ Revision ID: 004a017535dc

+ Revises: 484e28958b27

+ Create Date: 2022-07-28 12:20:31.777050

+ """

+ 

+ import sqlalchemy as sa

+ from alembic import op

+ 

+ 

+ revision = '004a017535dc'

+ down_revision = '484e28958b27'

+ 

+ 

+ def upgrade():

+     op.add_column('copr', sa.Column('packit_forge_projects_allowed', sa.Text(), nullable=True))

+ 

+ 

+ def downgrade():

+     op.drop_column('copr', 'packit_forge_projects_allowed')

@@ -429,6 +429,24 @@ 

              return helpers.PermissionEnum("request")

          return helpers.PermissionEnum("nothing")

  

+ class StripUrlSchemaListFilter():

+     """

+     Strip the URL schema if present for a list of forge projects.

+     """

+ 

+     def __call__(self, value):

+         if not value:

+             return ''

+ 

+         items = value.split()

+         result = []

+ 

+         for item in items:

+             parsed_url = urlparse(item)

+             result.append(parsed_url.netloc + parsed_url.path)

+ 

+         return "\n".join(result)

+ 

  def _optional_checkbox_filter(data):

      if data in [True, 'true']:

          return True
@@ -633,6 +651,11 @@ 

              Generating metadata slows down the builds in large Copr projects.""",

              default=True, false_values=FALSE_VALUES)

  

+     packit_forge_projects_allowed = wtforms.TextAreaField(

+         "Packit allowed forge projects",

+         filters=[StringListFilter(), StripUrlSchemaListFilter()],

+         validators=[wtforms.validators.Optional()],)

+ 

      @property

      def errors(self):

          """

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

              after_build_id=build_options.get("after_build_id"),

              with_build_id=build_options.get("with_build_id"),

              package_chroots_subset=package_chroots_subset,

+             packit_forge_project=build_options.get("packit_forge_project"),

          )

  

          if "timeout" in build_options:
@@ -795,10 +796,11 @@ 

              git_hashes=None, skip_import=False, background=False, batch=None,

              srpm_url=None, copr_dirname=None, bootstrap=None, isolation=None,

              package=None, after_build_id=None, with_build_id=None,

-             package_chroots_subset=None):

+             package_chroots_subset=None, packit_forge_project=None):

  

          coprs_logic.CoprsLogic.raise_if_unfinished_blocking_action(

              copr, "Can't build while there is an operation in progress: {action}")

+         coprs_logic.CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(copr, packit_forge_project)

          users_logic.UsersLogic.raise_if_cant_build_in_copr(

              user, copr,

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

@@ -413,6 +413,16 @@ 

          raise exceptions.InsufficientRightsException(

              "Only owners may delete their projects.")

  

+     @classmethod

+     def raise_if_packit_forge_project_cant_build_in_copr(cls, copr, packit_forge_project):

+         """

+         Raise InsufficientRightsException if given forge project can't build

+         in given copr via Packit. Return None otherwise.

+         """

+         if packit_forge_project and packit_forge_project not in copr.packit_forge_projects_allowed_list:

+             raise exceptions.InsufficientRightsException(

+                 f"Forge project {packit_forge_project} can't build in this Copr via Packit.")

+ 

  

  class CoprPermissionsLogic(object):

      @classmethod

@@ -349,6 +349,10 @@ 

  

      appstream = db.Column(db.Boolean, default=True, nullable=False, server_default="1")

  

+     # string containing forge projects (separated by whitespace)

+     # allowed to build in this Copr via Packit

+     packit_forge_projects_allowed = db.Column(db.Text)

+ 

  

  class _CoprPrivate(db.Model, helpers.Serializer):

      """
@@ -703,6 +707,15 @@ 

      def score(self):

          return sum([self.upvotes, self.downvotes * -1])

  

+     @property

+     def packit_forge_projects_allowed_list(self):

+         """

+         Return forge projects allowed to build in this copr via Packit

+          as a list of strings.

+         """

+         projects = self.packit_forge_projects_allowed or ""

+         return projects.split()

+ 

  

  class CoprPermission(db.Model, helpers.Serializer):

      """

@@ -161,6 +161,8 @@ 

      {{ render_bootstrap_options(form) }}

  

      {{ render_field(form.runtime_dependencies, rows=5, cols=50, placeholder='Optional - URL to additional yum repos, which can be used as runtime dependencies. Space separated. This should be baseurl from .repo file. E.g.: http://copr-be.cloud.fedoraproject.org/results/rhughes/f20-gnome-3-12/fedora-$releasever-$basearch/') }}

+ 

+     {{ render_field(form.packit_forge_projects_allowed, rows=5, cols=50, placeholder='Optional - forge projects, which can build in this project via Packit. Space separated. E.g.: github.com/packit/ogr', info='These projects will be allowed to build in this project via Packit.') }}

      </div>

    </div>

  

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

          "isolation": copr.isolation,

          "module_hotfixes": copr.module_hotfixes,

          "appstream": copr.appstream,

+         "packit_forge_projects_allowed": copr.packit_forge_projects_allowed_list,

      }

  

  
@@ -172,6 +173,7 @@ 

              follow_fedora_branching=form.follow_fedora_branching.data,

              runtime_dependencies=_form_field_repos(form.runtime_dependencies),

              appstream=form.appstream.data,

+             packit_forge_projects_allowed=_form_field_repos(form.packit_forge_projects_allowed),

          )

          db.session.commit()

      except (DuplicateException,

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

                  bootstrap=form.bootstrap.data,

                  isolation=form.isolation.data,

                  appstream=form.appstream.data,

+                 packit_forge_projects_allowed=form.packit_forge_projects_allowed.data,

              )

  

              db.session.commit()
@@ -542,6 +543,7 @@ 

      copr.bootstrap = form.bootstrap.data

      copr.isolation = form.isolation.data

      copr.appstream = form.appstream.data

+     copr.packit_forge_projects_allowed = form.packit_forge_projects_allowed.data

      if flask.g.user.admin:

          copr.auto_prune = form.auto_prune.data

      else:

@@ -169,7 +169,9 @@ 

                                runtime_dependencies=(

                                    "copr://user1/foocopr "

                                    "https://url.to/external/repo"

-                               ))

+                               ),

+                               packit_forge_projects_allowed="github.com/packit/ogr "

+                                                             "github.com/packit/requre")

          self.basic_coprs_list = [self.c1, self.c2, self.c3]

          self.db.session.add_all(self.basic_coprs_list)

  

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

          # testing method!

          already_tested = set([

              "delete_after", "build_enable_net", "auto_createrepo", "repos",

-             "runtime_dependencies",

+             "runtime_dependencies", "packit_forge_projects_allowed"

          ])

  

          # check non-trivial changes
@@ -90,11 +90,17 @@ 

          assert old_data["runtime_dependencies"] == ""

          assert old_data["auto_prune"] is True

          assert old_data["follow_fedora_branching"] is True

+         assert old_data["packit_forge_projects_allowed"] == ""

          self.api3.modify_project(

              "test", delete_after_days=5, enable_net=True, devel_mode=True,

              repos=["http://example/repo/", "http://another/"],

              runtime_dependencies=["http://run1/repo/", "http://run2/"],

              bootstrap_image="noop", appstream=True,

+             packit_forge_projects_allowed=[

+                 "https://github.com/packit/ogr",

+                 "github.com/packit/requre",

+                 "http://github.com/packit/packit"

+             ]

          )

          new_data = self._get_copr_id_data(1)

          delete_after = datetime.datetime.now() + datetime.timedelta(days=5)
@@ -105,6 +111,8 @@ 

          old_data["repos"] = "http://example/repo/\nhttp://another/"

          old_data["runtime_dependencies"] = "http://run1/repo/\nhttp://run2/"

          old_data["bootstrap"] = "default"

+         old_data["packit_forge_projects_allowed"] = "github.com/packit/ogr\ngithub.com/packit/requre\ngithub.com" \

+                                                     "/packit/packit"

          assert old_data == new_data

          old_data = new_data

  

@@ -105,6 +105,23 @@ 

          with pytest.raises(InsufficientRightsException):

              CoprsLogic.raise_if_cant_delete(self.u2, self.c2)

  

+     @pytest.mark.usefixtures("f_users", "f_coprs")

+     def test_raise_if_packit_forge_project_cant_build_in_copr(self):

+         # not defined forge project should not raise

+         CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(self.c1, None)

+ 

+         # in c1 there are no allowed forge projects

+         with pytest.raises(InsufficientRightsException):

+             CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(self.c1, "github.com/packit/ogr")

+ 

+         # in c3 github.com/packit/ogr and github.com/packit/requre are allowed

+         CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(self.c3, "github.com/packit/ogr")

+ 

+         CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(self.c3, "github.com/packit/requre")

+ 

+         with pytest.raises(InsufficientRightsException):

+             CoprsLogic.raise_if_packit_forge_project_cant_build_in_copr(self.c3, "github.com/packit/packit")

Thank you for those tests ^

+ 

      @new_app_context

      @pytest.mark.usefixtures("f_users", "f_mock_chroots", "f_db")

      def test_copr_logic_add_sends_create_gpg_key_action(self):

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

      warnings.warn("The 'use_bootstrap_container' argument is obsoleted by "

                    "'bootstrap' and 'bootstrap_image'")

  

+ 

  @for_all_methods(bind_proxy)

  class ProjectProxy(BaseProxy):

  
@@ -70,7 +71,7 @@ 

              auto_prune=True, use_bootstrap_container=None, devel_mode=False,

              delete_after_days=None, multilib=False, module_hotfixes=False,

              bootstrap=None, bootstrap_image=None, isolation=None,

-             fedora_review=None, appstream=True, runtime_dependencies=None):

+             fedora_review=None, appstream=True, runtime_dependencies=None, packit_forge_projects_allowed=None):

          """

          Create a project

  
@@ -105,6 +106,8 @@ 

          :param string runtime_dependencies: List of external repositories

              (== dependencies, specified as baseurls) that will be automatically

              enabled together with this project repository.

+         :param list packit_forge_projects_allowed: List of forge projects that

+             will be allowed to build in the project via Packit

          :return: Munch

          """

          endpoint = "/project/add/{ownername}"
@@ -133,6 +136,7 @@ 

              "fedora_review": fedora_review,

              "appstream": appstream,

              "runtime_dependencies": runtime_dependencies,

+             "packit_forge_projects_allowed": packit_forge_projects_allowed,

          }

  

          _compat_use_bootstrap_container(data, use_bootstrap_container)
@@ -151,7 +155,7 @@ 

               auto_prune=None, use_bootstrap_container=None, devel_mode=None,

               delete_after_days=None, multilib=None, module_hotfixes=None,

               bootstrap=None, bootstrap_image=None, isolation=None,

-              fedora_review=None, appstream=None, runtime_dependencies=None):

+              fedora_review=None, appstream=None, runtime_dependencies=None, packit_forge_projects_allowed=None):

          """

          Edit a project

  
@@ -182,6 +186,8 @@ 

          :param bool fedora_review: Run fedora-review tool for packages

                                     in this project

          :param bool appstream: Disable or enable generating the appstream metadata

+         :param list packit_forge_projects_allowed: List of forge projects that

+             will be allowed to build in the project via Packit

          :return: Munch

          """

          endpoint = "/project/edit/{ownername}/{projectname}"
@@ -209,6 +215,7 @@ 

              "fedora_review": fedora_review,

              "appstream": appstream,

              "runtime_dependencies": runtime_dependencies,

+             "packit_forge_projects_allowed": packit_forge_projects_allowed,

          }

  

          _compat_use_bootstrap_container(data, use_bootstrap_container)

Fixes #2216

In the UI, the projects can be set in the Other options in Project details.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto fccb0452fdc500c0d753879e66093fc0cf0cbcba

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

the pylint warnings are because of long names of the new methods and things that I used similarly as in already present codebase producing warnings as well - usage of pytest fixtures, filter inheriting from object. Should I do something about them?

Thank you for the PR!

Should I do something about them?

Preferably yes at least for those:
- Inheriting from object is not needed in frontend code (Python3 only).
- fixtures might be used by @pytest.mark.usefixtures

The long names, I don't know. I think we can keep them, @frostyx wdyt?

rebased onto dca379d4051d3ca093c84d5d2e94673a67dcef9d

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

a missing trailing space

Perhaps easier:

# remove the leading <scheme>://
result.append(item.split("://")[-1])

But maybe urlparse() and then url.netloc + url.path is slightly more powerful.

Also, space-only strings should be covered, like " ".split() => [].

Except for a few minor things, LGTM. What a non-trivial first contribution! Thank you.

rebased onto d220bf7e7956ff9b2216f60563306fbabd3350cd

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I was following how this is done for repos and runtime_dependencies and I think without this replace the data would be stored in the database with the newlines as separators since the StringListFilter adds the newlines before. But I may be wrong.

thanks for the review!

But maybe urlparse() and then url.netloc + url.path is slightly more powerful.
Also, space-only strings should be covered, like " ".split() => [].

As you wrote, I used the suggested urlparse for the schema stripping.

Regarding the space-only strings, the empty list as a result should not be a problem ('' from join will be returned), or should it?

rebased onto 5eb19a7d7f51c52243420b327e3c5d911e4427f4

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

But \n in DB isn't a bad thing... I bet that the other "replace()" occurrences are just historical mistakes...

Diving a bit deeper into the \n issue, I believe these filters should be swapped. First cleanup the strings, then strip the url schemas.

I would probably slightly prefer "\n".join() here, too.

According to my tests, this statement looks redundant. If you drop packit_forge_projects_allowed from add() arguments, the **kwargs will handle the argument properly.

Regarding the space-only strings, the empty list as a result should not be a problem ('' from join will be returned), or should it?

Ah, thats right. And even more, if you swap the two filters as suggested, you'll get the white-space issues resolved by the StringListFilter for free.

Thank you for the contribution guide fixes, btw

rebased onto e9afef9d35a19e0401a0a8bd38a4764e11454a4c

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Nit: Since we are removing protocol from the URLs before we store it into the DB, it might be good idea to test that here ^^^.

rebased onto 66bd55c9a5f2ba1993db481d50cf5a4ca6397f1c

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto afca5df

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Pull-Request has been merged by praiskup

2 years ago