fix(hive-forge): tolerate an empty CI state in pr-status + pr-merge
Forgejo reports `"state": ""` in the combined-status response for a commit that has no CI contexts at all. The typed `forgejo-api` client models that field as an enum with no empty variant, so deserialization failed and both verbs died outright — on exactly the pull requests where "no CI ran here" is the useful answer. `pr-merge` was the worse of the two: the crash sat in its pre-merge readiness check, blocking a merge it should have waved through. Route both call sites through the existing raw-JSON escape hatch (`Client::get_api_json`), which exists for this failure mode: the crate pins one schema while the server tracks the latest release line. A lenient local `CombinedStatus` keeps `state` a plain `String` and the per-context statuses as opaque values, so an empty or unknown state is reported rather than fatal. `status_state_str` and its enum mapping go away with it. Closes #2735
This commit is contained in:
parent
fd40138f5a
commit
1219f31c2f
2 changed files with 57 additions and 50 deletions
|
|
@ -15,10 +15,10 @@
|
|||
use anyhow::{Result, bail};
|
||||
use clap::{Args as ClapArgs, ValueEnum};
|
||||
use forgejo_api::structs::{
|
||||
CommitStatusState, MergePullRequestOption, MergePullRequestOptionDo, PullRequest, StateType,
|
||||
MergePullRequestOption, MergePullRequestOptionDo, PullRequest, StateType,
|
||||
};
|
||||
|
||||
use crate::client::{Client, index, split_repo};
|
||||
use crate::client::{Client, index};
|
||||
|
||||
/// Merge strategy. Squash is deliberately omitted (hive convention: keep the
|
||||
/// per-commit history, so a squash option isn't exposed).
|
||||
|
|
@ -136,17 +136,11 @@ fn check_ready(client: &Client, repo: &str, number: u64, pull: &PullRequest) ->
|
|||
}
|
||||
|
||||
if let Some(sha) = pull.head.as_ref().and_then(|h| h.sha.as_deref()) {
|
||||
let (owner, name) = split_repo(repo)?;
|
||||
let (_, combined) = client
|
||||
.api()
|
||||
.repo_get_combined_status_by_ref(owner, name, sha)
|
||||
.send()?;
|
||||
let state = combined.state;
|
||||
let has_statuses = combined.statuses.as_ref().is_some_and(|a| !a.is_empty());
|
||||
let combined = super::pr_status::fetch_combined_in(client, repo, sha)?;
|
||||
// An empty status set means no CI is configured — not a blocker.
|
||||
// Anything other than success once CI exists blocks the merge.
|
||||
if has_statuses && state != Some(CommitStatusState::Success) {
|
||||
let state = super::pr_status::status_state_str(state);
|
||||
if !combined.statuses.is_empty() && combined.state != "success" {
|
||||
let state = &combined.state;
|
||||
bail!(
|
||||
"pr-merge: PR #{number} CI is not green (state: {state}). Wait for green, or pass --force."
|
||||
);
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@
|
|||
|
||||
use anyhow::{Context, Result, bail};
|
||||
use clap::Args as ClapArgs;
|
||||
use forgejo_api::structs::{CommitStatusState, IssueGetCommentsQuery};
|
||||
use forgejo_api::structs::IssueGetCommentsQuery;
|
||||
use serde_json::Value;
|
||||
|
||||
use crate::client::{Client, index};
|
||||
|
|
@ -44,19 +44,21 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
}
|
||||
}
|
||||
|
||||
/// The wire string for a combined/per-context CI status state, matching
|
||||
/// what the raw API emitted (`""` when absent). Shared with `pr-merge`
|
||||
/// for its "CI is not green" message.
|
||||
pub(crate) fn status_state_str(state: Option<CommitStatusState>) -> &'static str {
|
||||
match state {
|
||||
Some(CommitStatusState::Pending) => "pending",
|
||||
Some(CommitStatusState::Success) => "success",
|
||||
Some(CommitStatusState::Error) => "error",
|
||||
Some(CommitStatusState::Failure) => "failure",
|
||||
Some(CommitStatusState::Warning) => "warning",
|
||||
Some(CommitStatusState::Skipped) => "skipped",
|
||||
None => "",
|
||||
}
|
||||
/// Lenient mirror of the combined-status response.
|
||||
///
|
||||
/// The typed client models `state` as an enum, but a commit with no CI
|
||||
/// contexts at all comes back as `"state": ""` — not a member of that
|
||||
/// enum — so the typed call fails to deserialize and the whole verb
|
||||
/// dies on exactly the PRs where "no CI ran" is the useful answer.
|
||||
/// Keeping `state` a plain `String` (and the per-context statuses as
|
||||
/// opaque `Value`s, which the render helpers already walk) means an
|
||||
/// unknown or empty state is reported rather than fatal.
|
||||
#[derive(serde::Deserialize, Default)]
|
||||
pub(crate) struct CombinedStatus {
|
||||
#[serde(default)]
|
||||
pub state: String,
|
||||
#[serde(default)]
|
||||
pub statuses: Vec<Value>,
|
||||
}
|
||||
|
||||
/// CI-only path for an explicit commit. Exit code mirrors the CI verdict.
|
||||
|
|
@ -160,23 +162,20 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
|
|||
}
|
||||
}
|
||||
|
||||
/// Fetch the combined commit status for `sha` in the client's repo.
|
||||
/// Goes through the raw-JSON escape hatch rather than the typed client
|
||||
/// so an empty (no-CI) state stays reportable — see [`CombinedStatus`].
|
||||
pub(crate) fn fetch_combined_in(client: &Client, repo: &str, sha: &str) -> Result<CombinedStatus> {
|
||||
let (owner, name) = crate::client::split_repo(repo)?;
|
||||
client.get_api_json(&format!("/repos/{owner}/{name}/commits/{sha}/status"), &[])
|
||||
}
|
||||
|
||||
/// Fetch the combined commit status: `(overall_state, statuses[])`.
|
||||
/// Statuses ride as their serialized (API-shape) JSON so the render
|
||||
/// helpers stay pure `Value` walkers.
|
||||
/// Statuses ride as their API-shape JSON so the render helpers stay
|
||||
/// pure `Value` walkers.
|
||||
fn fetch_combined(client: &Client, sha: &str) -> Result<(String, Vec<Value>)> {
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
let (_, combined) = client
|
||||
.api()
|
||||
.repo_get_combined_status_by_ref(owner, name, sha)
|
||||
.send()?;
|
||||
let state = status_state_str(combined.state).to_owned();
|
||||
let statuses = combined
|
||||
.statuses
|
||||
.unwrap_or_default()
|
||||
.iter()
|
||||
.map(serde_json::to_value)
|
||||
.collect::<Result<Vec<_>, _>>()?;
|
||||
Ok((state, statuses))
|
||||
let combined = fetch_combined_in(client, client.repo(), sha)?;
|
||||
Ok((combined.state, combined.statuses))
|
||||
}
|
||||
|
||||
/// The most recent issue comment on the PR, as `(login, created_at)`.
|
||||
|
|
@ -381,16 +380,30 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn status_state_str_matches_wire_names() {
|
||||
assert_eq!(
|
||||
status_state_str(Some(CommitStatusState::Success)),
|
||||
"success"
|
||||
);
|
||||
assert_eq!(
|
||||
status_state_str(Some(CommitStatusState::Failure)),
|
||||
"failure"
|
||||
);
|
||||
assert_eq!(status_state_str(None), "");
|
||||
fn combined_status_accepts_empty_state() {
|
||||
// A commit with no CI contexts: Forgejo emits `"state": ""`,
|
||||
// which the typed client's enum rejects outright.
|
||||
let c: CombinedStatus =
|
||||
serde_json::from_str(r#"{"state":"","statuses":[],"sha":"abc"}"#).unwrap();
|
||||
assert_eq!(c.state, "");
|
||||
assert!(c.statuses.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn combined_status_keeps_known_state_and_contexts() {
|
||||
let c: CombinedStatus = serde_json::from_str(
|
||||
r#"{"state":"success","statuses":[{"status":"success","context":"ci"}]}"#,
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(c.state, "success");
|
||||
assert_eq!(c.statuses.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn combined_status_tolerates_missing_fields() {
|
||||
let c: CombinedStatus = serde_json::from_str("{}").unwrap();
|
||||
assert_eq!(c.state, "");
|
||||
assert!(c.statuses.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Reference in a new issue