nisse-review: match repository patterns against Gitea's search #154
No files matched your search
@@ -882,9 +882,9 @@ else) towards better changes.
|
|||||||
a draft, and not marked work in progress. Any author's: the operator's,
|
a draft, and not marked work in progress. Any author's: the operator's,
|
||||||
`nisse-dev`'s, or another tool's. Its repositories are the ones its Gitea
|
`nisse-dev`'s, or another tool's. Its repositories are the ones its Gitea
|
||||||
account was given, or, when configured, those matching patterns (`dev/*`,
|
account was given, or, when configured, those matching patterns (`dev/*`,
|
||||||
`*/*`) among all it can read, public ones included: Gitea leaves a public
|
`*/*`) among all it can read, public ones included, so a public repository
|
||||||
repository's Read collaborators out of an account's own list, so access to
|
needs no grant in Gitea: access to public repositories is managed in
|
||||||
public repositories is managed in configuration, and to private ones in Gitea.
|
configuration, and to private ones in Gitea.
|
||||||
- **A work item per PR,** scoped to its repository (L1). It closes when the PR
|
- **A work item per PR,** scoped to its repository (L1). It closes when the PR
|
||||||
merges or closes.
|
merges or closes.
|
||||||
- **What starts a run:** the PR becoming ready, a new push, or a new comment or
|
- **What starts a run:** the PR becoming ready, a new push, or a new comment or
|
||||||
|
|||||||
@@ -608,9 +608,9 @@ waits until two exist to compare. Moved, unchanged in behaviour:
|
|||||||
ready unless a draft or titled WIP; closed when the PR merges or closes.
|
ready unless a draft or titled WIP; closed when the PR merges or closes.
|
||||||
Every open PR is reviewed, those open when it starts included (operator).
|
Every open PR is reviewed, those open when it starts included (operator).
|
||||||
Since #153, `repositories` holds patterns (`dev/*`, `*/*`) matched against
|
Since #153, `repositories` holds patterns (`dev/*`, `*/*`) matched against
|
||||||
every repository it can read, public ones included (Gitea's own list for the
|
every repository it can read, public ones included, through Gitea's search
|
||||||
account leaves out a public repository's Read collaborators, found at
|
(`/repos/search`, which needs no `read:organization` scope); the read-only wall
|
||||||
bring-up); the read-only wall is checked every round, not only at startup.
|
is checked every round, not only at startup.
|
||||||
- **Reading (R3):** the repository at the PR's head and base, from Gitea's
|
- **Reading (R3):** the repository at the PR's head and base, from Gitea's
|
||||||
archives, unpacked into a scratch folder and read in Python (`read_file`,
|
archives, unpacked into a scratch folder and read in Python (`read_file`,
|
||||||
`list_files`, `search_code`), and its issues (`read_issue`,
|
`list_files`, `search_code`), and its issues (`read_issue`,
|
||||||
|
|||||||
@@ -421,14 +421,6 @@ class GiteaClient:
|
|||||||
return found
|
return found
|
||||||
raise GiteaError("too many repositories to list")
|
raise GiteaError("too many repositories to list")
|
||||||
|
|
||||||
def owner_repositories(self, owner: str) -> list[str]:
|
|
||||||
"""Every repository of an organization (or a user) this account can
|
|
||||||
read, as "owner/name"."""
|
|
||||||
try:
|
|
||||||
return self._paged_names(f"/orgs/{owner}/repos")
|
|
||||||
except GiteaNotFound:
|
|
||||||
return self._paged_names(f"/users/{owner}/repos")
|
|
||||||
|
|
||||||
def visible_repositories(self) -> list[str]:
|
def visible_repositories(self) -> list[str]:
|
||||||
"""Every repository this account can read, public ones included."""
|
"""Every repository this account can read, public ones included."""
|
||||||
return self._paged_names("/repos/search", key="data")
|
return self._paged_names("/repos/search", key="data")
|
||||||
|
|||||||
@@ -361,19 +361,6 @@ def test_readable_repositories_page_to_the_end() -> None:
|
|||||||
assert len(found) == PR_PAGE_SIZE + 2 and found[-1] == "o/r2-1"
|
assert len(found) == PR_PAGE_SIZE + 2 and found[-1] == "o/r2-1"
|
||||||
|
|
||||||
|
|
||||||
def test_owner_repositories_try_the_organization_then_the_user() -> None:
|
|
||||||
asked: list[str] = []
|
|
||||||
|
|
||||||
def handler(request: httpx.Request) -> httpx.Response:
|
|
||||||
asked.append(request.url.path)
|
|
||||||
if request.url.path.startswith("/api/v1/orgs/"):
|
|
||||||
return httpx.Response(404, json={"message": "not found"})
|
|
||||||
return httpx.Response(200, json=[{"full_name": "don/r"}])
|
|
||||||
|
|
||||||
assert client(handler).owner_repositories("don") == ["don/r"]
|
|
||||||
assert asked == ["/api/v1/orgs/don/repos", "/api/v1/users/don/repos"]
|
|
||||||
|
|
||||||
|
|
||||||
def test_visible_repositories_read_the_search_and_a_404_is_not_found() -> None:
|
def test_visible_repositories_read_the_search_and_a_404_is_not_found() -> None:
|
||||||
def handler(request: httpx.Request) -> httpx.Response:
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
if request.url.path.endswith("/repos/search"):
|
if request.url.path.endswith("/repos/search"):
|
||||||
|
|||||||
@@ -3,7 +3,10 @@
|
|||||||
The repositories are worked out again every round: ``repositories`` in the
|
The repositories are worked out again every round: ``repositories`` in the
|
||||||
configuration is a list of patterns ("dev/nisse", "dev/*", "*/*"), matched
|
configuration is a list of patterns ("dev/nisse", "dev/*", "*/*"), matched
|
||||||
against what the account can read, public repositories included; unset, it
|
against what the account can read, public repositories included; unset, it
|
||||||
is every repository the account was given (``/user/repos``). Access is
|
is every repository the account was given (``/user/repos``). Patterns are
|
||||||
|
matched against Gitea's search, which needs only the token's repository
|
||||||
|
scope: listing an organization's repositories would need
|
||||||
|
``read:organization`` too. Access is
|
||||||
managed in Gitea for private repositories and in the configuration for
|
managed in Gitea for private repositories and in the configuration for
|
||||||
public ones.
|
public ones.
|
||||||
|
|
||||||
@@ -35,22 +38,15 @@ def reviewed(
|
|||||||
"""The repositories to review, sorted."""
|
"""The repositories to review, sorted."""
|
||||||
if patterns is None:
|
if patterns is None:
|
||||||
return sorted(gitea.readable_repositories())
|
return sorted(gitea.readable_repositories())
|
||||||
found: set[str] = set()
|
found = {p for p in patterns if not _glob(p)} # ``check`` sees they're readable
|
||||||
for pattern in patterns:
|
globs = [p for p in patterns if _glob(p)]
|
||||||
owner, _, name = pattern.partition("/")
|
if globs:
|
||||||
if not _glob(owner) and not _glob(name):
|
visible = gitea.visible_repositories()
|
||||||
|
|
|||||||
found.add(pattern) # checked by ``check``: it must be readable
|
for pattern in globs:
|
||||||
continue
|
matched = {repo for repo in visible if fnmatchcase(repo, pattern)}
|
||||||
try:
|
if not matched:
|
||||||
candidates = (
|
oplog.emit("access.no_match", cause="matches nothing", pattern=pattern)
|
||||||
gitea.visible_repositories()
|
found |= matched
|
||||||
if _glob(owner)
|
|
||||||
else gitea.owner_repositories(owner)
|
|
||||||
)
|
|
||||||
except GiteaNotFound:
|
|
||||||
oplog.emit("access.no_owner", cause="no such owner", pattern=pattern)
|
|
||||||
continue
|
|
||||||
found |= {repo for repo in candidates if fnmatchcase(repo, pattern)}
|
|
||||||
return sorted(found)
|
return sorted(found)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ from types import FrameType
|
|||||||
|
|
||||||
from nisse_core.config import ConfigError
|
from nisse_core.config import ConfigError
|
||||||
from nisse_core.envfile import load_env_file
|
from nisse_core.envfile import load_env_file
|
||||||
|
from nisse_core.gitea import GiteaError
|
||||||
from nisse_review.access import AccessRefused
|
from nisse_review.access import AccessRefused
|
||||||
from nisse_review.build import StartupError, build
|
from nisse_review.build import StartupError, build
|
||||||
from nisse_review.config import ReviewConfig
|
from nisse_review.config import ReviewConfig
|
||||||
@@ -19,7 +20,7 @@ def main() -> int:
|
|||||||
try:
|
try:
|
||||||
config = ReviewConfig.load()
|
config = ReviewConfig.load()
|
||||||
built = build(config)
|
built = build(config)
|
||||||
except (ConfigError, StartupError) as exc:
|
except (ConfigError, StartupError, GiteaError) as exc:
|
||||||
print(f"nisse-review: {exc}", file=sys.stderr)
|
print(f"nisse-review: {exc}", file=sys.stderr)
|
||||||
return 2
|
return 2
|
||||||
|
|
||||||
|
|||||||
@@ -70,8 +70,7 @@ class FakeGitea:
|
|||||||
comments: list[tuple[str, int, str]] = field(default_factory=list)
|
comments: list[tuple[str, int, str]] = field(default_factory=list)
|
||||||
refuse_off_diff: bool = False
|
refuse_off_diff: bool = False
|
||||||
down: bool = False
|
down: bool = False
|
||||||
# Readable but not given (public): missing from /user/repos, as Gitea
|
# Readable but not given (public, no grant): missing from /user/repos.
|
||||||
# leaves a public repository's Read collaborators out of it.
|
|
||||||
public: set[str] = field(default_factory=set)
|
public: set[str] = field(default_factory=set)
|
||||||
|
|
||||||
def _up(self) -> None:
|
def _up(self) -> None:
|
||||||
@@ -89,13 +88,6 @@ class FakeGitea:
|
|||||||
self._up()
|
self._up()
|
||||||
return list(self.repos)
|
return list(self.repos)
|
||||||
|
|
||||||
def owner_repositories(self, owner: str) -> list[str]:
|
|
||||||
self._up()
|
|
||||||
found = [r for r in self.repos if r.startswith(f"{owner}/")]
|
|
||||||
if not found:
|
|
||||||
raise GiteaNotFound(f"no owner {owner}")
|
|
||||||
return found
|
|
||||||
|
|
||||||
def repository(self, repo: str) -> dict[str, Any]:
|
def repository(self, repo: str) -> dict[str, Any]:
|
||||||
if repo not in self.repos:
|
if repo not in self.repos:
|
||||||
raise GiteaNotFound(f"no repository {repo}")
|
raise GiteaNotFound(f"no repository {repo}")
|
||||||
|
|||||||
@@ -311,9 +311,8 @@ def test_an_account_that_can_write_is_refused(tmp_path: Path) -> None:
|
|||||||
def test_patterns_reach_public_repositories_the_account_was_not_given(
|
def test_patterns_reach_public_repositories_the_account_was_not_given(
|
||||||
tmp_path: Path,
|
tmp_path: Path,
|
||||||
) -> None:
|
) -> None:
|
||||||
# Gitea leaves a public repository's Read collaborators out of
|
# A public repository the account wasn't given isn't in /user/repos: a
|
||||||
# /user/repos: a pattern finds it all the same (option a: the
|
# pattern finds it all the same (option a: the configuration decides).
|
||||||
# configuration decides).
|
|
||||||
gitea = FakeGitea(
|
gitea = FakeGitea(
|
||||||
repos={"dev/nisse": [pr(7)], "dev/wordle": [], "infra/tofu": [pr(3)]},
|
repos={"dev/nisse": [pr(7)], "dev/wordle": [], "infra/tofu": [pr(3)]},
|
||||||
public={"dev/nisse", "dev/wordle", "infra/tofu"},
|
public={"dev/nisse", "dev/wordle", "infra/tofu"},
|
||||||
@@ -336,7 +335,7 @@ def test_a_configured_repository_it_cannot_read_is_left_out() -> None:
|
|||||||
patterns = {REPO, "o/gone", "nobody/*"}
|
patterns = {REPO, "o/gone", "nobody/*"}
|
||||||
assert check(gitea, reviewed(gitea, patterns, log), log) == [REPO] # type: ignore[arg-type]
|
assert check(gitea, reviewed(gitea, patterns, log), log) == [REPO] # type: ignore[arg-type]
|
||||||
assert '"access.unreadable"' in out.getvalue() and "o/gone" in out.getvalue()
|
assert '"access.unreadable"' in out.getvalue() and "o/gone" in out.getvalue()
|
||||||
assert '"access.no_owner"' in out.getvalue()
|
assert '"access.no_match"' in out.getvalue() and "nobody/*" in out.getvalue()
|
||||||
|
|
||||||
|
|
||||||
def test_write_access_granted_while_running_stops_it(tmp_path: Path) -> None:
|
def test_write_access_granted_while_running_stops_it(tmp_path: Path) -> None:
|
||||||
|
|||||||
Reference in new issue
Block a user
Nice simplification — one
visible_repositories()call now covers every glob, and theread:organization-dependent owner path is gone.The one thing this rests on your live verification for rather than the unit suite: Gitea's
/repos/searchwith an empty query returning every repository the account can read and honoringlimit/page. The only test of that path (test_visible_repositories_read_the_search_and_a_404_is_not_found) sees a single item on a single page, so a Gitea change that truncates the search result, ignores the limit, or requires a query term would slip past CI and only the bring-up would catch it. A cheap two-page/multi-item assertion in that test would pin the paging contract. Not a blocker — the live bring-up you describe is the real evidence.Agreed, and done in
6e697b5:test_visible_repositories_page_through_the_searchserves a full first page and a short second one, and checks that both pages are asked for with the samelimitand that every repository comes back. The live bring-up still checks it against the real Gitea.