refactor(#1474): replace too_many_arguments allows on pub fns with param structs
This commit is contained in:
damocles
mara
parent
7c9954ceec
commit
3130e56cfb
9 changed files with 407 additions and 324 deletions
|
|
@ -259,6 +259,22 @@ impl Default for RebuildQueue {
|
|||
}
|
||||
}
|
||||
|
||||
/// Full-shape submit spec for [`RebuildQueue::enqueue_full`] — every
|
||||
/// `QueueEntry` field settable at submit time. The thinner `enqueue`
|
||||
/// / `enqueue_with_inputs` / `enqueue_with_perm` wrappers build this
|
||||
/// for the common cases.
|
||||
pub struct FullEnqueue {
|
||||
pub kind: QueueKind,
|
||||
pub agent: String,
|
||||
pub source: QueueSource,
|
||||
pub reason: String,
|
||||
pub parent_id: Option<u64>,
|
||||
pub inputs: Vec<String>,
|
||||
pub approval_id: Option<i64>,
|
||||
pub perm_payload: Option<PermPayload>,
|
||||
pub depends_on: Vec<u64>,
|
||||
}
|
||||
|
||||
impl RebuildQueue {
|
||||
pub fn new() -> Self {
|
||||
Self::default()
|
||||
|
|
@ -294,17 +310,17 @@ impl RebuildQueue {
|
|||
reason: String,
|
||||
parent_id: Option<u64>,
|
||||
) -> u64 {
|
||||
self.enqueue_full(
|
||||
self.enqueue_full(FullEnqueue {
|
||||
kind,
|
||||
agent,
|
||||
source,
|
||||
reason,
|
||||
parent_id,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
Vec::new(),
|
||||
)
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
/// Same as `enqueue` but carries an `inputs` payload — used by
|
||||
|
|
@ -321,17 +337,17 @@ impl RebuildQueue {
|
|||
parent_id: Option<u64>,
|
||||
inputs: Vec<String>,
|
||||
) -> u64 {
|
||||
self.enqueue_full(
|
||||
self.enqueue_full(FullEnqueue {
|
||||
kind,
|
||||
agent,
|
||||
source,
|
||||
reason,
|
||||
parent_id,
|
||||
inputs,
|
||||
None,
|
||||
None,
|
||||
Vec::new(),
|
||||
)
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
/// Enqueue a `PermChange` entry for `agent`. The worker applies the
|
||||
|
|
@ -344,17 +360,17 @@ impl RebuildQueue {
|
|||
reason: String,
|
||||
payload: PermPayload,
|
||||
) -> u64 {
|
||||
self.enqueue_full(
|
||||
QueueKind::PermChange,
|
||||
self.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::PermChange,
|
||||
agent,
|
||||
source,
|
||||
reason,
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
Some(payload),
|
||||
Vec::new(),
|
||||
)
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: Some(payload),
|
||||
depends_on: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
/// Full-shape enqueue — every `QueueEntry` field that's settable
|
||||
|
|
@ -362,27 +378,18 @@ impl RebuildQueue {
|
|||
/// `enqueue_with_perm` delegate to this; the approval-driven POST
|
||||
/// handlers call it directly with the source row's id so the
|
||||
/// worker can re-fetch the kind-specific payload.
|
||||
// 10 args: the queue entry has 6 independent submit-time fields plus
|
||||
// four kind-specific payload fields (inputs, approval_id, perm_payload,
|
||||
// depends_on). A builder struct would obscure the call sites; the
|
||||
// shorter wrappers already cover all common cases.
|
||||
#[allow(
|
||||
clippy::too_many_arguments,
|
||||
reason = "args mirror the queue-entry fields the row is built from; the \
|
||||
thinner enqueue helpers wrap this for the common cases"
|
||||
)]
|
||||
pub fn enqueue_full(
|
||||
&self,
|
||||
kind: QueueKind,
|
||||
agent: String,
|
||||
source: QueueSource,
|
||||
reason: String,
|
||||
parent_id: Option<u64>,
|
||||
inputs: Vec<String>,
|
||||
approval_id: Option<i64>,
|
||||
perm_payload: Option<PermPayload>,
|
||||
depends_on: Vec<u64>,
|
||||
) -> u64 {
|
||||
pub fn enqueue_full(&self, spec: FullEnqueue) -> u64 {
|
||||
let FullEnqueue {
|
||||
kind,
|
||||
agent,
|
||||
source,
|
||||
reason,
|
||||
parent_id,
|
||||
inputs,
|
||||
approval_id,
|
||||
perm_payload,
|
||||
depends_on,
|
||||
} = spec;
|
||||
let mut inner = self.inner.lock().expect("rebuild_queue mutex poisoned");
|
||||
// Dedup against a pending entry with the same (kind, agent) —
|
||||
// and, for MetaUpdate, the same `inputs` list (see method
|
||||
|
|
@ -1213,17 +1220,17 @@ mod tests {
|
|||
#[test]
|
||||
fn approval_entries_keep_approval_id() {
|
||||
let q = RebuildQueue::new();
|
||||
let id = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #42 apply commit".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(42),
|
||||
None,
|
||||
Vec::new(),
|
||||
);
|
||||
let id = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "agent-a".to_owned(),
|
||||
source: QueueSource::Approval,
|
||||
reason: "approval #42 apply commit".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: Some(42),
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
});
|
||||
let snap = q.snapshot();
|
||||
let entry = snap.iter().find(|e| e.id == id).expect("entry present");
|
||||
assert_eq!(entry.approval_id, Some(42));
|
||||
|
|
@ -1237,43 +1244,43 @@ mod tests {
|
|||
// approve click is a separate piece of work even when the
|
||||
// (kind, agent) pair matches.
|
||||
let q = RebuildQueue::new();
|
||||
let a = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #1".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(1),
|
||||
None,
|
||||
Vec::new(),
|
||||
);
|
||||
let b = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #2".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(2),
|
||||
None,
|
||||
Vec::new(),
|
||||
);
|
||||
let a = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "agent-a".to_owned(),
|
||||
source: QueueSource::Approval,
|
||||
reason: "approval #1".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: Some(1),
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
});
|
||||
let b = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "agent-a".to_owned(),
|
||||
source: QueueSource::Approval,
|
||||
reason: "approval #2".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: Some(2),
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
});
|
||||
assert_ne!(a, b);
|
||||
assert_eq!(q.snapshot().len(), 2);
|
||||
// Same approval_id submitted twice DOES dedup (rapid double-
|
||||
// click on the dashboard's approve button is a single op).
|
||||
let c = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #1 (duplicate)".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(1),
|
||||
None,
|
||||
Vec::new(),
|
||||
);
|
||||
let c = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "agent-a".to_owned(),
|
||||
source: QueueSource::Approval,
|
||||
reason: "approval #1 (duplicate)".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: Some(1),
|
||||
perm_payload: None,
|
||||
depends_on: Vec::new(),
|
||||
});
|
||||
assert_eq!(a, c);
|
||||
assert_eq!(q.snapshot().len(), 2);
|
||||
}
|
||||
|
|
@ -1459,17 +1466,17 @@ mod tests {
|
|||
"first".to_owned(),
|
||||
None,
|
||||
);
|
||||
let b = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-b".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"second (blocked on a)".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![a],
|
||||
);
|
||||
let b = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "agent-b".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "second (blocked on a)".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![a],
|
||||
});
|
||||
// B depends on A — take_next should give A first.
|
||||
let first = q.take_next().expect("a is ready");
|
||||
assert_eq!(first.id, a);
|
||||
|
|
@ -1529,17 +1536,17 @@ mod tests {
|
|||
"dep must be evicted from history"
|
||||
);
|
||||
// An entry that depends on the (evicted) dep must be immediately runnable.
|
||||
let downstream = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"downstream".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"downstream (dep evicted = resolved)".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![dep],
|
||||
);
|
||||
let downstream = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "downstream".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "downstream (dep evicted = resolved)".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![dep],
|
||||
});
|
||||
let got = q.take_next().expect("downstream runnable when dep evicted");
|
||||
assert_eq!(got.id, downstream);
|
||||
}
|
||||
|
|
@ -1563,42 +1570,42 @@ mod tests {
|
|||
"d2".to_owned(),
|
||||
None,
|
||||
);
|
||||
let a = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"target".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"r".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![dep1],
|
||||
);
|
||||
let a = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "target".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "r".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![dep1],
|
||||
});
|
||||
// Same kind+agent but different depends_on — must NOT dedup.
|
||||
let b = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"target".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"r".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![dep2],
|
||||
);
|
||||
let b = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "target".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "r".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![dep2],
|
||||
});
|
||||
assert_ne!(a, b, "different depends_on must produce distinct entries");
|
||||
// Same depends_on as a — must dedup.
|
||||
let c = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"target".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"r again".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![dep1],
|
||||
);
|
||||
let c = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "target".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "r again".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![dep1],
|
||||
});
|
||||
assert_eq!(a, c, "identical depends_on must dedup");
|
||||
}
|
||||
|
||||
|
|
@ -1615,17 +1622,17 @@ mod tests {
|
|||
"a".to_owned(),
|
||||
None,
|
||||
);
|
||||
let b = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"b".to_owned(),
|
||||
QueueSource::Manual,
|
||||
"b (blocked on a)".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
None,
|
||||
None,
|
||||
vec![a],
|
||||
);
|
||||
let b = q.enqueue_full(FullEnqueue {
|
||||
kind: QueueKind::Rebuild,
|
||||
agent: "b".to_owned(),
|
||||
source: QueueSource::Manual,
|
||||
reason: "b (blocked on a)".to_owned(),
|
||||
parent_id: None,
|
||||
inputs: Vec::new(),
|
||||
approval_id: None,
|
||||
perm_payload: None,
|
||||
depends_on: vec![a],
|
||||
});
|
||||
q.take_next(); // pop a, mark Running
|
||||
q.finish(a, QueueState::Failed, Some("nix build exploded".to_owned()));
|
||||
let got = q.take_next().expect("b runnable after a failed");
|
||||
|
|
|
|||
Loading…
Reference in a new issue