shahar1 commented on code in PR #74823: URL: https://github.com/apache/airflow/pull/74823#discussion_r4367561032
########## dev/breeze/src/airflow_breeze/utils/ci_audit.py: ########## @@ -0,0 +1,382 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Checks behind `breeze ci audit`: is this checkout and fork set up for fork CI (AIP-120)? + +Every check is read-only. A failing check offers the commands that fix it; `--fix` runs them after +confirmation. See "breeze ci audit" in dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. +""" + +from __future__ import annotations + +import json +import re +import subprocess +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from enum import Enum +from typing import TYPE_CHECKING + +from packaging.version import Version + +from airflow_breeze.global_constants import APACHE_AIRFLOW_GITHUB_REPOSITORY, MIN_GH_VERSION +from airflow_breeze.utils.git_remotes import find_remote, find_remote_for_repo, parse_remotes + +if TYPE_CHECKING: + from airflow_breeze.utils.fork_ci_gate import GateConfig + +UPSTREAM_URL = f"https://github.com/{APACHE_AIRFLOW_GITHUB_REPOSITORY}.git"; +CI_WORKFLOW_FILE = "ci-amd.yml" +# Commits a fork's main may lag behind upstream main before a sync is suggested. +MAX_FORK_MAIN_BEHIND = 200 + +Runner = Callable[[Sequence[str]], subprocess.CompletedProcess] + + +class CheckStatus(str, Enum): + OK = "ok" + FAIL = "fail" + INFO = "info" + + +class Role(str, Enum): + COMMITTER = "committer" + COLLABORATOR = "collaborator" + EXEMPTED = "exempted organisation member" + PILOT = "fork CI pilot participant" + EXTERNAL = "external contributor" + + +@dataclass(frozen=True) +class CheckResult: + number: int + name: str + status: CheckStatus + finding: str + fix: tuple[str, ...] = () + + +def run_tool(command: Sequence[str]) -> subprocess.CompletedProcess: + """Run a command for the audit; a missing executable is a failed command, not a crash.""" + try: + return subprocess.run(list(command), capture_output=True, text=True, check=False) + except OSError as e: + return subprocess.CompletedProcess(list(command), 127, stdout="", stderr=str(e)) + + +def _ok(result: subprocess.CompletedProcess) -> bool: + return result.returncode == 0 + + +def _out(result: subprocess.CompletedProcess) -> str: + return (result.stdout or "").strip() + + +def redact_url(url: str) -> str: + """Drop the credentials an http(s) remote URL may carry, so a finding never prints a token.""" + return re.sub(r"^(https?://)[^@/]+@", r"\1", url) + + +def _is_not_found(result: subprocess.CompletedProcess) -> bool: + return "404" in (result.stderr or "") or "Not Found" in (result.stderr or "") + + +def is_public_apache_member(*, login: str, run: Runner) -> bool: + return _ok(run(["gh", "api", f"orgs/apache/public_members/{login}"])) Review Comment: **minor, non-blocking** (Description doesn't match code: "PR description describes something different from what the code actually does.") The doc added in this layer says "A failed API call is reported as \"could not check\" rather than as a finding", but this call does not follow that rule: any non-zero exit (rate limit, 5xx, network) is read as "not a public member". A committer who hits a transient failure is then classified as an external contributor. Checks 2, 3 and 5 turn into FAIL rows, and `--fix` offers `gh workflow enable` on their fork, which is the double-run this audit is meant to prevent. The endpoint returns 404 for a non-member, so `_is_not_found` can tell the two cases apart. Suggested fix: Treat only a 404 as "not a public member". On any other failure, report the role as could-not-check, and keep checks 2, 3 and 5 at INFO. ########## dev/breeze/src/airflow_breeze/utils/ci_audit.py: ########## @@ -0,0 +1,382 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Checks behind `breeze ci audit`: is this checkout and fork set up for fork CI (AIP-120)? + +Every check is read-only. A failing check offers the commands that fix it; `--fix` runs them after +confirmation. See "breeze ci audit" in dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. +""" + +from __future__ import annotations + +import json +import re +import subprocess +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from enum import Enum +from typing import TYPE_CHECKING + +from packaging.version import Version + +from airflow_breeze.global_constants import APACHE_AIRFLOW_GITHUB_REPOSITORY, MIN_GH_VERSION +from airflow_breeze.utils.git_remotes import find_remote, find_remote_for_repo, parse_remotes + +if TYPE_CHECKING: + from airflow_breeze.utils.fork_ci_gate import GateConfig + +UPSTREAM_URL = f"https://github.com/{APACHE_AIRFLOW_GITHUB_REPOSITORY}.git"; +CI_WORKFLOW_FILE = "ci-amd.yml" +# Commits a fork's main may lag behind upstream main before a sync is suggested. +MAX_FORK_MAIN_BEHIND = 200 + +Runner = Callable[[Sequence[str]], subprocess.CompletedProcess] + + +class CheckStatus(str, Enum): + OK = "ok" + FAIL = "fail" + INFO = "info" + + +class Role(str, Enum): + COMMITTER = "committer" + COLLABORATOR = "collaborator" + EXEMPTED = "exempted organisation member" + PILOT = "fork CI pilot participant" + EXTERNAL = "external contributor" + + +@dataclass(frozen=True) +class CheckResult: + number: int + name: str + status: CheckStatus + finding: str + fix: tuple[str, ...] = () + + +def run_tool(command: Sequence[str]) -> subprocess.CompletedProcess: + """Run a command for the audit; a missing executable is a failed command, not a crash.""" + try: + return subprocess.run(list(command), capture_output=True, text=True, check=False) + except OSError as e: + return subprocess.CompletedProcess(list(command), 127, stdout="", stderr=str(e)) + + +def _ok(result: subprocess.CompletedProcess) -> bool: + return result.returncode == 0 + + +def _out(result: subprocess.CompletedProcess) -> str: + return (result.stdout or "").strip() + + +def redact_url(url: str) -> str: + """Drop the credentials an http(s) remote URL may carry, so a finding never prints a token.""" + return re.sub(r"^(https?://)[^@/]+@", r"\1", url) + + +def _is_not_found(result: subprocess.CompletedProcess) -> bool: + return "404" in (result.stderr or "") or "Not Found" in (result.stderr or "") + + +def is_public_apache_member(*, login: str, run: Runner) -> bool: + return _ok(run(["gh", "api", f"orgs/apache/public_members/{login}"])) + + +def is_private_apache_member(*, run: Runner) -> bool: + membership = run(["gh", "api", "user/memberships/orgs/apache", "--jq", ".state"]) + return _ok(membership) and _out(membership) == "active" + + +def get_role(*, login: str, run: Runner, config: GateConfig, collaborators: frozenset[str]) -> Role: + # In the gate's order: a pilot entry gates a pull request whatever its author's role. + if config.pilot_joined_at(login) is not None: + return Role.PILOT + # The gate only sees public membership: GitHub reports a private member's pull requests as + # `CONTRIBUTOR`, so they are gated like an external contributor's. + if is_public_apache_member(login=login, run=run): + return Role.COMMITTER + if login.casefold() in {c.casefold() for c in collaborators}: + return Role.COLLABORATOR + if config.is_allowlisted(login): + return Role.EXEMPTED + return Role.EXTERNAL + + +def fork_ci_required(role: Role, config: GateConfig) -> bool | None: + """True when fork CI must be on, False when it must be off, None when either is fine.""" + if role == Role.PILOT: + return True + if role == Role.EXTERNAL: + return True if config.enabled else None + return False + + +def run_audit(*, run: Runner, config: GateConfig, collaborators: frozenset[str]) -> list[CheckResult]: + results: list[CheckResult] = [] + + gh_version = run(["gh", "--version"]) + if not _ok(gh_version): + results.append( + CheckResult( + 1, "gh", CheckStatus.FAIL, "`gh` is not installed", ("echo See https://cli.github.com";,) + ) + ) + return results + version_match = re.search(r"gh version (\d+\.\d+\.\d+)", _out(gh_version)) + if version_match is None or Version(version_match.group(1)) < Version(MIN_GH_VERSION): + results.append( + CheckResult(1, "gh", CheckStatus.FAIL, f"`gh` is older than {MIN_GH_VERSION}; upgrade it") + ) + return results + user = run(["gh", "api", "user", "--jq", ".login"]) + if not _ok(user): + results.append( + CheckResult(1, "gh", CheckStatus.FAIL, "`gh` is not authenticated", ("gh auth login",)) + ) + return results + login = _out(user) + results.append(CheckResult(1, "gh", CheckStatus.OK, f"authenticated as {login}")) + + # The role decides whether the fork setup matters at all: a committer working from an + # apache/airflow clone without a fork is fine while their pull requests run project CI. + role = get_role(login=login, run=run, config=config, collaborators=collaborators) + required = fork_ci_required(role, config) + setup_status = CheckStatus.INFO if required is False else CheckStatus.FAIL + setup_note = "; only needed for fork CI, which your pull requests do not use" if required is False else "" + + remotes = parse_remotes(_out(run(["git", "remote", "-v"]))) + upstream = find_remote(remotes, "upstream") + apache = find_remote_for_repo(remotes, APACHE_AIRFLOW_GITHUB_REPOSITORY) + origin = find_remote(remotes, "origin") + # Whether the remotes fix already adds `origin`, so the fork fix must not add it again. + adds_origin = False + if ( + upstream is not None + and upstream.repo + and upstream.repo.casefold() == APACHE_AIRFLOW_GITHUB_REPOSITORY + ): + if origin is not None and origin.repo and origin.repo.casefold() == APACHE_AIRFLOW_GITHUB_REPOSITORY: + results.append( + CheckResult( + 2, + "remotes", + setup_status, + f"`origin` points at apache/airflow; it should be your fork{setup_note}", + (f"git remote set-url origin https://github.com/{login}/airflow.git";,), + ) + ) + else: + results.append(CheckResult(2, "remotes", CheckStatus.OK, "`upstream` is apache/airflow")) + elif apache is not None and upstream is None: + fix = [f"git remote rename {apache.name} upstream"] + if apache.name == "origin": + fix.append(f"git remote add origin https://github.com/{login}/airflow.git";) + adds_origin = True + results.append( + CheckResult( + 2, + "remotes", + setup_status, + f"apache/airflow is the remote `{apache.name}`; Airflow's convention is `upstream`{setup_note}", + tuple(fix), + ) + ) + elif upstream is not None: + results.append( + CheckResult( + 2, + "remotes", + CheckStatus.FAIL, + f"`upstream` points at {redact_url(upstream.url)}, not apache/airflow", + (f"git remote set-url upstream {UPSTREAM_URL}",), + ) + ) + else: + results.append( + CheckResult( + 2, + "remotes", + CheckStatus.FAIL, + "no `upstream` remote pointing at apache/airflow", + (f"git remote add upstream {UPSTREAM_URL}",), + ) + ) + + fork = origin.repo if origin is not None and origin.repo else None + fork_ok = False + if fork is None or fork.casefold() == APACHE_AIRFLOW_GITHUB_REPOSITORY: + results.append( + CheckResult( + 3, + "fork", + setup_status, + f"no `origin` remote pointing at your fork of apache/airflow{setup_note}", + ( + f"gh repo fork {APACHE_AIRFLOW_GITHUB_REPOSITORY} --remote=false", + *( + () + if adds_origin + else (f"git remote add origin https://github.com/{login}/airflow.git";,) Review Comment: **minor, non-blocking** (Description doesn't match code: "PR description describes something different from what the code actually does.") Doc row 3 says check 3 adds `origin` "unless check 2's fix already adds it". The `origin-also-upstream` case (both remotes on apache/airflow) is not covered. Check 2 offers `git remote set-url origin https://github.com/{login}/airflow.git` but leaves `adds_origin` False, so check 3 still offers `git remote add origin ...`. When both fixes run under `--fix`, that second command fails with "remote origin already exists". The re-run audit still ends clean because `set-url` already did the job, so the only harm is a failing command in the output. `test_remote_fixes[origin-also-upstream]` asserts only check 2's fix. Suggested fix: Set `adds_origin = True` in the `origin points at apache/airflow` branch, and assert check 3's fix for that case too. ########## dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md: ########## @@ -763,19 +763,36 @@ prints one row: status, finding, and the command that fixes it. |---|---|---|---| | 1 | `gh` present and authenticated | `gh auth status` succeeds, version at least `MIN_GH_VERSION` | install / `gh auth login` | Review Comment: **nit, non-blocking** (Description doesn't match code: "PR description describes something different from what the code actually does.") Two rows of the check table no longer match the code. Row 1 says `gh auth status`, but the code runs `gh api user --jq .login`. Row 6 says "the configured threshold", but the threshold is the constant `MAX_FORK_MAIN_BEHIND = 200`. The paragraph below the table also still says "currently inside `breeze ci upgrade`" about code this layer has already moved. Suggested fix: Change the two rows to `gh api user` and "more than 200 commits (`MAX_FORK_MAIN_BEHIND`)", and drop "currently". ########## dev/breeze/src/airflow_breeze/utils/git_remotes.py: ########## @@ -0,0 +1,78 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Git remotes of the local checkout, as `breeze ci upgrade` and `breeze ci audit` understand them.""" + +from __future__ import annotations + +import re +from collections.abc import Sequence +from dataclasses import dataclass + +from airflow_breeze.utils.run_utils import run_command + +# Every form git and GitHub accept for a GitHub repository: http or https with optional +# credentials, `www.` or a port, scp-like ssh and ssh URLs with an optional port. The host is +# case-insensitive. An ssh host may also be an alias from ~/.ssh/config, such as +# `git@github-personal:apache/airflow.git`: a host without a dot that names `github`. A dotted +# host other than github.com, such as a GitHub Enterprise server, is a different GitHub. The +# repository must be the whole path, so `apache/airflow-site` never matches `apache/airflow`. +_SSH_GITHUB_HOST = r"(?:(?:www\.)?github\.com|[^@\s/:.]*github[^@\s/:.]*)" Review Comment: **minor, non-blocking** (Description doesn't match code: "PR description describes something different from what the code actually does.") The second commit says it recognises "ssh host aliases from ~/.ssh/config as GitHub remotes again, as breeze ci upgrade did before", but the alias branch excludes any dot, so the common `Host github.com-work` convention is still dropped. Checked against this regex: `[email protected]:apache/airflow.git` and `ssh://[email protected]/apache/airflow.git` both give `None`, while the old `"apache/airflow" in remote_url` in `ci upgrade` found them. Effects at this head: `breeze ci upgrade` warns that no apache/airflow remote exists and, if the user then picks the switch, reaches the existing `run_command(["git", "fetch", apache_remote_name])` with `None`; `breeze ci audit` reports "`upstream` points at [email protected]:apache/airflow.git, not apache/airflow" and `--fix` offers to rewrite that working ssh remote to https. A dotted host that starts with `github.com` followed by `-` is an alias, not a GitHub Enterprise server. Suggested fix: Also accept a host of the form `github.com-<suffix>` in `_SSH_GITHUB_HOST` (for example `github\.com-[^@\s/:]+`), and add `[email protected]:apache/airflow.git` to `test_get_github_repo`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
