Fix template sync (#222)

* Revert "fix typo (#165)" This reverts commit 5f091ed. * Cruft PR workflow (#168) Co-authored-by: Gregor Sturm <[email protected]> * Fix `git clone` in cruft PR workflow (#196) * depend on cruft in cruft PRs workflow (#197) * Fix cruft invocation typo (#198) * Use namespaced head in cruft PRs (#199) * Use user.login instead of user.name (#200) * Finish cruft PRs (#201) * Cruft PRs: wait for fork creation (#202) * Another backoff for cruft (#207) * Add retry with backoff also for clone * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Update template sync - better logging - check out release tag instead of main - put repo owner in PR branch name to deal with forks * fix tests * fix tests * Make name of forked repos include the original user * debug log * fix artifact upload * Fetch main branch from upstream * Fix get fork * fix clone url * fmt * comment on why pre-commit is a runtime dep * less positional * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Wrap entire command in try/catch * Skip PR if already exists for current version. This allows to re-run the entire action on failures without creating noise in the repos where the update was successful. * Fix log messages * Fix tests * Try fix test * Arming bot: Apply to all repos * Fix global exception handler * Fix global exception handler * Simplify code * Allow maintainer to modify the PR * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix logging for existing PRs * Set git default branch in tests --------- Co-authored-by: Philipp A <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
scverse · Nov 28, 2023 · ff4b39a · ff4b39a
1 parent 3b088cd
commit ff4b39a
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 23 deletions.
diff --git a/.github/workflows/cruft-prs.yml b/.github/workflows/cruft-prs.yml
@@ -30,3 +30,7 @@ jobs:
           GITHUB_TOKEN: ${{ secrets.BOT_GH_TOKEN }}
           FORCE_COLOR: "1"
           COLUMNS: "150"
+      - uses: actions/upload-artifact@v3
+        with:
+          name: cruft-logs
+          path: log/
diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
@@ -5,8 +5,6 @@ on:
     branches: [main]
   pull_request:
     branches: [main]
-  schedule:
-    - cron: "0 5 1,15 * *"
 
 defaults:
   run:
@@ -81,6 +79,8 @@ jobs:
           python-version: "3.11"
           cache: "pip"
           cache-dependency-path: "**/pyproject.toml"
+      - name: set python default branch
+        run: git config --global init.defaultBranch main
       - name: Install build dependencies
         run: python -m pip install --upgrade pip wheel
       - name: Install package

diff --git a/scripts/pyproject.toml b/scripts/pyproject.toml
@@ -23,6 +23,7 @@ dependencies = [
     "GitPython",
     "PyGitHub",
     "PyYAML",
+    "pre-commit", # is ran by cruft
 ]
 
 [project.optional-dependencies]

diff --git a/scripts/src/scverse_template_scripts/cruft_prs.py b/scripts/src/scverse_template_scripts/cruft_prs.py
@@ -86,6 +86,7 @@ def auth(self, url_str: str) -> str:
 class PR:
     con: GitHubConnection
     release: GHRelease
+    repo_id: str  # something like grst-infercnvpy
 
     title_prefix: ClassVar[LiteralString] = "Update template to "
     branch_prefix: ClassVar[LiteralString] = "template-update-"
@@ -96,7 +97,7 @@ def title(self) -> str:
 
     @property
     def branch(self) -> str:
-        return f"{self.branch_prefix}{self.release.tag_name}"
+        return f"{self.branch_prefix}{self.repo_id}-{self.release.tag_name}"
 
     @property
     def namespaced_head(self) -> str:
@@ -109,10 +110,15 @@ def body(self) -> str:
             template_usage="https://cookiecutter-scverse-instance.readthedocs.io/en/latest/template_usage.html",
         )
 
-    def matches(self, pr: PullRequest) -> bool:
+    def matches_prefix(self, pr: PullRequest) -> bool:
+        """Check if `pr` is either a current or previous template update PR by matching the branch name"""
         # Don’t compare title prefix, people might rename PRs
         return pr.head.ref.startswith(self.branch_prefix) and pr.user.id == self.con.user.id
 
+    def matches_current_version(self, pr: PullRequest) -> bool:
+        """Check if `pr` is a template update PR for the current version"""
+        return pr.head.ref == self.branch and pr.user.id == self.con.user.id
+
 
 class RepoInfo(TypedDict):
     url: str
@@ -138,9 +144,22 @@ def get_repo_urls(gh: Github) -> Generator[str]:
             yield repo["url"]
 
 
-def run_cruft(cwd: Path) -> CompletedProcess:
-    args = [sys.executable, "-m", "cruft", "update", "--checkout=main", "--skip-apply-ask", "--project-dir=."]
-    return run(args, check=True, cwd=cwd)
+def run_cruft(cwd: Path, *, tag_name: str, log_name: str) -> CompletedProcess:
+    args = [
+        sys.executable,
+        "-m",
+        "cruft",
+        "update",
+        f"--checkout={tag_name}",
+        "--skip-apply-ask",
+        "--project-dir=.",
+    ]
+
+    log_path = Path(f"./log/{log_name}.txt")
+    log_path.parent.mkdir(exist_ok=True)
+
+    with log_path.open("w") as log_file:
+        return run(args, check=True, cwd=cwd, stdout=log_file, stderr=log_file)
 
 
 # GitHub says that up to 5 minutes of wait are OK,
@@ -149,14 +168,26 @@ def run_cruft(cwd: Path) -> CompletedProcess:
 # Due to exponential backoff, we’ll maximally wait 2⁹ sec, or 8.5 min
 
 
-def cruft_update(con: GitHubConnection, repo: GHRepo, path: Path, pr: PR) -> bool:
+def cruft_update(  # noqa: PLR0913
+    con: GitHubConnection,
+    pr: PR,
+    *,
+    tag_name: str,
+    repo: GHRepo,
+    origin: GHRepo,
+    path: Path,
+) -> bool:
     clone = retry_with_backoff(
-        lambda: Repo.clone_from(con.auth(repo.clone_url), path), retries=n_retries, exc_cls=GitCommandError
+        lambda: Repo.clone_from(con.auth(repo.clone_url), path),
+        retries=n_retries,
+        exc_cls=GitCommandError,
     )
-    branch = clone.create_head(pr.branch, clone.active_branch)
+    upstream = clone.create_remote(name=pr.repo_id, url=origin.clone_url)
+    upstream.fetch()
+    branch = clone.create_head(pr.branch, f"{pr.repo_id}/{origin.default_branch}")
     branch.checkout()
 
-    run_cruft(path)
+    run_cruft(path, tag_name=tag_name, log_name=pr.branch)
 
     if not clone.is_dirty():
         return False
@@ -176,25 +207,45 @@ def cruft_update(con: GitHubConnection, repo: GHRepo, path: Path, pr: PR) -> boo
 
 
 def get_fork(con: GitHubConnection, repo: GHRepo) -> GHRepo:
-    if fork := next((f for f in repo.get_forks() if f.owner.id == con.user.id), None):
-        return fork
+    """Fork target repo into the scverse-bot namespace and wait until the fork has been created.
+    If the fork already exists it is reused.
+    """
     fork = repo.create_fork()
     return retry_with_backoff(lambda: con.gh.get_repo(fork.id), retries=n_retries, exc_cls=UnknownObjectException)
 
 
 def make_pr(con: GitHubConnection, release: GHRelease, repo_url: str) -> None:
-    pr = PR(con, release)
-    log.info(f"Sending PR to {repo_url}: {pr.title}")
+    repo_id = repo_url.replace("https://github.com/", "").replace("/", "-")
+    pr = PR(con, release, repo_id)
+    log.info(f"Sending PR to {repo_url} : {pr.title}")
 
     # create fork, populate branch, do PR from it
     origin = con.gh.get_repo(repo_url.removeprefix("https://github.com/"))
     repo = get_fork(con, origin)
+
+    if old_pr := next((p for p in origin.get_pulls("open") if pr.matches_current_version(p)), None):
+        log.info(
+            f"PR for current version already exists: #{old_pr.number} with branch name `{old_pr.head.ref}`. Skipping."
+        )
+        return
+
     with TemporaryDirectory() as td:
-        updated = cruft_update(con, repo, Path(td), pr)
+        updated = cruft_update(
+            con,
+            pr,
+            tag_name=release.tag_name,
+            repo=repo,
+            origin=origin,
+            path=Path(td),
+        )
     if updated:
-        if old_pr := next((p for p in origin.get_pulls("open") if pr.matches(p)), None):
+        if old_pr := next((p for p in origin.get_pulls("open") if pr.matches_prefix(p)), None):
+            log.info(f"Closing old PR #{old_pr.number} with branch name `{old_pr.head.ref}`.")
             old_pr.edit(state="closed")
-        origin.create_pull(pr.title, pr.body, origin.default_branch, pr.namespaced_head)
+        new_pr = origin.create_pull(
+            pr.title, pr.body, origin.default_branch, pr.namespaced_head, maintainer_can_modify=True
+        )
+        log.info(f"Created PR #{new_pr.number} with branch name `{new_pr.head.ref}`.")
 
 
 def setup() -> None:
@@ -210,8 +261,15 @@ def main(tag_name: str) -> None:
     con = GitHubConnection("scverse-bot", token)
     release = get_template_release(con.gh, tag_name)
     repo_urls = get_repo_urls(con.gh)
+    failed = 0
     for repo_url in repo_urls:
-        make_pr(con, release, repo_url)
+        try:
+            make_pr(con, release, repo_url)
+        except Exception as e:
+            failed += 1
+            log.exception(f"Error updating {repo_url}: %s", e)
+
+    sys.exit(failed > 0)
 
 
 def cli() -> None:

diff --git a/scripts/tests/test_cruft.py b/scripts/tests/test_cruft.py
@@ -1,5 +1,6 @@
 import json
 from dataclasses import dataclass
+from pathlib import Path
 from typing import cast
 from warnings import catch_warnings, filterwarnings
 
@@ -16,6 +17,7 @@
 class MockGHRepo:
     git_url: str  # git://github.com/foo/bar.git
     clone_url: str  # https://github.com/foo/bar.git
+    default_branch: str  # main
 
 
 @dataclass
@@ -43,18 +45,22 @@ def repo(git_repo: GitRepo) -> GHRepo:
     (git_repo.workspace / "b").write_text("b content")
     git_repo.api.index.add(["a", "b"])
     git_repo.api.index.commit("initial commit")
-    return cast(GHRepo, MockGHRepo(git_repo.uri, git_repo.uri))
+    return cast(GHRepo, MockGHRepo(git_repo.uri, git_repo.uri, "main"))
 
 
 @pytest.fixture
 def pr(con) -> PR:
-    return PR(con, cast(GHRelease, MockRelease()))
+    return PR(con, cast(GHRelease, MockRelease()), "scverse-test")
 
 
 def test_cruft_update(con, repo, tmp_path, pr, git_repo: GitRepo, monkeypatch: pytest.MonkeyPatch):
     old_active_branch_name = git_repo.api.active_branch.name
-    monkeypatch.setattr("scverse_template_scripts.cruft_prs.run_cruft", lambda p: (p / "b").write_text("b modified"))
-    changed = cruft_update(con, repo, tmp_path, pr)
+
+    def _mock_run_cruft(cwd: Path, *, tag_name, log_name):
+        return (cwd / "b").write_text("b modified")
+
+    monkeypatch.setattr("scverse_template_scripts.cruft_prs.run_cruft", _mock_run_cruft)
+    changed = cruft_update(con, pr, tag_name="main", repo=repo, origin=repo, path=tmp_path)
     assert changed  # TODO: add test for short circuit
     main_branch = git_repo.api.active_branch
     assert main_branch.name == old_active_branch_name, "Shouldn’t change active branch"

diff --git a/{{cookiecutter.project_name}}/.github/workflows/test.yaml b/{{cookiecutter.project_name}}/.github/workflows/test.yaml
@@ -5,6 +5,8 @@ on:
     branches: [main]
   pull_request:
     branches: [main]
+  schedule:
+    - cron: "0 5 1,15 * *"
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}

diff --git a/{{cookiecutter.project_name}}/.pre-commit-config.yaml b/{{cookiecutter.project_name}}/.pre-commit-config.yaml
@@ -39,6 +39,9 @@ repos:
         args: [--fix=lf]
       - id: trailing-whitespace
       - id: check-case-conflict
+      # Check that there are no merge conflicts (could be generated by template sync)
+      - id: check-merge-conflict
+        args: [--assume-in-merge]
   - repo: local
     hooks:
       - id: forbid-to-commit