swarm-ui, swarm-controller: the review try-out's repo and PR fields offer the repos and PRs to pick
Two read-only routes feed them: GET /api/pulls/repos lists the repos with an open pull request, and GET /api/pulls?repo=owner/name lists a repo's open pull requests, highest number first. The page offers the lists through the browser's own suggestion list and keeps both fields free text, since a run also works on a merged or closed PR.
This commit is contained in:
parent
60ea7fdc04
commit
9a4059e130
10 changed files with 399 additions and 7 deletions
|
|
@ -58,6 +58,32 @@ pub struct ConfigPrStatus {
|
|||
pub html_url: Option<String>,
|
||||
}
|
||||
|
||||
/// An open PR as the review try-out's PR dropdown lists it.
|
||||
#[derive(Clone, Debug, PartialEq, Eq, Serialize, ToSchema)]
|
||||
pub struct OpenPull {
|
||||
pub number: u64,
|
||||
pub title: String,
|
||||
/// The head commit the PR is at now. Absent only if Forgejo omitted it.
|
||||
pub head_sha: Option<String>,
|
||||
}
|
||||
|
||||
/// `pulls` as the dropdown's rows, newest (highest number) first. A PR
|
||||
/// without a usable number cannot be named, so it is left out.
|
||||
fn open_pulls(pulls: Vec<forgejo_api::structs::PullRequest>) -> Vec<OpenPull> {
|
||||
let mut rows: Vec<OpenPull> = pulls
|
||||
.into_iter()
|
||||
.filter_map(|pr| {
|
||||
Some(OpenPull {
|
||||
number: u64::try_from(pr.number?).ok()?,
|
||||
title: pr.title.unwrap_or_default(),
|
||||
head_sha: pr.head.and_then(|h| h.sha),
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
rows.sort_by_key(|r| std::cmp::Reverse(r.number));
|
||||
rows
|
||||
}
|
||||
|
||||
/// A PR as [`Client::pull_for_review`] reads it for the review classifier.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub struct PullForReview {
|
||||
|
|
@ -871,6 +897,35 @@ impl Client {
|
|||
/// already carries that count, so this stays one request per page
|
||||
/// regardless of how many repos exist.
|
||||
pub async fn list_repos_with_open_issues(&self) -> Result<Vec<String>> {
|
||||
Ok(Self::full_names_where(self.search_repos().await?, |r| {
|
||||
r.open_issues_count.unwrap_or(0) > 0
|
||||
}))
|
||||
}
|
||||
|
||||
/// Repos with at least one open pull request, as `owner/name` full
|
||||
/// names — the data source for the review try-out's repo dropdown. Same
|
||||
/// one-request-per-page shape as [`Self::list_repos_with_open_issues`],
|
||||
/// filtered on `Repository::open_pr_counter`.
|
||||
pub async fn list_repos_with_open_pulls(&self) -> Result<Vec<String>> {
|
||||
Ok(Self::full_names_where(self.search_repos().await?, |r| {
|
||||
r.open_pr_counter.unwrap_or(0) > 0
|
||||
}))
|
||||
}
|
||||
|
||||
/// The `owner/name` of each repo in `repos` that `keep` accepts.
|
||||
fn full_names_where(
|
||||
repos: Vec<forgejo_api::structs::Repository>,
|
||||
keep: impl Fn(&forgejo_api::structs::Repository) -> bool,
|
||||
) -> Vec<String> {
|
||||
repos
|
||||
.into_iter()
|
||||
.filter(|r| keep(r))
|
||||
.filter_map(|r| r.full_name)
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Every repo the forge hosts, from the search endpoint.
|
||||
async fn search_repos(&self) -> Result<Vec<forgejo_api::structs::Repository>> {
|
||||
const PAGE_SIZE: u32 = 50;
|
||||
// 40 pages at 50/page is 2000 repos — comfortably past any repo
|
||||
// count this instance has today; see `page_search_results` for why
|
||||
|
|
@ -887,11 +942,29 @@ impl Client {
|
|||
.data)
|
||||
})
|
||||
.await?;
|
||||
Ok(repos
|
||||
.into_iter()
|
||||
.filter(|r| r.open_issues_count.unwrap_or(0) > 0)
|
||||
.filter_map(|r| r.full_name)
|
||||
.collect())
|
||||
Ok(repos)
|
||||
}
|
||||
|
||||
/// The open pull requests of `owner/repo`, newest first — the data
|
||||
/// source for the review try-out's PR dropdown. A Forgejo 404 stays in
|
||||
/// the error chain for [`is_repo_not_found`] to find.
|
||||
pub async fn list_open_pulls(&self, owner: &str, repo: &str) -> Result<Vec<OpenPull>> {
|
||||
let query = RepoListPullRequestsQuery {
|
||||
state: Some(RepoListPullRequestsQueryState::Open),
|
||||
sort: None,
|
||||
milestone: None,
|
||||
labels: None,
|
||||
poster: None,
|
||||
base: None,
|
||||
head: None,
|
||||
};
|
||||
let pulls = self
|
||||
.api
|
||||
.repo_list_pull_requests(owner, repo, query)
|
||||
.all()
|
||||
.await
|
||||
.with_context(|| format!("list open pull requests in {owner}/{repo}"))?;
|
||||
Ok(open_pulls(pulls))
|
||||
}
|
||||
|
||||
/// Every open issue in `owner/repo`, each with its dependency-derived
|
||||
|
|
@ -1650,6 +1723,34 @@ impl Client {
|
|||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn open_pulls_are_listed_newest_first_and_skip_one_with_no_number() {
|
||||
// The generated type insists on `requested_reviewers` being present.
|
||||
let pull = |mut pr: serde_json::Value| {
|
||||
pr["requested_reviewers"] = serde_json::json!([]);
|
||||
serde_json::from_value(pr).expect("a pull request")
|
||||
};
|
||||
let pulls = vec![
|
||||
pull(serde_json::json!({"number": 7, "title": "older", "head": {"sha": "aaa"}})),
|
||||
pull(serde_json::json!({"title": "no number", "head": {"sha": "bbb"}})),
|
||||
pull(serde_json::json!({"number": 12, "title": "newer", "head": {"sha": "ccc"}})),
|
||||
pull(serde_json::json!({"number": 9, "head": {}})),
|
||||
];
|
||||
let want = |number, title: &str, head_sha: Option<&str>| OpenPull {
|
||||
number,
|
||||
title: title.to_owned(),
|
||||
head_sha: head_sha.map(str::to_owned),
|
||||
};
|
||||
assert_eq!(
|
||||
open_pulls(pulls),
|
||||
vec![
|
||||
want(12, "newer", Some("ccc")),
|
||||
want(9, "", None),
|
||||
want(7, "older", Some("aaa")),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
// Both scenarios live in ONE test, not two, deliberately: `cargo
|
||||
// test` runs tests in parallel by default, and two tests mutating
|
||||
// the same process-global env var would race each other. A single
|
||||
|
|
|
|||
|
|
@ -141,7 +141,7 @@ pub async fn get_issue_report(
|
|||
/// anything else — including a 404 on some *other* call this function
|
||||
/// doesn't cover, an outage, auth failure, etc. — stays 503, same as before
|
||||
/// this existed.
|
||||
fn issue_report_error_status(e: &anyhow::Error) -> axum::http::StatusCode {
|
||||
pub(crate) fn issue_report_error_status(e: &anyhow::Error) -> axum::http::StatusCode {
|
||||
match e.downcast_ref::<forgejo_api::ForgejoError>() {
|
||||
Some(fe) if forge::is_repo_not_found(fe) => axum::http::StatusCode::NOT_FOUND,
|
||||
_ => axum::http::StatusCode::SERVICE_UNAVAILABLE,
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ mod issue_report;
|
|||
mod linked_accounts;
|
||||
mod matrix_account;
|
||||
mod otel_http_client;
|
||||
mod pulls;
|
||||
mod queue_identity;
|
||||
mod read_policy;
|
||||
mod review_classifier;
|
||||
|
|
@ -768,6 +769,7 @@ fn socket_path() -> PathBuf {
|
|||
(name = "forge", description = "forge site admins (`swarmctl forge make-admin`)"),
|
||||
(name = "schedules", description = "scheduled prompts (`swarmctl schedule`)"),
|
||||
(name = "review-classifier", description = "trying review classifications against a PR (swarm-ui's try-out page)"),
|
||||
(name = "pulls", description = "open pull requests, for the try-out page's dropdowns"),
|
||||
)
|
||||
)]
|
||||
struct ApiDoc;
|
||||
|
|
@ -3333,6 +3335,8 @@ fn build_app(state: AppState) -> axum::Router {
|
|||
.routes(routes!(issue_report::get_repos))
|
||||
.routes(routes!(issue_report::get_issue_report_all))
|
||||
.routes(routes!(issue_report::get_issue_report))
|
||||
.routes(routes!(pulls::get_pull_repos))
|
||||
.routes(routes!(pulls::get_pulls))
|
||||
.routes(routes!(review_classifier::post_try_out))
|
||||
.routes(routes!(review_classifier::get_try_out))
|
||||
.routes(routes!(review_classifier::get_rules))
|
||||
|
|
|
|||
156
swarm-controller/src/pulls.rs
Normal file
156
swarm-controller/src/pulls.rs
Normal file
|
|
@ -0,0 +1,156 @@
|
|||
//! The data the review try-out's dropdowns list: the repos that have an open
|
||||
//! pull request, and a repo's open pull requests. Read-only views of the
|
||||
//! forge, over the controller's own forge token. The try-out page still takes
|
||||
//! a repo or a PR typed by hand, since the run also works on a PR that is
|
||||
//! merged or closed.
|
||||
|
||||
use axum::Json;
|
||||
use axum::extract::rejection::QueryRejection;
|
||||
use axum::extract::{Query, State};
|
||||
use axum::http::StatusCode;
|
||||
use problem_details::ProblemDetails;
|
||||
use serde::Deserialize;
|
||||
use utoipa::IntoParams;
|
||||
|
||||
use crate::forge::{self, OpenPull};
|
||||
use crate::issue_report::issue_report_error_status;
|
||||
use crate::review_classifier::split_repo;
|
||||
use crate::{AppState, error_problem};
|
||||
|
||||
/// Whose open pull requests to list.
|
||||
#[derive(Debug, Deserialize, IntoParams)]
|
||||
pub(crate) struct PullsQuery {
|
||||
/// `owner/name`.
|
||||
repo: String,
|
||||
}
|
||||
|
||||
/// Repos with at least one open pull request, as `owner/name`.
|
||||
#[utoipa::path(
|
||||
get,
|
||||
path = "/api/pulls/repos",
|
||||
responses(
|
||||
(status = 200, description = "repos with at least one open pull request, as owner/name", body = Vec<String>),
|
||||
(status = 503, description = "this host has no forge token yet; or the forge failed or \
|
||||
did not answer within two minutes (problem+json)", body = String),
|
||||
),
|
||||
tag = "pulls"
|
||||
)]
|
||||
pub(crate) async fn get_pull_repos(
|
||||
State(state): State<AppState>,
|
||||
) -> Result<Json<Vec<String>>, ProblemDetails> {
|
||||
let Some(client) = state.forge.get() else {
|
||||
return Err(no_forge());
|
||||
};
|
||||
let repos = within_deadline("pull request repos", client.list_repos_with_open_pulls()).await?;
|
||||
Ok(Json(repos.map_err(|e| unavailable(&e))?))
|
||||
}
|
||||
|
||||
/// A repo's open pull requests, newest first.
|
||||
#[utoipa::path(
|
||||
get,
|
||||
path = "/api/pulls",
|
||||
params(PullsQuery),
|
||||
responses(
|
||||
(status = 200, description = "the repo's open pull requests, highest number first", body = Vec<OpenPull>),
|
||||
(status = 400, description = "no `repo`, or a repo that is not owner/name with segments \
|
||||
of `[A-Za-z0-9._-]` other than `.` and `..` (problem+json)", body = String),
|
||||
(status = 404, description = "the forge knows no such repo (problem+json)", body = String),
|
||||
(status = 503, description = "this host has no forge token yet; or the forge failed or \
|
||||
did not answer within two minutes (problem+json)", body = String),
|
||||
),
|
||||
tag = "pulls"
|
||||
)]
|
||||
pub(crate) async fn get_pulls(
|
||||
State(state): State<AppState>,
|
||||
query: Result<Query<PullsQuery>, QueryRejection>,
|
||||
) -> Result<Json<Vec<OpenPull>>, ProblemDetails> {
|
||||
let Query(query) = query.map_err(|e| error_problem(StatusCode::BAD_REQUEST, &e.body_text()))?;
|
||||
let (owner, name) =
|
||||
split_repo(&query.repo).map_err(|d| error_problem(StatusCode::BAD_REQUEST, &d))?;
|
||||
let Some(client) = state.forge.get() else {
|
||||
return Err(no_forge());
|
||||
};
|
||||
let pulls = within_deadline("open pull requests", client.list_open_pulls(owner, name))
|
||||
.await?
|
||||
.map_err(|e| {
|
||||
if issue_report_error_status(&e) == StatusCode::NOT_FOUND {
|
||||
error_problem(StatusCode::NOT_FOUND, &format!("{owner}/{name} not found"))
|
||||
} else {
|
||||
unavailable(&e)
|
||||
}
|
||||
})?;
|
||||
Ok(Json(pulls))
|
||||
}
|
||||
|
||||
fn no_forge() -> ProblemDetails {
|
||||
error_problem(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"no forge access on this host yet",
|
||||
)
|
||||
}
|
||||
|
||||
/// `fut`'s answer, or a 503 if the forge did not answer by the deadline.
|
||||
async fn within_deadline<T>(
|
||||
what: &str,
|
||||
fut: impl std::future::Future<Output = T>,
|
||||
) -> Result<T, ProblemDetails> {
|
||||
forge::within_forge_deadline(what, fut)
|
||||
.await
|
||||
.ok_or_else(|| {
|
||||
error_problem(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
"the forge did not answer within the deadline",
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
fn unavailable(e: &anyhow::Error) -> ProblemDetails {
|
||||
let detail = format!("{e:#}");
|
||||
tracing::warn!(error = %detail, "listing pull requests failed");
|
||||
error_problem(StatusCode::SERVICE_UNAVAILABLE, &detail)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use axum::body::Body;
|
||||
use axum::http::{Request, StatusCode};
|
||||
use tower::ServiceExt as _;
|
||||
|
||||
async fn get(uri: &str) -> (StatusCode, serde_json::Value) {
|
||||
let (state, _) = crate::tests::state_with_roster();
|
||||
let req = Request::get(uri).body(Body::empty()).expect("a request");
|
||||
let res = crate::build_app(state)
|
||||
.oneshot(req)
|
||||
.await
|
||||
.expect("infallible");
|
||||
let status = res.status();
|
||||
let bytes = axum::body::to_bytes(res.into_body(), usize::MAX)
|
||||
.await
|
||||
.expect("a body");
|
||||
(status, serde_json::from_slice(&bytes).unwrap_or_default())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_bad_or_missing_repo_is_400_before_the_forge_is_asked() {
|
||||
for (query, says) in [
|
||||
("", "repo"),
|
||||
("?repo=o", "owner/name"),
|
||||
("?repo=o/..", "owner/name"),
|
||||
("?repo=a/b/c", "owner/name"),
|
||||
] {
|
||||
let (status, problem) = get(&format!("/api/pulls{query}")).await;
|
||||
assert_eq!(status, StatusCode::BAD_REQUEST, "{query}: {problem}");
|
||||
let detail = problem["detail"].as_str().expect("a problem+json detail");
|
||||
assert!(detail.contains(says), "{query}: {detail}");
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn both_lists_without_forge_access_on_this_host_are_503() {
|
||||
for uri in ["/api/pulls?repo=o/r", "/api/pulls/repos"] {
|
||||
let (status, problem) = get(uri).await;
|
||||
assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{uri}: {problem}");
|
||||
assert_eq!(problem["detail"], "no forge access on this host yet");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -551,7 +551,7 @@ impl Run {
|
|||
}
|
||||
|
||||
/// `repo` as its owner and name, or what is wrong with it.
|
||||
fn split_repo(repo: &str) -> Result<(&str, &str), String> {
|
||||
pub(crate) fn split_repo(repo: &str) -> Result<(&str, &str), String> {
|
||||
repo.split_once('/')
|
||||
.filter(|(o, n)| segment(o) && segment(n))
|
||||
.ok_or_else(|| format!("repo {repo:?} is not owner/name"))
|
||||
|
|
|
|||
Loading…
Reference in a new issue