Repository: packit-service/packit
Pull request #746 (2020-03-04 16:36:32)

Split job and jobconfig (packit)
- Split the JobTriggerType to one related to config and one for actual triggers.

softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/587990f7e1b141779768e95cc0217eea) : SUCCESS in 3m 21s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/98d01184b4514c0aa7a52680b86a6955) : SUCCESS in 7m 07s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/9b8c0ea1373d4b0c82f968e307a06285) : SUCCESS in 10m 14s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/81d9dec38a144b4abe29fc21bee19fbb) : SUCCESS in 7m 46s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/5acdc81777044588a228b5bde37ff917) : SUCCESS in 2m 25s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/422612abe84c40d2a3b89ed88cbd7533) : SUCCESS in 4m 46s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/efc33093c1484e418b371fba7a84d97c) : SUCCESS in 3m 58s

packit-as-a-service[bot]: Congratulations! One of the builds has completed. :champagne:

You can install the built RPMs by following these steps:

* `sudo yum install -y dnf-plugins-core` on RHEL 8
* `sudo dnf install -y dnf-plugins-core` on Fedora
* `dnf copr enable packit/packit-service-packit-746`
* And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.
softwarefactory-project-zuul[bot]: Build failed.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/745ff2abe18348109f36c18e5046767e) : SUCCESS in 3m 41s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/43f65da15da44de5a9877449c131677f) : SUCCESS in 8m 04s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/45d20a88df6e41b8a833c4ab21cdfb59) : SUCCESS in 6m 40s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/b84ec508f8714ac2b81171cefe316765) : FAILURE in 7m 47s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/8b9727d4fd5446efb85824f14e4d00be) : SUCCESS in 2m 24s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/b8d9d14cce904654a8ae811c513c373d) : SUCCESS in 3m 09s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/629c2bd15c754bc899100662fc91014f) : SUCCESS in 3m 29s

lachmanfrantisek: Do not merge this before we are ready in the p-s pull-request otherwise, the p-s code will be broken. (Ideally, merge this one, retest the p-s one and merge it in a short period.)
softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/fc9a33600fe3432c9e530fa48420305e) : SUCCESS in 3m 02s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/118f017554cc49b1b084dc0fba7ff915) : SUCCESS in 6m 41s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/d3fd2f89a06d4ddd90dae173da2dd502) : SUCCESS in 7m 27s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/bbf16c953a304131b507d752a04b6a7b) : SUCCESS in 7m 42s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/3f1e35619ea844ae9426602b46601cd9) : SUCCESS in 2m 46s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/c007efc7cb1348eeb52a173abde4d6d3) : SUCCESS in 3m 14s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/df11e9241f5a4254ae8910eb3af5319a) : SUCCESS in 3m 16s

csomh: I'm having difficulties understanding the context of this change: why is it needed, and why is it done the way it is?

