fix(workflows): pull hard-errors on a definition-less workflow, not silent skip
Re-review flagged that the previous warn-and-skip (aeebdd2) traded a loud
failure for a SILENT prune hazard: against a server too old to return a
workflow's `definition`, pull dropped it with only a stderr warning, exited 0,
and wrote a workflow-less manifest — which a later `apply --prune` uses to
DELETE the server-side workflow (and `pull --force` would clobber a
hand-authored `[[workflows]]` block first).
A live workflow always has ≥1 step (zero-step is rejected at apply), so an
empty `definition` can only mean an out-of-date server. Pull now bails BEFORE
writing anything — consistent with the existing clobber / unsafe-name fail-fast
gates — so nothing partial hits disk and the operator gets an actionable error.
wire_workflow_to_manifest reverts to infallible (the guard upholds its
precondition).
Verified: fmt + clippy -D warnings clean; 5 workflow CLI journeys pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -47,6 +47,20 @@ pub async fn run(app_ident: &str, dir: &Path, force: bool, mode: OutputMode) ->
|
|||||||
let scripts = client.scripts_list_by_app(app_ident).await?;
|
let scripts = client.scripts_list_by_app(app_ident).await?;
|
||||||
let triggers = client.triggers_list(app_ident).await?.triggers;
|
let triggers = client.triggers_list(app_ident).await?.triggers;
|
||||||
let workflows = client.workflows_list(app_ident).await?;
|
let workflows = client.workflows_list(app_ident).await?;
|
||||||
|
// A live workflow always has ≥1 step (a zero-step definition is rejected at
|
||||||
|
// apply), so an empty `definition` here can only mean the server predates
|
||||||
|
// the definition-in-list field. Fail fast BEFORE writing anything — silently
|
||||||
|
// dropping the workflow would leave a manifest that a later `apply --prune`
|
||||||
|
// uses to DELETE the server-side workflow (and `pull --force` would clobber
|
||||||
|
// a hand-authored block). Mirror the clobber/unsafe-name bails above.
|
||||||
|
if let Some(w) = workflows.iter().find(|w| w.definition.steps.is_empty()) {
|
||||||
|
anyhow::bail!(
|
||||||
|
"workflow `{}` came back without its definition — the server is too old to \
|
||||||
|
round-trip workflows via `pull`. Refusing to write a manifest that omits it \
|
||||||
|
(a later `apply --prune` would delete the workflow). Upgrade the server and retry.",
|
||||||
|
w.name
|
||||||
|
);
|
||||||
|
}
|
||||||
let secrets = client
|
let secrets = client
|
||||||
.secrets_list(VarOwnerArg::App(app_ident), None)
|
.secrets_list(VarOwnerArg::App(app_ident), None)
|
||||||
.await?
|
.await?
|
||||||
@@ -253,10 +267,7 @@ pub async fn run(app_ident: &str, dir: &Path, force: bool, mode: OutputMode) ->
|
|||||||
},
|
},
|
||||||
vars: manifest_vars,
|
vars: manifest_vars,
|
||||||
suppress: crate::manifest::ManifestSuppress::default(),
|
suppress: crate::manifest::ManifestSuppress::default(),
|
||||||
workflows: workflows
|
workflows: workflows.iter().map(wire_workflow_to_manifest).collect(),
|
||||||
.iter()
|
|
||||||
.filter_map(wire_workflow_to_manifest)
|
|
||||||
.collect(),
|
|
||||||
};
|
};
|
||||||
|
|
||||||
std::fs::write(&manifest_path, manifest.to_toml()?)
|
std::fs::write(&manifest_path, manifest.to_toml()?)
|
||||||
@@ -284,20 +295,11 @@ fn trigger_count(t: &ManifestTriggers) -> usize {
|
|||||||
/// drop the app's workflows, and a subsequent `pic apply --prune` would delete
|
/// drop the app's workflows, and a subsequent `pic apply --prune` would delete
|
||||||
/// them from the server.
|
/// them from the server.
|
||||||
///
|
///
|
||||||
/// Returns `None` (with a warning) when the definition came back empty. A
|
/// The caller guarantees `definition.steps` is non-empty (an empty definition
|
||||||
/// zero-step workflow is rejected at apply, so an empty `definition` can only
|
/// means an out-of-date server and is rejected up front in `run`, before any
|
||||||
/// mean the server predates the definition-in-list field (version skew) —
|
/// file is written).
|
||||||
/// emitting a stepless block would just fail the next apply, so skip it.
|
fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> ManifestWorkflow {
|
||||||
fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> Option<ManifestWorkflow> {
|
ManifestWorkflow {
|
||||||
if w.definition.steps.is_empty() {
|
|
||||||
eprintln!(
|
|
||||||
"warning: skipping workflow `{}` — the server did not return its \
|
|
||||||
definition (upgrade the server to round-trip workflows via pull)",
|
|
||||||
w.name
|
|
||||||
);
|
|
||||||
return None;
|
|
||||||
}
|
|
||||||
Some(ManifestWorkflow {
|
|
||||||
name: w.name.clone(),
|
name: w.name.clone(),
|
||||||
enabled: w.enabled,
|
enabled: w.enabled,
|
||||||
steps: w
|
steps: w
|
||||||
@@ -306,7 +308,7 @@ fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> Option<ManifestW
|
|||||||
.iter()
|
.iter()
|
||||||
.map(wire_step_to_manifest)
|
.map(wire_step_to_manifest)
|
||||||
.collect(),
|
.collect(),
|
||||||
})
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn wire_step_to_manifest(s: &picloud_shared::WorkflowStepDef) -> ManifestWorkflowStep {
|
fn wire_step_to_manifest(s: &picloud_shared::WorkflowStepDef) -> ManifestWorkflowStep {
|
||||||
|
|||||||
Reference in New Issue
Block a user