Merge pull request #479 from IceWreck/psql-api

Rewrite the copr-builds part of the API using PostgresSQL instead of Redis

Reviewed-by: https://github.com/apps/softwarefactory-project-zuul

alembic/versions/61d0f32eda4b_added_project_name_and_git_fields.py

@@ -0,0 +1,32 @@
+"""added project name and git fields
+
+Revision ID: 61d0f32eda4b
+Revises: 70dcb6140e11
+Create Date: 2020-03-10 19:11:36.729552
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = "61d0f32eda4b"
+down_revision = "70dcb6140e11"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column("git_projects", sa.Column("https_url", sa.String(), nullable=True))
+    op.add_column("copr_builds", sa.Column("project_name", sa.String(), nullable=True))
+    op.add_column("copr_builds", sa.Column("owner", sa.String(), nullable=True))
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column("copr_builds", "owner")
+    op.drop_column("copr_builds", "project_name")
+    op.drop_column("git_projects", "https_url")
+    # ### end Alembic commands ###

packit_service/models.py

@@ -27,7 +27,7 @@
 import os
 from contextlib import contextmanager
 from datetime import datetime
-from typing import TYPE_CHECKING, Optional, Union
+from typing import TYPE_CHECKING, Optional, Union, Iterable
 
 from sqlalchemy import Column, Integer, String, DateTime, ForeignKey, Text
 from sqlalchemy import JSON, create_engine
@@ -86,6 +86,10 @@ class GitProject(Base):
     repo_name = Column(String, index=True)
     pull_requests = relationship("PullRequest", back_populates="project")
 
+    # Git URL of the repo
+    # Example: https://github.com/packit-service/hello-world.git
+    https_url = Column(String)
+
     @classmethod
     def get_or_create(cls, namespace: str, repo_name: str) -> "GitProject":
         with get_sa_session() as session:
@@ -169,6 +173,11 @@ class CoprBuild(Base):
     build_submitted_time = Column(DateTime, default=datetime.utcnow)
     build_start_time = Column(DateTime)
     build_finished_time = Column(DateTime)
+
+    # project name as shown in copr
+    project_name = Column(String)
+    owner = Column(String)
+
     # metadata for the build which didn't make it to schema yet
     # metadata is reserved to sqlalch
     data = Column(JSON)
@@ -188,6 +197,23 @@ def get_by_id(cls, id_: int) -> Optional["CoprBuild"]:
         with get_sa_session() as session:
             return session.query(CoprBuild).filter_by(id=id_).first()
 
+    @classmethod
+    def get_all(cls) -> Optional[Iterable["CoprBuild"]]:
+        with get_sa_session() as session:
+            return session.query(CoprBuild).all()
+
+    # Returns all builds with that build_id, irrespective of target
+    @classmethod
+    def get_all_build_id(
+        cls, build_id: Union[str, int]
+    ) -> Optional[Iterable["CoprBuild"]]:
+        if isinstance(build_id, int):
+            # See the comment in get_by_build_id()
+            build_id = str(build_id)
+        with get_sa_session() as session:
+            return session.query(CoprBuild).filter_by(build_id=build_id)
+
+    # returns the build matching the build_id and the target
     @classmethod
     def get_by_build_id(
         cls, build_id: Union[str, int], target: str
@@ -213,6 +239,8 @@ def get_or_create(
         commit_sha: str,
         repo_name: str,
         namespace: str,
+        project_name: str,
+        owner: str,
         web_url: str,
         target: str,
         status: str,
@@ -229,6 +257,8 @@ def get_or_create(
                 build.pr_id = pr.id
                 build.srpm_build_id = srpm_build.id
                 build.status = status
+                build.project_name = project_name
+                build.owner = owner
                 build.commit_sha = commit_sha
                 build.web_url = web_url
                 build.target = target

packit_service/service/api/copr_builds.py

@@ -25,10 +25,9 @@
 
 from flask import make_response
 from flask_restplus import Namespace, Resource
-from persistentdict.dict_in_redis import PersistentDict
 
 from packit_service.service.api.parsers import indices, pagination_arguments
-from packit_service.service.models import CoprBuild, LAST_PK
+from packit_service.models import CoprBuild
 
 logger = getLogger("packit_service")
 
@@ -41,21 +40,44 @@ class CoprBuildsList(Resource):
     @ns.expect(pagination_arguments)
     @ns.response(HTTPStatus.PARTIAL_CONTENT, "Copr builds list follows")
     def get(self):
-        """ List all Copr builds. From 'copr-builds' hash, filled by service. """
-        # I know it's expensive to first convert whole dict to a list and then slice the list,
-        # but how else would you slice a dict, huh?
+        """ List all Copr builds. """
+
+        # Return relevant info thats concise
+        # Usecases like the packit-dashboard copr-builds table
+
+        builds_list = CoprBuild.get_all()
         result = []
-        for k, cb in CoprBuild.db().get_all().items():
-            if k == LAST_PK:
-                # made-up keys, remove LAST_PK
-                continue
-            cb.pop("identifier", None)  # see PR#179
-            result.append(cb)
+        checklist = []
+        for build in builds_list:
+            if int(build.build_id) not in checklist:
+                build_dict = {
+                    "project": build.project_name,
+                    "owner": build.owner,
+                    "repo_name": build.pr.project.repo_name,
+                    "build_id": build.build_id,
+                    "status": build.status,
+                    "chroots": [],
+                    "build_submitted_time": build.build_submitted_time.strftime(
+                        "%d/%m/%Y %H:%M:%S"
+                    ),
+                    "repo_namespace": build.pr.project.namespace,
+                    "web_url": build.web_url,
+                }
+                # same_buildid_builds are copr builds created due to the same trigger
+                # multiple identical builds are created which differ only in target
+                # so we merge them into one
+                same_buildid_builds = CoprBuild.get_all_build_id(str(build.build_id))
+                for sbid_build in same_buildid_builds:
+                    build_dict["chroots"].append(sbid_build.target)
+
+                checklist.append(int(build.build_id))
+                result.append(build_dict)
 
         first, last = indices()
         resp = make_response(dumps(result[first:last]), HTTPStatus.PARTIAL_CONTENT)
         resp.headers["Content-Range"] = f"copr-builds {first + 1}-{last}/{len(result)}"
         resp.headers["Content-Type"] = "application/json"
+
         return resp
 
 
@@ -66,7 +88,40 @@ class InstallationItem(Resource):
     @ns.response(HTTPStatus.NO_CONTENT, "Copr build identifier not in db/hash")
     def get(self, id):
         """A specific copr build details. From copr_build hash, filled by worker."""
-        # hash name is defined in worker (CoprBuildDB), which I don't want to import from
-        db = PersistentDict(hash_name="copr_build")
-        build = db.get(id)
-        return build if build else ("", HTTPStatus.NO_CONTENT)
+        builds_list = CoprBuild.get_all_build_id(str(id))
+        if bool(builds_list.first()):
+            build = builds_list[0]
+            build_dict = {
+                "project": build.project_name,
+                "owner": build.owner,
+                "repo_name": build.pr.project.repo_name,
+                "build_id": build.build_id,
+                "status": build.status,
+                "chroots": [],
+                "build_submitted_time": build.build_submitted_time.strftime(
+                    "%d/%m/%Y %H:%M:%S"
+                ),
+                "build_start_time": build.build_start_time,
+                "build_finished_time": build.build_finished_time,
+                "pr_id": build.pr.pr_id,
+                "commit_sha": build.commit_sha,
+                "repo_namespace": build.pr.project.namespace,
+                "web_url": build.web_url,
+                "srpm_logs": build.srpm_build.logs,
+                "git_repo": f"https://github.com/{build.pr.project.namespace}"
+                "{build.pr.project.repo_name}",
+                # For backwards compatability with the redis API
+                "ref": build.commit_sha,
+                "https_url": f"https://github.com/{build.pr.project.namespace}/"
+                "{build.pr.project.repo_name}.git",
+            }
+            # merge chroots into one
+            for sbid_build in builds_list:
+                build_dict["chroots"].append(sbid_build.target)
+
+            build = make_response(dumps(build_dict))
+            build.headers["Content-Type"] = "application/json"
+            return build if build else ("", HTTPStatus.NO_CONTENT)
+
+        else:
+            return ("", HTTPStatus.NO_CONTENT)

packit_service/worker/build/copr_build.py

@@ -159,6 +159,8 @@ def run_copr_build(self) -> HandlerResults:
                 commit_sha=self.event.commit_sha,
                 repo_name=self.project.repo,
                 namespace=self.project.namespace,
+                project_name=self.job_project,
+                owner=self.job_owner,
                 web_url=build_metadata.copr_web_url,
                 target=chroot,
                 status="pending",

tests_requre/test_db.py

@@ -53,6 +53,7 @@ def clean_db():
         session.query(GitProject).delete()
 
 
+# Create a single build
 @pytest.fixture()
 def a_copr_build():
     with get_sa_session() as session:
@@ -64,6 +65,8 @@ def a_copr_build():
             commit_sha="687abc76d67d",
             repo_name="lithium",
             namespace="nirvana",
+            project_name="SomeUser-hello-world-9",
+            owner="packit",
             web_url="https://copr.something.somewhere/123456",
             target=TARGET,
             status="pending",
@@ -72,13 +75,69 @@ def a_copr_build():
     clean_db()
 
 
+# Create multiple builds
+# Used for testing querys
+@pytest.fixture()
+def multiple_copr_builds():
+    with get_sa_session() as session:
+        session.query(CoprBuild).delete()
+        srpm_build = SRPMBuild.create("asd\nqwe\n")
+        yield [
+            CoprBuild.get_or_create(
+                pr_id=1,
+                build_id="123456",
+                commit_sha="687abc76d67d",
+                repo_name="lithium",
+                namespace="nirvana",
+                project_name="SomeUser-hello-world-9",
+                owner="packit",
+                web_url="https://copr.something.somewhere/123456",
+                target="fedora-42-x86_64",
+                status="pending",
+                srpm_build=srpm_build,
+            ),
+            # Same build_id but different chroot
+            CoprBuild.get_or_create(
+                pr_id=1,
+                build_id="123456",
+                commit_sha="687abc76d67d",
+                repo_name="lithium",
+                namespace="nirvana",
+                project_name="SomeUser-hello-world-9",
+                owner="packit",
+                web_url="https://copr.something.somewhere/123456",
+                target="fedora-43-x86_64",
+                status="pending",
+                srpm_build=srpm_build,
+            ),
+            # Completely different build
+            CoprBuild.get_or_create(
+                pr_id=4,
+                build_id="987654",
+                commit_sha="987def76d67e",
+                repo_name="cockpit-project",
+                namespace="cockpit",
+                project_name="SomeUser-random-text-7",
+                owner="cockpit-project",
+                web_url="https://copr.something.somewhere/987654",
+                target="fedora-43-x86_64",
+                status="pending",
+                srpm_build=srpm_build,
+            ),
+        ]
+
+    clean_db()
+
+
 def test_create_copr_build(a_copr_build):
     assert a_copr_build.pr_id == a_copr_build.pr.id
     assert a_copr_build.pr.pr_id == 1
     assert a_copr_build.build_id == "123456"
     assert a_copr_build.commit_sha == "687abc76d67d"
     assert a_copr_build.pr.project.namespace == "nirvana"
     assert a_copr_build.pr.project.repo_name == "lithium"
+    assert a_copr_build.project_name == "SomeUser-hello-world-9"
+    assert a_copr_build.owner == "packit"
     assert a_copr_build.web_url == "https://copr.something.somewhere/123456"
     assert a_copr_build.srpm_build.logs == "asd\nqwe\n"
     assert a_copr_build.target == TARGET
@@ -155,3 +214,34 @@ def test_errors_while_doing_db():
             assert len(session.query(PullRequest).all()) == 1
         finally:
             clean_db()
+
+
+# return all builds in table
+def test_get_all(multiple_copr_builds):
+    builds_list = CoprBuild.get_all()
+    assert len(builds_list) == 3
+    # we just wanna check if result is iterable
+    # order doesn't matter, so all of them are set to pending in supplied data
+    assert builds_list[1].status == "pending"
+
+
+# return all builds with given build_id
+def test_get_all_build_id(multiple_copr_builds):
+    builds_list = CoprBuild.get_all_build_id(str(123456))
+    assert len(list(builds_list)) == 2
+    # both should have the same project_name
+    assert builds_list[1].project_name == builds_list[0].project_name
+    assert builds_list[1].project_name == "SomeUser-hello-world-9"
+
+
+# returns the first build with given build id and target
+def test_get_by_build_id(multiple_copr_builds):
+    # these are not iterable and thus should be accessible directly
+    build_a = CoprBuild.get_by_build_id(str(123456), "fedora-42-x86_64")
+    assert build_a.project_name == "SomeUser-hello-world-9"
+    assert build_a.target == "fedora-42-x86_64"
+    build_b = CoprBuild.get_by_build_id(str(123456), "fedora-43-x86_64")
+    assert build_b.project_name == "SomeUser-hello-world-9"
+    assert build_b.target == "fedora-43-x86_64"
+    build_c = CoprBuild.get_by_build_id(str(987654), "fedora-43-x86_64")
+    assert build_c.project_name == "SomeUser-random-text-7"