Could you capture some of these details in the commit message. (See [this](https://chris.beams.io/posts/git-commit/#why-not-how) explanation, why this is nice to have).
lachmanfrantisek: > Could you capture some of these details in the commit message. (See [this](https://chris.beams.io/posts/git-commit/#why-not-how) explanation, why this is nice to have).

I don't want to have it in the commit since it's cross-project change and complex and I am not sure if it will be useful to have such long message in the commit.

The context (which I wanted to discuss on the ar-meeting):
- By now, we have `JobTriggerType` which is used in the config as well as for the events we are using in the packit-service.
- Some triggers cannot be configured by users (e.g. CoprBuildEnd,..)
- Problem 1: We are using one enum for multiple purposes.
- Problem 2: It's hard to get the right config for the "fake" triggers.
- Problem 3: Current workflow does not support multiple triggers for one action.
- Here is a first part of the solution to the problems -- split the enum to two.


> (See [this](https://chris.beams.io/posts/git-commit/#why-not-how)
Nice article, I know about it.
csomh: > > Could you capture some of these details in the commit message. (See [this](https://chris.beams.io/posts/git-commit/#why-not-how) explanation, why this is nice to have).
> 
> I don't want to have it in the commit since it's cross-project change and complex and I am not sure if it will be useful to have such long message in the commit.

These details should be captured in the commit message especially because there are complex and cross-project.

As for the usefulness:

1. we would have saved a lot of typing time during this review, if this information would have been in the commit message (I was expecting it and looking for it) :smiley: 
2. could have helped folks not aware of this change coming up (me :confused: ) to have a meaningful contribution to the review process while also catching up with what's going on.
3. will potentially save a lot of time for someone in the future, trying to re-establish the context of this change in order to fix some issue.
4. good commit messages will help us immensely with building a community of contributors around Packit, as it will be much easier for outside contributors to understand how the project evolved, without us having to spend super much extra effort to keep some separate set of docs up to date and relevant.
5. the folks writing the blog posts on Fridays wouldn't need to spend hours reading through all the diffs of all the changes during the week trying to figure out why things happened, so that they can summarise it.

> 
> The context (which I wanted to discuss on the arch-meeting):
> 
> * By now, we have `JobTriggerType` which is used in the config as well as for the events we are using in the packit-service.
> * Some triggers cannot be configured by users (e.g. CoprBuildEnd,..)
> * Problem 1: We are using one enum for multiple purposes.
> * Problem 2: It's hard to get the right config for the "fake" triggers.
> * Problem 3: Current workflow does not support multiple triggers for one action.
> * Here is a first part of the solution to the problems -- split the enum to two.

Very nice explanation, thanks!

> 
> > (See [this](https://chris.beams.io/posts/git-commit/#why-not-how)
> 
> Nice article, I know about it.

lachmanfrantisek: > These details should be captured in the commit message especially because there are complex and cross-project.

complex +1, cross-project -1

I will add the content of the comment to the git message.

Patch:
diff --git a/packit/config/__init__.py b/packit/config/__init__.py
index 0ee494b3..48046c3b 100644
--- a/packit/config/__init__.py
+++ b/packit/config/__init__.py
@@ -5,7 +5,11 @@ from packit.config.config import (
     get_default_map_from_file,
     pass_config,
 )
-from packit.config.job_config import JobConfig, JobTriggerType, JobType
+from packit.config.job_config import (
+    JobConfig,
+    JobConfigTriggerType,
+    JobType,
+)
 from packit.config.package_config import (
     PackageConfig,
     get_package_config_from_repo,
@@ -21,7 +25,7 @@ from packit.config.sync_files_config import (
 __all__ = [
     Config.__name__,
     JobConfig.__name__,
-    JobTriggerType.__name__,
+    JobConfigTriggerType.__name__,
     JobType.__name__,
     PackageConfig.__name__,
     RawSyncFilesItem.__name__,
diff --git a/packit/config/job_config.py b/packit/config/job_config.py
index 036d1b98..1e7fab74 100644
--- a/packit/config/job_config.py
+++ b/packit/config/job_config.py
@@ -36,6 +36,7 @@ class JobType(Enum):
     build = "build"
     sync_from_downstream = "sync_from_downstream"
     copr_build = "copr_build"
+    production_build = "production_build"  # koji build
     add_to_whitelist = "add_to_whitelist"
     tests = "tests"
     report_test_results = "report_test_results"
@@ -44,24 +45,21 @@ class JobType(Enum):
     copr_build_started = "copr_build_started"
 
 
-class JobTriggerType(Enum):
+class JobConfigTriggerType(Enum):
     release = "release"
     pull_request = "pull_request"
     commit = "commit"
-    installation = "installation"
-    testing_farm_results = "testing_farm_results"
-    comment = "comment"
 
 
 class JobConfig:
-    def __init__(self, job: JobType, trigger: JobTriggerType, metadata: dict):
-        self.job = job
-        self.trigger = trigger
-        self.metadata = metadata
+    def __init__(self, type: JobType, trigger: JobConfigTriggerType, metadata: dict):
+        self.type = type
+        self.trigger: JobConfigTriggerType = trigger
+        self.metadata: dict = metadata
 
     def __repr__(self):
         return (
-            f"JobConfig(job={self.job}, trigger={self.trigger}, meta={self.metadata})"
+            f"JobConfig(job={self.type}, trigger={self.trigger}, meta={self.metadata})"
         )
 
     @classmethod
@@ -75,7 +73,7 @@ class JobConfig:
         if not isinstance(other, JobConfig):
             raise PackitConfigException("Provided object is not a JobConfig instance.")
         return (
-            self.job == other.job
+            self.type == other.type
             and self.trigger == other.trigger
             and self.metadata == other.metadata
         )
@@ -83,13 +81,13 @@ class JobConfig:
 
 default_jobs = [
     JobConfig(
-        job=JobType.tests,
-        trigger=JobTriggerType.pull_request,
+        type=JobType.tests,
+        trigger=JobConfigTriggerType.pull_request,
         metadata={"targets": ["fedora-stable"]},
     ),
     JobConfig(
-        job=JobType.propose_downstream,
-        trigger=JobTriggerType.release,
+        type=JobType.propose_downstream,
+        trigger=JobConfigTriggerType.release,
         metadata={"dist-git-branch": ["fedora-all"]},
     ),
 ]
diff --git a/packit/schema.py b/packit/schema.py
index 5d5ce14e..7c6c4c2e 100644
--- a/packit/schema.py
+++ b/packit/schema.py
@@ -25,11 +25,19 @@ import typing
 
 from marshmallow import Schema, fields, post_load, pre_load, ValidationError
 from marshmallow_enum import EnumField
+
 from packit.actions import ActionName
 from packit.config import PackageConfig, Config, SyncFilesConfig
-from packit.config.job_config import JobType, JobTriggerType, JobConfig, default_jobs
-from packit.config.package_config import NotificationsConfig
-from packit.config.package_config import PullRequestNotificationsConfig
+from packit.config.job_config import (
+    JobType,
+    JobConfig,
+    default_jobs,
+    JobConfigTriggerType,
+)
+from packit.config.package_config import (
+    NotificationsConfig,
+    PullRequestNotificationsConfig,
+)
 from packit.sync import SyncFilesItem
 
 logger = logging.getLogger(__name__)
@@ -108,6 +116,8 @@ class NotProcessedField(fields.Field):
     """
     Field class to mark fields which wil not be processed, only generates warning.
     Can be passed additional message via additional_message parameter.
+
+    :param str additional_message: additional warning message to be displayed
     """
 
     def _serialize(self, value: typing.Any, attr: str, obj: typing.Any, **kwargs):
@@ -159,12 +169,13 @@ class JobConfigSchema(Schema):
     """
 
     job = EnumField(JobType, required=True)
-    trigger = EnumField(JobTriggerType, required=True)
+    trigger = EnumField(JobConfigTriggerType, required=True)
     metadata = fields.Dict(missing={})
 
     @post_load
-    def make_instance(self, data, **kwargs):
-        return JobConfig(**data)
+    def make_instance(self, data, **_):
+        type = data.pop("job")
+        return JobConfig(type=type, **data)
 
 
 class PullRequestNotificationsSchema(Schema):


Test patch:
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index 498eb3a4..4ad72424 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -33,12 +33,12 @@ from packit.actions import ActionName
 from packit.config import (
     Config,
     JobConfig,
-    JobTriggerType,
     JobType,
     PackageConfig,
     get_package_config_from_repo,
     SyncFilesConfig,
     SyncFilesItem,
+    JobConfigTriggerType,
 )
 from packit.config.package_config import get_local_specfile_path
 from tests.spellbook import UP_OSBUILD, SYNC_FILES
@@ -57,13 +57,15 @@ def get_job_config_dict_full():
 
 
 def get_job_config_simple():
-    return JobConfig(job=JobType.build, trigger=JobTriggerType.release, metadata={})
+    return JobConfig(
+        type=JobType.build, trigger=JobConfigTriggerType.release, metadata={}
+    )
 
 
 def get_job_config_full():
     return JobConfig(
-        job=JobType.propose_downstream,
-        trigger=JobTriggerType.pull_request,
+        type=JobType.propose_downstream,
+        trigger=JobConfigTriggerType.pull_request,
         metadata={"a": "b"},
     )
 
@@ -71,13 +73,13 @@ def get_job_config_full():
 def get_default_job_config():
     return [
         JobConfig(
-            job=JobType.tests,
-            trigger=JobTriggerType.pull_request,
+            type=JobType.tests,
+            trigger=JobConfigTriggerType.pull_request,
             metadata={"targets": ["fedora-stable"]},
         ),
         JobConfig(
-            job=JobType.propose_downstream,
-            trigger=JobTriggerType.release,
+            type=JobType.propose_downstream,
+            trigger=JobConfigTriggerType.release,
             metadata={"dist-git-branch": ["fedora-all"]},
         ),
     ]
@@ -515,7 +517,7 @@ def test_package_config_parse(raw, expected):
     # tests for https://github.com/packit-service/packit-service/pull/342
     if expected.jobs:
         for j in package_config.jobs:
-            assert j.job
+            assert j.type
     assert package_config == expected