fix: always add branch in gh pages changes (#450)

* fix: always add branch in gh pages changes

applying changes to gh pages always requires the source branch in the
payload.

this change make sure that source branch is always added

Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>

* fixup! fix: always add branch in gh pages changes

Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>

* test: add full coverage for new function

Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>

---------

Signed-off-by: Kairo Araujo <kairo@dearaujo.nl>

Author: kairoaraujo

otterdog/models/repository.py

@@ -1050,13 +1050,6 @@ def generate_live_patch(
                         web_commit_signoff_required, web_commit_signoff_required
                     )
 
-            # FIXME: needed to add this hack to ensure that gh_pages_source_path is also present in
-            #        the modified data as GitHub needs the path as well when the branch is changed.
-            #        this needs to make clean to support making the diff operation generic as possible.
-            if "gh_pages_source_branch" in modified_repo:
-                gh_pages_source_path = cast(Repository, coerced_object).gh_pages_source_path
-                modified_repo["gh_pages_source_path"] = Change(gh_pages_source_path, gh_pages_source_path)
-
             # similar fix as above for squash_merge_commit_title and squash_merge_commit_message as well
             squash_merge_commit_title_present = "squash_merge_commit_title" in modified_repo
             squash_merge_commit_message_present = "squash_merge_commit_message" in modified_repo
@@ -1156,6 +1149,26 @@ def generate_live_patch(
                 handler,
             )
 
+    @staticmethod
+    def _include_gh_pages_patch_required_properties(patch: LivePatch[Repository]) -> LivePatch[Repository]:
+        """
+        Ensure that the gh_pages_source_branch and gh_pages_source_path are set when
+        the patch contains gh_pages change.
+        """
+        if patch.changes:
+            gh_pages_source_branch_present = "gh_pages_source_branch" in patch.changes
+            gh_pages_source_path_present = "gh_pages_source_path" in patch.changes
+
+            if gh_pages_source_path_present and not gh_pages_source_branch_present:
+                gh_pages_source_branch = cast(Repository, patch.current_object).gh_pages_source_branch
+                patch.changes["gh_pages_source_branch"] = Change(gh_pages_source_branch, gh_pages_source_branch)
+
+            if gh_pages_source_branch_present and not gh_pages_source_path_present:
+                gh_pages_source_path = cast(Repository, patch.current_object).gh_pages_source_path
+                patch.changes["gh_pages_source_path"] = Change(gh_pages_source_path, gh_pages_source_path)
+
+        return patch
+
     @classmethod
     async def apply_live_patch(
         cls,
@@ -1188,6 +1201,7 @@ async def apply_live_patch(
                 await provider.delete_repo(org_id, unwrap(patch.current_object).name)
 
             case LivePatchType.CHANGE:
+                cls._include_gh_pages_patch_required_properties(patch)
                 expected_object = unwrap(patch.expected_object)
                 github_settings = await cls.changes_to_provider(org_id, unwrap(patch.changes), provider)
 

poetry.lock

@@ -112,6 +112,7 @@ pytest = ">=8.0"
 pytest-asyncio = ">=0.24"
 parameterized  = ">=0.9"
 pytest-cov = ">=6.1.1"
+pretend = "^1.0.9"
 
 [tool.poetry.group.typing.dependencies]
 mypy = ">=1.8.0"

pyproject.toml

@@ -0,0 +1,30 @@
+from collections.abc import Mapping
+from typing import Any
+
+import pytest
+
+from otterdog.jsonnet import JsonnetConfig
+from otterdog.models import ModelObject
+from otterdog.models.repository import Repository
+from tests.models import ModelTest
+
+
+@pytest.fixture()
+def repository_test():
+    class RepositoryTest(ModelTest):
+        def create_model(self, data: Mapping[str, Any]) -> ModelObject:
+            return Repository.from_model_data(data)
+
+        @property
+        def template_function(self) -> str:
+            return JsonnetConfig.create_repo
+
+        @property
+        def model_data(self):
+            return self.load_json_resource("otterdog-repo.json")
+
+        @property
+        def provider_data(self):
+            return self.load_json_resource("github-repo.json")
+
+    return RepositoryTest()

tests/conftest.py

@@ -22,5 +22,7 @@
   "allow_auto_merge": false,
   "merge_commit_message": "PR_TITLE",
   "secret_scanning": "enabled",
-  "dependabot_alerts_enabled": true
+  "dependabot_alerts_enabled": true,
+  "gh_pages_source_path": "/",
+  "gh_pages_source_branch": "gh-pages"
 }

tests/models/resources/otterdog-repo.json

@@ -6,35 +6,16 @@
 #  SPDX-License-Identifier: EPL-2.0
 #  *******************************************************************************
 
-from collections.abc import Mapping
-from typing import Any
+import pretend
+import pytest
 
-from otterdog.jsonnet import JsonnetConfig
-from otterdog.models import ModelObject
-from otterdog.models.repository import Repository
+from otterdog.models.repository import LivePatch, Repository
 from otterdog.utils import UNSET, Change, query_json
 
-from . import ModelTest
 
-
-class RepositoryTest(ModelTest):
-    def create_model(self, data: Mapping[str, Any]) -> ModelObject:
-        return Repository.from_model_data(data)
-
-    @property
-    def template_function(self) -> str:
-        return JsonnetConfig.create_repo
-
-    @property
-    def model_data(self):
-        return self.load_json_resource("otterdog-repo.json")
-
-    @property
-    def provider_data(self):
-        return self.load_json_resource("github-repo.json")
-
-    def test_load_from_model(self):
-        repo = Repository.from_model_data(self.model_data)
+class TestRepository:
+    def test_load_from_model(self, repository_test):
+        repo = Repository.from_model_data(repository_test.model_data)
 
         assert repo.name == "otterdog-defaults"
         assert repo.description is None
@@ -65,8 +46,8 @@ def test_load_from_model(self):
         assert repo.post_process_template_content == []
         assert repo.auto_init is False
 
-    def test_load_from_provider(self):
-        repo = Repository.from_provider_data(self.org_id, self.provider_data)
+    def test_load_from_provider(self, repository_test):
+        repo = Repository.from_provider_data(repository_test.org_id, repository_test.provider_data)
 
         assert repo.id == 605555050
         assert repo.node_id == "R_kgDOJBgJag"
@@ -95,22 +76,22 @@ def test_load_from_provider(self):
         assert repo.secret_scanning_push_protection == "disabled"
         assert repo.dependabot_alerts_enabled is True
 
-    async def test_to_provider(self):
-        repo = Repository.from_model_data(self.model_data)
+    async def test_to_provider(self, repository_test):
+        repo = Repository.from_model_data(repository_test.model_data)
 
         repo.description = UNSET
 
-        provider_data = await repo.to_provider_data(self.org_id, self.provider)
+        provider_data = await repo.to_provider_data(repository_test.org_id, repository_test.provider)
 
         assert len(provider_data) == 22
         assert provider_data["name"] == "otterdog-defaults"
         assert provider_data.get("description") is None
 
         assert query_json("security_and_analysis.secret_scanning.status", provider_data) or "" == "enabled"
 
-    async def test_changes_to_provider(self):
-        current = Repository.from_model_data(self.model_data)
-        other = Repository.from_model_data(self.model_data)
+    async def test_changes_to_provider(self, repository_test):
+        current = Repository.from_model_data(repository_test.model_data)
+        other = Repository.from_model_data(repository_test.model_data)
 
         other.name = "other"
         other.has_wiki = False
@@ -124,10 +105,10 @@ async def test_changes_to_provider(self):
         assert provider_data["has_wiki"] is True
         assert query_json("security_and_analysis.secret_scanning.status", provider_data) or "" == "enabled"
 
-    def test_patch(self):
-        current = Repository.from_model_data(self.model_data)
+    def test_patch(self, repository_test):
+        current = Repository.from_model_data(repository_test.model_data)
 
-        default = Repository.from_model_data(self.model_data)
+        default = Repository.from_model_data(repository_test.model_data)
 
         default.name = None
         default.web_commit_signoff_required = True
@@ -138,9 +119,9 @@ def test_patch(self):
         assert patch["name"] == current.name
         assert patch["web_commit_signoff_required"] is current.web_commit_signoff_required
 
-    def test_difference(self):
-        current = Repository.from_model_data(self.model_data)
-        other = Repository.from_model_data(self.model_data)
+    def test_difference(self, repository_test):
+        current = Repository.from_model_data(repository_test.model_data)
+        other = Repository.from_model_data(repository_test.model_data)
 
         other.name = "other"
         other.has_wiki = False
@@ -150,3 +131,57 @@ def test_difference(self):
         assert len(diff) == 2
         assert diff["name"] == Change(other.name, current.name)
         assert diff["has_wiki"] == Change(other.has_wiki, current.has_wiki)
+
+    @pytest.mark.parametrize(
+        "gh_pages_source_path, gh_pages_source_branch, expected_changes",
+        [
+            (
+                "/docs",
+                None,
+                {
+                    "gh_pages_source_path": Change("/", "/docs"),
+                    "gh_pages_source_branch": Change("gh-pages", "gh-pages"),
+                },
+            ),
+            (
+                None,
+                "main",
+                {"gh_pages_source_branch": Change("gh-pages", "main"), "gh_pages_source_path": Change("/", "/")},
+            ),
+            (None, None, {}),
+            (
+                "/docs",
+                "main",
+                {"gh_pages_source_path": Change("/", "/docs"), "gh_pages_source_branch": Change("gh-pages", "main")},
+            ),
+        ],
+    )
+    def test__include_gh_pages_patch_required_properties(
+        self, repository_test, gh_pages_source_path, gh_pages_source_branch, expected_changes
+    ):
+        current = Repository.from_model_data(repository_test.model_data)
+        default = Repository.from_model_data(repository_test.model_data)
+
+        # build changes using parametrization if not none
+        changes = {}
+        if gh_pages_source_path is not None:
+            current.gh_pages_source_path = gh_pages_source_path
+            changes["gh_pages_source_path"] = Change(default.gh_pages_source_path, gh_pages_source_path)
+        if gh_pages_source_branch is not None:
+            current.gh_pages_source_branch = gh_pages_source_branch
+            changes["gh_pages_source_branch"] = Change(default.gh_pages_source_branch, gh_pages_source_branch)
+
+        test_livepatch = LivePatch(
+            patch_type=3,
+            expected_object=current,
+            current_object=default,
+            changes=changes,
+            parent_object=None,
+            forced_update=False,
+            fn=pretend.stub(),
+            changes_object_to_readonly=False,
+        )
+        # Call the function under test
+        current._include_gh_pages_patch_required_properties(test_livepatch)
+
+        assert test_livepatch.changes == expected_changes