Repository: packit-service/packit
Pull request #848 (2020-05-29 10:21:28)

enable overriding package config per job
Needed for https://github.com/packit-service/packit-service/issues/418

[p-s PR](https://github.com/packit-service/packit-service/pull/640)

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-848`
* 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 succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/packit-service/build/50e774a43ff74603b4f0334b946ed57e) : SUCCESS in 2m 36s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/packit-service/build/9d163278c7ef45c696f55c31c33607f5) : SUCCESS in 6m 48s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/packit-service/build/328b825a3a6e4e32960ecc5edd772821) : SUCCESS in 8m 31s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/packit-service/build/9876192c744f4867a495cd21c2214d04) : SUCCESS in 8m 39s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/330e1d5ca15b4b8f9f60d0ac01962709) : SUCCESS in 2m 29s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/48f61d009be44753b118f5f4da9756d7) : SUCCESS in 3m 01s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/0f7a5623eea4405682ac44ced9d29bda) : SUCCESS in 3m 29s
- [reverse-dep-packit-service-tests ](https://softwarefactory-project.io/zuul/t/packit-service/build/7a095d3df329443f8a0457d6555963f4) : SUCCESS in 4m 36s

TomasTomecek: > Conditional ACK if you update https://packit.dev/docs/configuration as well.

'duh
softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/packit-service/build/d6878891905c41f69c3daee8a39ed85c) : SUCCESS in 2m 33s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/packit-service/build/5957adcbfa7145269c38a4d4e9f3a05e) : SUCCESS in 8m 17s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/packit-service/build/cb19be64937649f98b33c43841f17a68) : SUCCESS in 8m 53s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/packit-service/build/5e5d154f5b9041b693e8fc7bd5cb9dce) : SUCCESS in 7m 51s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/c2da0cfa5d8445538a3bcb47dbcde587) : SUCCESS in 1m 53s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/ccc5d6949ba444acbd9c1cbd688d03ab) : SUCCESS in 2m 35s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/5f44312d0dea4bd8ac157cc3ce2ef33e) : SUCCESS in 2m 45s
- [reverse-dep-packit-service-tests ](https://softwarefactory-project.io/zuul/t/packit-service/build/7f88389ed8c347fca024178cf358d649) : SUCCESS in 3m 50s

TomasTomecek: still want to hear at least one more opinion on this since the implementation is controversial to me
TomasTomecek: >     * Looks good from the developer perspective. (But not so clean TBH.)

Yes, first I started implementing it by reusing all the package config parsing and schema but realized it would be `+1000-1000` and would be insanely complicated and fragile for a small benefit - the same validation we have in the rest of the package config. If the present implementation proves to be inefficient, we can always improve.

>     * I am not sure if users want to have `overrides` key there. They will probably want to specify those values directly. But it's better to have worse UX than nothing...;)

It's our config :D it matter what **we** want to have there, I'd say people in the end don't give a damn unless it works well.

My thinking with the overrides is:
1. One can clearly see every override in a job, they don't get boggled with the rest of the params.
2. We also have dedicated metadata key, so we're consistent.
3. It makes it much more easier to implement on our side.

Patch:
diff --git a/packit/config/job_config.py b/packit/config/job_config.py
index ce203b26..c4befbae 100644
--- a/packit/config/job_config.py
+++ b/packit/config/job_config.py
@@ -23,7 +23,7 @@
 
 from enum import Enum
 from logging import getLogger
-from typing import List, Set
+from typing import List, Set, Dict
 
 from packit.exceptions import PackitConfigException
 
@@ -117,10 +117,12 @@ class JobConfig:
         type: JobType,
         trigger: JobConfigTriggerType,
         metadata: JobMetadataConfig = None,
+        overrides: Dict = None,
     ):
         self.type: JobType = type
         self.trigger: JobConfigTriggerType = trigger
         self.metadata: JobMetadataConfig = metadata or JobMetadataConfig()
+        self.overrides: Dict = overrides or {}
 
     def __repr__(self):
         return (
diff --git a/packit/config/package_config.py b/packit/config/package_config.py
index f0fb6785..0d79d246 100644
--- a/packit/config/package_config.py
+++ b/packit/config/package_config.py
@@ -22,6 +22,7 @@
 
 import json
 import logging
+from copy import deepcopy
 from pathlib import Path
 from typing import Optional, List, Dict, Union
 
@@ -118,6 +119,8 @@ class PackageConfig:
         ]
         # template to create an upstream tag name (upstream may use different tagging scheme)
         self.upstream_tag_template = upstream_tag_template
+        # so we can reuse it for job.overrides
+        self.raw_dict: Optional[Dict] = None
 
         if kwargs:
             logger.warning(f"Following kwargs were not processed:" f"{kwargs}")
@@ -146,6 +149,11 @@ class PackageConfig:
             f"patch_generation_ignore_paths='{self.patch_generation_ignore_paths}')"
         )
 
+    def set_job_overrides(self, job: JobConfig):
+        d = deepcopy(self.raw_dict)
+        d.update(job.overrides)
+        self.__dict__ = PackageConfig.get_from_dict(d).__dict__
+
     @property
     def downstream_project_url(self) -> str:
         if not self._downstream_project_url:
@@ -190,6 +198,7 @@ class PackageConfig:
         if "jobs" not in raw_dict:
             package_config.jobs = default_jobs
 
+        package_config.raw_dict = raw_dict
         logger.debug(package_config)
         return package_config
 
diff --git a/packit/schema.py b/packit/schema.py
index ee160065..f427795b 100644
--- a/packit/schema.py
+++ b/packit/schema.py
@@ -238,6 +238,7 @@ class JobConfigSchema(Schema):
     job = EnumField(JobType, required=True)
     trigger = EnumField(JobConfigTriggerType, required=True)
     metadata = fields.Nested(JobMetadataSchema)
+    overrides = fields.Dict()
 
     @post_load
     def make_instance(self, data, **_):


Test patch:
diff --git a/tests/unit/test_package_config.py b/tests/unit/test_package_config.py
index 342d57b6..6b55a75a 100644
--- a/tests/unit/test_package_config.py
+++ b/tests/unit/test_package_config.py
@@ -591,6 +591,117 @@ def test_package_config_parse(raw, expected):
     assert package_config == expected
 
 
+@pytest.mark.parametrize(
+    "raw,expected",
+    [
+        pytest.param(
+            {
+                "specfile_path": "fedora/package.spec",
+                "jobs": [
+                    {
+                        "job": "build",
+                        "trigger": "release",
+                        "overrides": {"specfile_path": "somewhere/package.spec"},
+                    }
+                ],
+            },
+            PackageConfig(
+                specfile_path="somewhere/package.spec",
+                jobs=[
+                    JobConfig(
+                        type=JobType.build,
+                        trigger=JobConfigTriggerType.release,
+                        overrides={"specfile_path": "somewhere/package.spec"},
+                    )
+                ],
+            ),
+            id="override-specfile_path",
+        ),
+        pytest.param(
+            {
+                "specfile_path": "fedora/package.spec",
+                "synced_files": ["x", "y"],
+                "actions": {"post-upstream-clone": "ls"},
+                "jobs": [
+                    {
+                        "job": "build",
+                        "trigger": "release",
+                        "overrides": {
+                            "specfile_path": "somewhere/package.spec",
+                            "synced_files": ["a", "b", "c"],
+                            "actions": {"create-archive": "ls"},
+                        },
+                    }
+                ],
+            },
+            PackageConfig(
+                specfile_path="somewhere/package.spec",
+                synced_files=SyncFilesConfig(
+                    [SyncFilesItem(x, x) for x in ("a", "b", "c")]
+                ),
+                actions={ActionName.create_archive: "ls"},
+                jobs=[
+                    JobConfig(
+                        type=JobType.build,
+                        trigger=JobConfigTriggerType.release,
+                        overrides={
+                            "specfile_path": "somewhere/package.spec",
+                            "synced_files": ["a", "b", "c"],
+                            "actions": {"create-archive": "ls"},
+                        },
+                    )
+                ],
+            ),
+            id="override-alot",
+        ),
+    ],
+)
+def test_package_config_overrides(raw, expected):
+    package_config = PackageConfig.get_from_dict(raw_dict=raw)
+    package_config.set_job_overrides(package_config.jobs[0])
+    assert package_config == expected
+
+
+@pytest.mark.parametrize(
+    "raw,err_message",
+    [
+        pytest.param(
+            {
+                "specfile_path": "fedora/package.spec",
+                "jobs": [
+                    {
+                        "job": "build",
+                        "trigger": "release",
+                        "overrides": {"actions": ["a"]},
+                    }
+                ],
+            },
+            "'dict' required, got <class 'list'>.",
+            id="bad_actions",
+        ),
+        pytest.param(
+            {
+                "specfile_path": "fedora/package.spec",
+                "jobs": [
+                    {
+                        "job": "build",
+                        "trigger": "release",
+                        "overrides": {"synced_files": "a"},
+                    }
+                ],
+            },
+            "Invalid input type.",
+            id="bad_synced_files",
+        ),
+    ],
+)
+def test_package_config_overrides_bad(raw, err_message):
+    package_config = PackageConfig.get_from_dict(raw_dict=raw)
+    with pytest.raises(ValidationError) as ex:
+        package_config.set_job_overrides(package_config.jobs[0])
+    assert err_message in str(ex)
+
+
 @pytest.mark.parametrize(
     "raw,expected",
     [