diff --git a/diff_poetry_lock/run_poetry.py b/diff_poetry_lock/run_poetry.py index 0384345..b44ad56 100644 --- a/diff_poetry_lock/run_poetry.py +++ b/diff_poetry_lock/run_poetry.py @@ -83,7 +83,7 @@ def post_comment(api: GithubApi, comment: str | None) -> None: def format_comment( packages: list[PackageSummary], base_commit_hash: str | None = None, - target_commit_hash: str | None = None, + head_commit_hash: str | None = None, ) -> str | None: added = sorted([p for p in packages if p.added()], key=attrgetter("name")) removed = sorted([p for p in packages if p.removed()], key=attrgetter("name")) @@ -95,8 +95,8 @@ def format_comment( change_count = len(added + removed + updated) comment = f"### Detected {change_count} changes to dependencies in Poetry lockfile\n\n" - if base_commit_hash and target_commit_hash: - comment += f"From base {base_commit_hash} to target {target_commit_hash}:\n\n" + if base_commit_hash and head_commit_hash: + comment += f"From base {base_commit_hash} to head {head_commit_hash}:\n\n" summary_lines = [p.summary_line() for p in added + removed + updated] comment += "\n".join(summary_lines) comment += ( @@ -143,11 +143,11 @@ def do_diff(settings: Settings) -> None: if not any(package.changed() for package in packages): summary = None else: - target_commit_hash, base_commit_hash = api.resolve_commit_hashes(settings.ref, settings.base_ref) + head_commit_hash, base_commit_hash = api.resolve_commit_hashes(settings.ref, settings.base_ref) summary = format_comment( packages, base_commit_hash=base_commit_hash, - target_commit_hash=target_commit_hash, + head_commit_hash=head_commit_hash, ) if summary: diff --git a/diff_poetry_lock/settings.py b/diff_poetry_lock/settings.py index 41c833f..310a4ea 100644 --- a/diff_poetry_lock/settings.py +++ b/diff_poetry_lock/settings.py @@ -22,7 +22,6 @@ class Settings(ABC): ref: str repository: str base_ref: str - pr_num: str | None # from step config including secrets token: str @@ -32,6 +31,31 @@ class Settings(ABC): sigil_envvar: ClassVar[str] """The envvar in this will always be present when this settings is valid.""" + _pr_lookup_service: PrLookupService | None = None + + def set_pr_lookup_service(self, service: PrLookupService) -> None: + self._pr_lookup_service = service + + _pr_num_cached: str = "" + + @property + def pr_num(self) -> str | None: + if self._pr_num_cached: + return self._pr_num_cached + + if self._pr_lookup_service is None: + logger.warning("PR lookup requested before service configured; returning None") + return None + + logger.debug("Settings.pr_num looking up PR for branch {}", self.ref) + pr_num = self._pr_lookup_service.find_pr_for_branch(self.ref) + object.__setattr__(self, "_pr_num_cached", pr_num) + if pr_num: + logger.debug("Settings.pr_num found PR #{}", pr_num) + else: + logger.warning("Settings.pr_num found no open PR") + return pr_num + @classmethod def matches_env(cls, env: dict[str, str]) -> bool: """Check whether this CI's identifying env var is present.""" @@ -65,34 +89,15 @@ def __init__(self, **values: Any) -> None: # noqa: ANN401 logger.debug("VelaSettings ref={}", self.ref) logger.debug("VelaSettings event_name={}", self.event_name) - def set_pr_lookup_service(self, service: PrLookupService) -> None: - self._pr_lookup_service = service - - @property - def pr_num(self) -> str | None: # type: ignore[override] - if self._pr_num_cached: - return self._pr_num_cached - - if self._pr_lookup_service is None: - logger.warning("PR lookup requested before service configured; returning None") - return None - - logger.debug("VelaSettings.pr_num looking up PR for branch {}", self.ref) - pr_num = self._pr_lookup_service.find_pr_for_branch(self.ref) - self._pr_num_cached = pr_num - if pr_num: - logger.debug("VelaSettings.pr_num found PR #{}", pr_num) - else: - logger.warning("VelaSettings.pr_num found no open PR") - return pr_num - class GitHubActionsSettings(BaseSettings, Settings): sigil_envvar: ClassVar[str] = "github_repository" + _pr_lookup_service: PrLookupService | None = PrivateAttr(default=None) + _pr_num_cached: str = PrivateAttr(default="") # from CI event_name: str = Field(env="github_event_name") # must be 'pull_request' - ref: str = Field(env="github_ref") + ref: str = Field(env="github_head_ref") repository: str = Field(env="github_repository") base_ref: str = Field(env="github_base_ref") @@ -120,12 +125,6 @@ def event_must_be_pull_request(cls, v: str) -> str: raise ValueError(msg) return v - @property - # todo: Avoid this MyPy error by having Pydantic compute the field - def pr_num(self) -> str | None: # type: ignore[override] - # TODO: Validate early - return self.ref.split("/")[2] - _CI_SETTINGS_CANDIDATES: list[type[Settings]] = [GitHubActionsSettings, VelaSettings] diff --git a/diff_poetry_lock/test/test_poetry_diff.py b/diff_poetry_lock/test/test_poetry_diff.py index df4799b..0e9b168 100644 --- a/diff_poetry_lock/test/test_poetry_diff.py +++ b/diff_poetry_lock/test/test_poetry_diff.py @@ -12,7 +12,13 @@ from diff_poetry_lock import __version__ from diff_poetry_lock.github import MAGIC_COMMENT_IDENTIFIER, GithubApi from diff_poetry_lock.run_poetry import PackageSummary, diff, do_diff, format_comment, load_packages, main -from diff_poetry_lock.settings import GitHubActionsSettings, Settings +from diff_poetry_lock.settings import ( + GitHubActionsSettings, + PrLookupConfigurable, + PrLookupService, + Settings, + VelaSettings, +) TESTFILE_1 = Path("diff_poetry_lock/test/res/poetry1.lock") TESTFILE_2 = Path("diff_poetry_lock/test/res/poetry2.lock") @@ -33,21 +39,71 @@ def data2() -> bytes: return load_file(TESTFILE_2) +@pytest.fixture(autouse=True) +def patch_pr_lookup_setter(monkeypatch: MonkeyPatch) -> None: + def _safe_setter(self: Settings, service: PrLookupService) -> None: + object.__setattr__(self, "_pr_lookup_service", service) + + monkeypatch.setattr(Settings, "set_pr_lookup_service", _safe_setter) + + +class _LookupService: + def __init__(self, settings: Settings) -> None: + self.s = settings + if isinstance(self.s, PrLookupConfigurable): + self.s.set_pr_lookup_service(self) + + def find_pr_for_branch(self, branch_ref: str) -> str: + if branch_ref == "refs/heads/github-actions": + return "42" + if branch_ref == "refs/heads/vela": + return "1" + return "" + + def test_settings(monkeypatch: MonkeyPatch) -> None: monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") - monkeypatch.setenv("GITHUB_REF", "refs/pull/1/merge") + monkeypatch.setenv("GITHUB_HEAD_REF", "refs/heads/github-actions") monkeypatch.setenv("GITHUB_REPOSITORY", "account/repo") monkeypatch.setenv("INPUT_GITHUB_TOKEN", "foobar") monkeypatch.setenv("GITHUB_BASE_REF", "main") s = GitHubActionsSettings() + assert s.event_name == "pull_request" + assert s.ref == "refs/heads/github-actions" + assert s.repository == "account/repo" + assert s.base_ref == "main" + assert s.pr_num is None + + _LookupService(s) + + assert s.pr_num == "42" + + +def test_vela_settings(monkeypatch: MonkeyPatch) -> None: + monkeypatch.setenv("VELA_BUILD_EVENT", "push") + monkeypatch.setenv("VELA_BUILD_REF", "refs/heads/vela") + monkeypatch.setenv("VELA_REPO_FULL_NAME", "account/repo") + monkeypatch.setenv("VELA_REPO_BRANCH", "main") + monkeypatch.setenv("PARAMETER_GITHUB_TOKEN", "vela-token") + monkeypatch.setenv("PARAMETER_LOCKFILE_PATH", "poetry.lock") + monkeypatch.setenv("PARAMETER_GITHUB_API_URL", "https://api.github.com") + + s = VelaSettings() + assert s.event_name == "push" + assert s.ref == "refs/heads/vela" + assert s.repository == "account/repo" + assert s.base_ref == "refs/heads/main" + assert s.pr_num is None + + _LookupService(s) assert s.pr_num == "1" def test_settings_not_pr(monkeypatch: MonkeyPatch) -> None: monkeypatch.setenv("GITHUB_EVENT_NAME", "push") - monkeypatch.setenv("GITHUB_REF", "refs/pull/1/merge") + monkeypatch.setenv("GITHUB_HEAD_REF", "deps-update") monkeypatch.setenv("GITHUB_REPOSITORY", "account/repo") monkeypatch.setenv("INPUT_GITHUB_TOKEN", "foobar") monkeypatch.setenv("GITHUB_BASE_REF", "main") @@ -79,7 +135,7 @@ def test_diff() -> None: expected_comment = f"""\ ### Detected 3 changes to dependencies in Poetry lockfile - From base new-sha to target old-sha: + From base new-sha to head old-sha: Removed **pydantic** (1.10.6) Removed **typing-extensions** (4.5.0) @@ -93,7 +149,7 @@ def test_diff() -> None: format_comment( summary, base_commit_hash="new-sha", - target_commit_hash="old-sha", + head_commit_hash="old-sha", ) or "" ).strip() == dedent(expected_comment).strip() @@ -145,7 +201,7 @@ def test_diff_2() -> None: expected_comment = f"""\ ### Detected 3 changes to dependencies in Poetry lockfile - From base new-sha to target old-sha: + From base new-sha to head old-sha: Added **pydantic** (1.10.6) Added **typing-extensions** (4.5.0) @@ -159,7 +215,7 @@ def test_diff_2() -> None: format_comment( summary, base_commit_hash="new-sha", - target_commit_hash="old-sha", + head_commit_hash="old-sha", ) or "" ).strip() == dedent(expected_comment).strip() @@ -238,16 +294,17 @@ def test_e2e_diff_inexisting_comment(cfg: Settings, data1: bytes, data2: bytes) summary = format_comment( diff(load_packages(TESTFILE_2), load_packages(TESTFILE_1)), base_commit_hash="new-sha", - target_commit_hash="old-sha", + head_commit_hash="old-sha", ) with requests_mock.Mocker() as m: mock_get_file(m, cfg, data1, cfg.base_ref, resolved_hash="old-sha") mock_get_file(m, cfg, data2, cfg.ref, resolved_hash="new-sha") mock_resolve_commit_hashes(m, cfg, head_hash="new-sha", base_hash="old-sha") - mock_list_comments(m, cfg, []) + mock_find_pr_for_branch(m, cfg, pr_num="1") + mock_list_comments(m, cfg, [], pr_num="1") m.post( - f"{cfg.api_url}/repos/{cfg.repository}/issues/{cfg.pr_num}/comments", + f"{cfg.api_url}/repos/{cfg.repository}/issues/1/comments", headers=GithubApi.Headers.JSON.headers(cfg.token), json={"body": f"{MAGIC_COMMENT_IDENTIFIER}{summary}"}, ) @@ -259,19 +316,20 @@ def test_e2e_diff_existing_comment_same_data(cfg: Settings, data1: bytes, data2: summary = format_comment( diff(load_packages(TESTFILE_1), load_packages(TESTFILE_2)), base_commit_hash="old-sha", - target_commit_hash="new-sha", + head_commit_hash="new-sha", ) with requests_mock.Mocker() as m: mock_get_file(m, cfg, data1, cfg.base_ref, resolved_hash="old-sha") mock_get_file(m, cfg, data2, cfg.ref, resolved_hash="new-sha") mock_resolve_commit_hashes(m, cfg, head_hash="new-sha", base_hash="old-sha") + mock_find_pr_for_branch(m, cfg, pr_num="1") comments = [ {"body": "foobar", "id": 1334, "user": {"id": 123}}, {"body": "foobar", "id": 1335, "user": {"id": 41898282}}, {"body": f"{MAGIC_COMMENT_IDENTIFIER}{summary}", "id": 1337, "user": {"id": 41898282}}, ] - mock_list_comments(m, cfg, comments) + mock_list_comments(m, cfg, comments, pr_num="1") do_diff(cfg) @@ -280,19 +338,20 @@ def test_e2e_diff_existing_comment_different_data(cfg: Settings, data1: bytes, d summary = format_comment( diff(load_packages(TESTFILE_1), []), base_commit_hash="new-sha", - target_commit_hash="old-sha", + head_commit_hash="old-sha", ) with requests_mock.Mocker() as m: mock_get_file(m, cfg, data1, cfg.base_ref, resolved_hash="old-sha") mock_get_file(m, cfg, data2, cfg.ref, resolved_hash="new-sha") mock_resolve_commit_hashes(m, cfg, head_hash="new-sha", base_hash="old-sha") + mock_find_pr_for_branch(m, cfg, pr_num="1") comments = [ {"body": "foobar", "id": 1334, "user": {"id": 123}}, {"body": "foobar", "id": 1335, "user": {"id": 41898282}}, {"body": f"{MAGIC_COMMENT_IDENTIFIER}{summary}", "id": 1337, "user": {"id": 41898282}}, ] - mock_list_comments(m, cfg, comments) + mock_list_comments(m, cfg, comments, pr_num="1") m.patch( f"{cfg.api_url}/repos/{cfg.repository}/issues/comments/1337", headers=GithubApi.Headers.JSON.headers(cfg.token), @@ -306,20 +365,21 @@ def test_e2e_diff_commit_lookup_http_failure_falls_back_to_ref(cfg: Settings, da summary = format_comment( diff(load_packages(TESTFILE_2), load_packages(TESTFILE_1)), base_commit_hash=cfg.ref, - target_commit_hash=cfg.base_ref, + head_commit_hash=cfg.base_ref, ) with requests_mock.Mocker() as m: mock_get_file(m, cfg, data1, cfg.base_ref) mock_get_file(m, cfg, data2, cfg.ref) + mock_find_pr_for_branch(m, cfg, pr_num="1") m.post( f"{cfg.api_url}/graphql", request_headers=GithubApi.Headers.JSON.headers(cfg.token), status_code=500, ) - mock_list_comments(m, cfg, []) + mock_list_comments(m, cfg, [], pr_num="1") m.post( - f"{cfg.api_url}/repos/{cfg.repository}/issues/{cfg.pr_num}/comments", + f"{cfg.api_url}/repos/{cfg.repository}/issues/1/comments", headers=GithubApi.Headers.JSON.headers(cfg.token), json={"body": f"{MAGIC_COMMENT_IDENTIFIER}{summary}"}, ) @@ -332,9 +392,18 @@ def load_file(filename: Path) -> bytes: return f.read() -def mock_list_comments(m: Mocker, s: Settings, response_json: list[dict[Any, Any]]) -> None: +def mock_find_pr_for_branch(m: Mocker, s: Settings, pr_num: str = "1") -> None: + head = f"{s.repository.split('/')[0]}:{s.ref}" + m.get( + f"{s.api_url}/repos/{s.repository}/pulls?head={head}&state=open", + request_headers=GithubApi.Headers.JSON.headers(s.token), + json=[{"number": int(pr_num)}], + ) + + +def mock_list_comments(m: Mocker, s: Settings, response_json: list[dict[Any, Any]], pr_num: str = "1") -> None: m.get( - f"{s.api_url}/repos/{s.repository}/issues/{s.pr_num}/comments?per_page=100&page=1", + f"{s.api_url}/repos/{s.repository}/issues/{pr_num}/comments?per_page=100&page=1", request_headers=GithubApi.Headers.JSON.headers(s.token), json=response_json, ) @@ -377,7 +446,7 @@ def create_settings( ) -> Settings: return GitHubActionsSettings( event_name="pull_request", - ref="refs/pull/1/merge", + ref="deps-update", repository=repository, token=token, base_ref="main",