diff --git a/docs/commands.md b/docs/commands.md index 0a6045313..6ce44ca3b 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -27,7 +27,7 @@ Some runtime commands support all benches when invoked with the CLI option for a ## App Commands - `pilot new-app APP`: scaffold a new Frappe app under `apps/` and install it. Prompts for title, description, publisher, email, license, GitHub workflow, and branch; pass any of `--title/--description/--publisher/--email/--license/--branch/--github-workflow` to skip prompts (branch defaults to `develop`). -- `pilot get-app REPO_OR_NAME`: clone and install an app into the bench. +- `pilot get-app REPO_OR_NAME [--branch BRANCH_OR_COMMIT]`: clone and install an app into the bench. Without `--branch`, a fresh clone uses the remote default branch and an existing install is reused at its current revision. An explicit branch or commit is a revision constraint, so a mismatch with an existing checkout fails instead of reusing it. - `pilot list-apps`: list apps present in the bench. - `pilot install-app APP --site SITE`: install apps on a site. An app the site only has disabled is enabled instead, bringing back anything it requires first. - `pilot uninstall-app APP --site SITE`: uninstall apps from a site, dropping their data. diff --git a/pilot/commands/apps/download.py b/pilot/commands/apps/download.py index f3dc357f2..6937b352d 100644 --- a/pilot/commands/apps/download.py +++ b/pilot/commands/apps/download.py @@ -21,7 +21,7 @@ class GetAppCommand(Command): def __post_init__(self) -> None: from pilot.core.app import App - self.app = App.from_repo(self.bench, self.repo, self.branch or "main") + self.app = App.from_repo(self.bench, self.repo, self.branch) self.installed_dependencies: list[App] = [] def run(self) -> None: diff --git a/pilot/core/app/__init__.py b/pilot/core/app/__init__.py index 973fdd7d0..66a4a0bd1 100644 --- a/pilot/core/app/__init__.py +++ b/pilot/core/app/__init__.py @@ -245,6 +245,40 @@ def build_assets(self) -> None: return run_command(["yarn", "--cwd", str(self.path), "build"]) + def _reject_revision_mismatch(self, commit: str = "") -> None: + from pilot.integrations.git.base import repo_host, same_repository + + requested = self.config.branch + installed = self.bench.app(self.module_name) + if ( + repo_host(self.config.repo) + and repo_host(installed.config.repo) + and not same_repository(self.config.repo, installed.config.repo) + ): + raise BenchError( + f"'{self.config.name}' is already installed from a different repository. " + "Remove the app and add it again to change its repository." + ) + requested_commit = commit or (requested if self.is_commit_hash(requested) else "") + if requested_commit: + if not installed.is_on_revision(RevisionPin(kind="commit", ref=requested_commit)): + raise BenchError( + f"'{self.config.name}' is already installed at a different commit, " + f"so this install cannot deliver '{requested_commit}'. Remove the app and " + f"add it again to change its revision." + ) + return + if not requested: + return + current = installed.current_branch + if requested != current: + source = f"branch '{current}'" if current else "a detached commit" + raise BenchError( + f"'{self.config.name}' is already installed from {source}, " + f"so this install cannot deliver '{requested}'. Remove the app and " + f"add it again to change its branch." + ) + def _skip_already_installed( self, on_progress: Callable[[str], None], install_dependencies: bool = False ) -> AppInstallResult: @@ -262,6 +296,7 @@ def install( ) -> AppInstallResult: """Pinned commit based Clone, validate, install, register, and build app assets.""" if self.bench.is_app_installed(self.config.name): + self._reject_revision_mismatch(commit) return self._skip_already_installed(on_progress, install_dependencies) existing_clone = self.existing_clone_path diff --git a/tests/e2e/flows/admin.py b/tests/e2e/flows/admin.py index 27cec807b..0f4da1563 100644 --- a/tests/e2e/flows/admin.py +++ b/tests/e2e/flows/admin.py @@ -7,8 +7,8 @@ def login(page: Page, base_url: str, password: str) -> None: - # The wizard's own sign-in carries over in this shared browser context, so without - # clearing it the login form would never render. Drop it to exercise real login. + # Let the setup page finish its restart redirect before starting login. + page.wait_for_url(f"{base_url}/sites", timeout=30_000) page.context.clear_cookies() page.goto(f"{base_url}/") page.get_by_placeholder("Password").fill(password) diff --git a/tests/pilot/commands/test_get_app.py b/tests/pilot/commands/test_get_app.py index e9de3ea0e..009f13117 100644 --- a/tests/pilot/commands/test_get_app.py +++ b/tests/pilot/commands/test_get_app.py @@ -2,15 +2,55 @@ from __future__ import annotations +import subprocess from pathlib import Path from unittest.mock import patch +import pytest + from pilot.commands.apps.download import GetAppCommand from pilot.core.app import App +from pilot.exceptions import BenchError from pilot.integrations.marketplace import Marketplace, Resolver from tests.pilot.commands.test_commands import make_bench +def register_app_on_branch(bench, name: str, branch: str) -> str: + app_dir = bench.apps_path / name + app_dir.mkdir(parents=True) + subprocess.run(["git", "init", "-b", branch, str(app_dir)], check=True, capture_output=True) + subprocess.run( + ["git", "-C", str(app_dir), "remote", "add", "origin", f"https://github.com/frappe/{name}"], + check=True, + capture_output=True, + ) + (app_dir / "hooks.py").write_text("") + subprocess.run(["git", "-C", str(app_dir), "add", "hooks.py"], check=True, capture_output=True) + subprocess.run( + [ + "git", + "-C", + str(app_dir), + "-c", + "user.name=Pilot Tests", + "-c", + "user.email=pilot@example.com", + "commit", + "-m", + "Initial commit", + ], + check=True, + capture_output=True, + ) + (bench.sites_path / "apps.txt").write_text(f"frappe\n{name}\n") + return subprocess.run( + ["git", "-C", str(app_dir), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def make_resolver(name: str, deps: dict[str, str] | None = None) -> Resolver: return Resolver( app=name, @@ -71,6 +111,103 @@ def test_run_short_circuits_when_app_already_registered(tmp_path: Path) -> None: mock_build.assert_not_called() +def test_reinstall_with_a_different_branch_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16-hotfix") + + with pytest.raises(BenchError, match="already installed from branch 'version-16'"): + cmd.run() + + +def test_reinstall_from_a_different_repository_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/acme/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed from a different repository"): + cmd.run() + + +def test_reinstall_with_the_same_branch_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16") + + with patch.object(App, "clone") as mock_clone: + cmd.run() + + mock_clone.assert_not_called() + + +def test_reinstall_with_a_branch_rejects_a_detached_checkout(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + subprocess.run( + ["git", "-C", str(bench.apps_path / "myapp"), "checkout", "--detach", current], + check=True, + capture_output=True, + ) + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed from a detached commit"): + cmd.run() + + +def test_reinstall_with_a_different_commit_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + + requested = "0" * 40 + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch=requested) + + with pytest.raises(BenchError, match="already installed at a different commit"): + cmd.run() + + +def test_reinstall_with_the_same_commit_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + + cmd = GetAppCommand(bench, repo="https://github.com/frappe/myapp", branch=current[:8]) + + with patch.object(App, "clone") as mock_clone: + cmd.run() + + mock_clone.assert_not_called() + + +def test_reinstall_with_a_different_marketplace_commit_fails_loudly(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + register_app_on_branch(bench, "myapp", "version-16") + app = App.from_repo(bench, "https://github.com/frappe/myapp", branch="version-16") + + with pytest.raises(BenchError, match="already installed at a different commit"): + app.install(commit="0" * 40) + + +def test_reinstall_with_the_same_marketplace_commit_still_short_circuits(tmp_path: Path) -> None: + bench = make_bench(tmp_path) + bench.create_directories() + current = register_app_on_branch(bench, "myapp", "version-16") + app = App.from_repo(bench, "https://github.com/frappe/myapp", branch="version-16") + + result = app.install(commit=current[:8]) + + assert result.already_installed is True + + def test_short_circuit_adopts_real_on_disk_app_path(tmp_path: Path) -> None: """Regression: short-circuit uses the normalized on-disk app path.""" bench = make_bench(tmp_path)