let notification flows start from submitted pull request reviews
This commit is contained in:
parent
31f8a521e0
commit
dd2f0d3ece
7 changed files with 142 additions and 11 deletions
|
|
@ -420,7 +420,8 @@ itself, all under `https://<swarm.domain>/webhook/forge/`:
|
||||||
- an instance-wide `pull_request_review_request` system hook, which starts
|
- an instance-wide `pull_request_review_request` system hook, which starts
|
||||||
the [review classifier](review-classifier.md#trigger-and-lifecycle)
|
the [review classifier](review-classifier.md#trigger-and-lifecycle)
|
||||||
- instance-wide `issues` + `issue_assign` and `pull_request` +
|
- instance-wide `issues` + `issue_assign` and `pull_request` +
|
||||||
`pull_request_assign` + `pull_request_sync` system hooks, whose deliveries it
|
`pull_request_assign` + `pull_request_sync` + `pull_request_review_approved` +
|
||||||
|
`pull_request_review_rejected` + `pull_request_review_comment` system hooks, whose deliveries it
|
||||||
keeps in the `SWARM_EVENTS` stream for the
|
keeps in the `SWARM_EVENTS` stream for the
|
||||||
[notification router](notify-router.md#the-path)
|
[notification router](notify-router.md#the-path)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -41,8 +41,9 @@ router notices an unassign only on an issue it has seen an assign event for.
|
||||||
|
|
||||||
1. swarm-controller registers two instance-wide hooks, one for the `issues`
|
1. swarm-controller registers two instance-wide hooks, one for the `issues`
|
||||||
and `issue_assign` events at `/webhook/forge/issues` and one for
|
and `issue_assign` events at `/webhook/forge/issues` and one for
|
||||||
`pull_request`, `pull_request_assign` and `pull_request_sync` (a push to an
|
`pull_request`, `pull_request_assign`, `pull_request_sync` (a push to an
|
||||||
open PR's branch) at `/webhook/forge/pull-request`. At each start it adds
|
open PR's branch) and the submitted-review events `pull_request_review_approved`,
|
||||||
|
`pull_request_review_rejected` and `pull_request_review_comment` at `/webhook/forge/pull-request`. At each start it adds
|
||||||
any of these events a registered hook lacks.
|
any of these events a registered hook lacks.
|
||||||
2. On each delivery the controller checks the HMAC and publishes the body,
|
2. On each delivery the controller checks the HMAC and publishes the body,
|
||||||
unchanged, to the `SWARM_EVENTS` stream on
|
unchanged, to the `SWARM_EVENTS` stream on
|
||||||
|
|
@ -90,8 +91,8 @@ switched off, and the custom flows. Only the controller writes it, through
|
||||||
`GET` and `PUT /api/notify/flows`. A `PUT` replaces the whole document in one
|
`GET` and `PUT /api/notify/flows`. A `PUT` replaces the whole document in one
|
||||||
compare-and-set. The controller answers 400 unless every flow passes the tree
|
compare-and-set. The controller answers 400 unless every flow passes the tree
|
||||||
check above and reads only what exists: a source event from `issues`,
|
check above and reads only what exists: a source event from `issues`,
|
||||||
`issue_assign`, `pull_request`, `pull_request_assign` or `pull_request_sync`,
|
`issue_assign`, `pull_request`, `pull_request_assign`, `pull_request_sync` or one of the
|
||||||
and condition and recipient fields from the event's field list. The same goes for the `{field}`
|
three `pull_request_review_*` events, and condition and recipient fields from the event's field list. The same goes for the `{field}`
|
||||||
placeholders in a todo's key and text. A `PUT` must carry the `revision` it read, `0` for no
|
placeholders in a todo's key and text. A `PUT` must carry the `revision` it read, `0` for no
|
||||||
document, and gets 409 when the stored revision is another. Swarm UI's
|
document, and gets 409 when the stored revision is another. Swarm UI's
|
||||||
[`/notifications`](ui.md#notification-flows) page shows the document and
|
[`/notifications`](ui.md#notification-flows) page shows the document and
|
||||||
|
|
@ -156,6 +157,21 @@ its own request queue, which replaces a waiting request for the same PR. A
|
||||||
repo the classifier's account can't read, or a PR that account opened, answers
|
repo the classifier's account can't read, or a PR that account opened, answers
|
||||||
`error:`.
|
`error:`.
|
||||||
|
|
||||||
|
**Reviews as a source.** A submitted review arrives as a pull request
|
||||||
|
payload: `pull_request` and `repository` as for any PR event, `action` is
|
||||||
|
`reviewed`, and `sender` is the reviewer. Approving, requesting changes and
|
||||||
|
commenting are three event types, so a flow picks the verdict with its source
|
||||||
|
and reads the reviewer with a condition on `sender`. A flow that asks argus to
|
||||||
|
review once the classifier has approved:
|
||||||
|
|
||||||
|
| node | setting |
|
||||||
|
| ---------------- | ----------------------------------- |
|
||||||
|
| source | `pull_request_review_approved` |
|
||||||
|
| match | `sender` equals `review-classifier` |
|
||||||
|
| request_reviewer | `reviewer` = `argus` |
|
||||||
|
|
||||||
|
The skip rule above applies: the router asks for argus's review once per head.
|
||||||
|
|
||||||
**What the router runs.** It reads the key at most every 10 seconds, when an
|
**What the router runs.** It reads the key at most every 10 seconds, when an
|
||||||
event arrives, and runs the built-ins you left on, then the custom flows that
|
event arrives, and runs the built-ins you left on, then the custom flows that
|
||||||
are on and pass the tree check. It logs a custom flow that fails the check,
|
are on and pass the tree check. It logs a custom flow that fails the check,
|
||||||
|
|
|
||||||
|
|
@ -119,6 +119,10 @@ export const FALLBACK_SCHEMA: FlowSchema = {
|
||||||
"issue_assign",
|
"issue_assign",
|
||||||
"pull_request",
|
"pull_request",
|
||||||
"pull_request_assign",
|
"pull_request_assign",
|
||||||
|
"pull_request_sync",
|
||||||
|
"pull_request_review_approved",
|
||||||
|
"pull_request_review_rejected",
|
||||||
|
"pull_request_review_comment",
|
||||||
].map((name) => ({ name, description: "" })),
|
].map((name) => ({ name, description: "" })),
|
||||||
fields: [
|
fields: [
|
||||||
"event",
|
"event",
|
||||||
|
|
|
||||||
|
|
@ -1780,12 +1780,19 @@ fn hook_registration(kind: DeliveryKind) -> (&'static [&'static str], HookScope<
|
||||||
// classifier's account access can request its review.
|
// classifier's account access can request its review.
|
||||||
DeliveryKind::ReviewRequest => (&["pull_request_review_request"], HookScope::Instance),
|
DeliveryKind::ReviewRequest => (&["pull_request_review_request"], HookScope::Instance),
|
||||||
// Instance-wide, for the notification router, which reads the assign
|
// Instance-wide, for the notification router, which reads the assign
|
||||||
// events and a push to a PR's branch. They are named: Forgejo's web
|
// events, a push to a PR's branch and submitted reviews. They are named: Forgejo's web
|
||||||
// form shows each as a box of its own, so a hook edited there can drop
|
// form shows each as a box of its own, so a hook edited there can drop
|
||||||
// one while keeping `issues` or `pull_request`.
|
// one while keeping `issues` or `pull_request`.
|
||||||
DeliveryKind::Issues => (&["issues", "issue_assign"], HookScope::Instance),
|
DeliveryKind::Issues => (&["issues", "issue_assign"], HookScope::Instance),
|
||||||
DeliveryKind::PullRequest => (
|
DeliveryKind::PullRequest => (
|
||||||
&["pull_request", "pull_request_assign", "pull_request_sync"],
|
&[
|
||||||
|
"pull_request",
|
||||||
|
"pull_request_assign",
|
||||||
|
"pull_request_sync",
|
||||||
|
"pull_request_review_approved",
|
||||||
|
"pull_request_review_rejected",
|
||||||
|
"pull_request_review_comment",
|
||||||
|
],
|
||||||
HookScope::Instance,
|
HookScope::Instance,
|
||||||
),
|
),
|
||||||
}
|
}
|
||||||
|
|
@ -2131,7 +2138,14 @@ mod tests {
|
||||||
assert_eq!(events(DeliveryKind::Issues), ["issues", "issue_assign"]);
|
assert_eq!(events(DeliveryKind::Issues), ["issues", "issue_assign"]);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
events(DeliveryKind::PullRequest),
|
events(DeliveryKind::PullRequest),
|
||||||
["pull_request", "pull_request_assign", "pull_request_sync"]
|
[
|
||||||
|
"pull_request",
|
||||||
|
"pull_request_assign",
|
||||||
|
"pull_request_sync",
|
||||||
|
"pull_request_review_approved",
|
||||||
|
"pull_request_review_rejected",
|
||||||
|
"pull_request_review_comment",
|
||||||
|
]
|
||||||
);
|
);
|
||||||
// The events a stored flow's source may name are the ones the hooks
|
// The events a stored flow's source may name are the ones the hooks
|
||||||
// deliver to the router.
|
// deliver to the router.
|
||||||
|
|
|
||||||
|
|
@ -233,8 +233,8 @@ pub(super) enum DeliveryKind {
|
||||||
/// assigned, …), kept for the notification router (`crate::forge_events`).
|
/// assigned, …), kept for the notification router (`crate::forge_events`).
|
||||||
#[strum(serialize = "issues")]
|
#[strum(serialize = "issues")]
|
||||||
Issues,
|
Issues,
|
||||||
/// Every `pull_request` event on every repo in the instance, kept the
|
/// Every `pull_request` event, and every submitted review, on every repo in
|
||||||
/// same way.
|
/// the instance, kept the same way.
|
||||||
#[strum(serialize = "pull-request")]
|
#[strum(serialize = "pull-request")]
|
||||||
PullRequest,
|
PullRequest,
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -78,7 +78,7 @@ pub fn route(
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
use swarm_wire::notify_flows::builtins;
|
use swarm_wire::notify_flows::{Condition, Edge, Node, NodeKind, builtins};
|
||||||
|
|
||||||
fn delivery(action: &str, assignees: &[&str]) -> Delivery {
|
fn delivery(action: &str, assignees: &[&str]) -> Delivery {
|
||||||
Delivery {
|
Delivery {
|
||||||
|
|
@ -248,4 +248,88 @@ mod tests {
|
||||||
assert!(r.actions.is_empty());
|
assert!(r.actions.is_empty());
|
||||||
assert_eq!(r.next, before);
|
assert_eq!(r.next, before);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The body Forgejo sends for a submitted review: a pull request payload
|
||||||
|
/// whose `sender` is the reviewer.
|
||||||
|
fn review_body(reviewer: &str) -> Vec<u8> {
|
||||||
|
serde_json::to_vec(&serde_json::json!({
|
||||||
|
"action": "reviewed",
|
||||||
|
"number": 9,
|
||||||
|
"pull_request": {
|
||||||
|
"number": 9,
|
||||||
|
"title": "t",
|
||||||
|
"html_url": "u",
|
||||||
|
"assignees": [],
|
||||||
|
},
|
||||||
|
"repository": { "id": 1, "full_name": "o/r" },
|
||||||
|
"sender": { "login": reviewer },
|
||||||
|
"review": { "type": "pull_request_review_approved", "content": "" },
|
||||||
|
}))
|
||||||
|
.expect("encodes")
|
||||||
|
}
|
||||||
|
|
||||||
|
/// "A review by the classifier on a PR → request argus's review", as a
|
||||||
|
/// user builds it: valid, routed from the delivery body, and gated on the
|
||||||
|
/// reviewer.
|
||||||
|
#[test]
|
||||||
|
fn a_review_by_the_classifier_requests_argus() {
|
||||||
|
let id = |s: &str| s.to_owned();
|
||||||
|
let flow = Flow {
|
||||||
|
id: id("classifier-then-argus"),
|
||||||
|
name: id("classifier-then-argus"),
|
||||||
|
enabled: true,
|
||||||
|
nodes: vec![
|
||||||
|
Node {
|
||||||
|
id: id("source"),
|
||||||
|
kind: NodeKind::Source {
|
||||||
|
events: vec![id("pull_request_review_approved")],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
Node {
|
||||||
|
id: id("match"),
|
||||||
|
kind: NodeKind::Match {
|
||||||
|
when: vec![Condition {
|
||||||
|
field: id("sender"),
|
||||||
|
equals: id("review-classifier"),
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
Node {
|
||||||
|
id: id("request"),
|
||||||
|
kind: NodeKind::RequestReviewer {
|
||||||
|
reviewer: id("argus"),
|
||||||
|
},
|
||||||
|
},
|
||||||
|
],
|
||||||
|
edges: vec![
|
||||||
|
Edge {
|
||||||
|
from: id("source"),
|
||||||
|
to: id("match"),
|
||||||
|
},
|
||||||
|
Edge {
|
||||||
|
from: id("match"),
|
||||||
|
to: id("request"),
|
||||||
|
},
|
||||||
|
],
|
||||||
|
};
|
||||||
|
assert_eq!(flow.validate(), Ok(()));
|
||||||
|
let flows = [flow];
|
||||||
|
let routed = |event: &str, reviewer: &str| {
|
||||||
|
let d = Delivery::parse(&review_body(reviewer)).expect("parses");
|
||||||
|
route(event, &d, &Snapshot::default(), &flows, |_| true).requests
|
||||||
|
};
|
||||||
|
assert_eq!(
|
||||||
|
routed("pull_request_review_approved", "review-classifier"),
|
||||||
|
[ReviewRequest {
|
||||||
|
flow: "classifier-then-argus".to_owned(),
|
||||||
|
node: "request".to_owned(),
|
||||||
|
repo: "o/r".to_owned(),
|
||||||
|
number: 9,
|
||||||
|
reviewer: "argus".to_owned(),
|
||||||
|
}]
|
||||||
|
);
|
||||||
|
// Another reviewer, and another review event, request nothing.
|
||||||
|
assert!(routed("pull_request_review_approved", "mara").is_empty());
|
||||||
|
assert!(routed("pull_request_review_rejected", "review-classifier").is_empty());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -62,6 +62,18 @@ pub const SOURCE_EVENTS: &[(&str, &str)] = &[
|
||||||
"pull_request_sync",
|
"pull_request_sync",
|
||||||
"A push to the branch of an open pull request",
|
"A push to the branch of an open pull request",
|
||||||
),
|
),
|
||||||
|
(
|
||||||
|
"pull_request_review_approved",
|
||||||
|
"A review submitted on a pull request that approves it",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"pull_request_review_rejected",
|
||||||
|
"A review submitted on a pull request that asks for changes",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"pull_request_review_comment",
|
||||||
|
"A review submitted on a pull request that only comments",
|
||||||
|
),
|
||||||
];
|
];
|
||||||
|
|
||||||
/// The fields of an event a condition or a recipient can name, with what each
|
/// The fields of an event a condition or a recipient can name, with what each
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue