Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 47 additions & 36 deletions engine/core/src/adapters/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,9 @@ fn collect_params(argv: &[String], schema: &Value) -> Result<Value, String> {
let mut values: Map<String, Value> = Map::new();
let mut secret_files: BTreeMap<String, String> = BTreeMap::new();
let mut secret_sources: BTreeMap<String, String> = BTreeMap::new();
// stdin is read only after the required arguments are checked, so a usage
// error never consumes or waits on standard input.
let mut stdin_secrets: Vec<String> = Vec::new();
let mut index = 0usize;
while index < argv.len() {
let token = &argv[index];
Expand Down Expand Up @@ -532,8 +535,7 @@ fn collect_params(argv: &[String], schema: &Value) -> Result<Value, String> {
return Err(format!("--{name} takes no value"));
}
index += 1;
// A repeated secret flag is the last one, matching argparse.
values.insert(secret.to_string(), Value::String(read_secret_stdin()?));
stdin_secrets.push(secret.to_string());
continue;
}
let value = take_value(argv, &mut index, inline, &name)?;
Expand Down Expand Up @@ -578,8 +580,10 @@ fn collect_params(argv: &[String], schema: &Value) -> Result<Value, String> {
continue;
}
let raw = take_value(argv, &mut index, inline, &name)?;
let parsed =
coerce(&raw, property).map_err(|message| format!("argument --{name}: {message}"))?;
// argparse rejects a value it cannot convert as an argument error:
// the usage synopsis, then `vogt <cmd>: error:`. Not the full help page.
let parsed = coerce(&raw, property)
.map_err(|message| format!("arg: argument --{name}: {message}"))?;
if is_list(property) {
let entry = values
.entry(field)
Expand All @@ -589,36 +593,41 @@ fn collect_params(argv: &[String], schema: &Value) -> Result<Value, String> {
values.insert(field, parsed);
}
}
for (secret, path) in &secret_files {
values.insert(secret.clone(), Value::String(read_secret_file(path)?));
// Required arguments that are not secrets are a usage error, and that check
// runs before any secret is read, as argparse does before pydantic does.
let missing: Vec<String> = required
.iter()
.filter(|name| {
!is_secret(name) && !is_nullable(properties.get(*name)) && !values.contains_key(*name)
})
.map(|name| format!("--{}", name.replace('_', "-")))
.collect();
if !missing.is_empty() {
return Err(format!("usage: {}", missing.join(", ")));
}

for name in &required {
if is_secret(name) {
if !values.contains_key(name) {
let dashed = name.replace('_', "-");
// Off a terminal there is no prompt, so a missing secret is a
// domain error (exit 1), not a usage error.
if !io::stdin().is_terminal() {
return Err(format!(
"domain: --{dashed}-file or --{dashed}-stdin is required when not on a terminal"
));
}
return Err(format!("--{dashed}-file or --{dashed}-stdin is required"));
for secret in &stdin_secrets {
// A repeated secret flag is the last one, matching argparse.
let text = read_secret_stdin().map_err(|message| format!("domain: {message}"))?;
values.insert(secret.clone(), Value::String(text));
}
for (secret, path) in &secret_files {
// An unreadable file is a domain error (exit 1, on stderr), as the
// secret-not-on-a-terminal case is. Python's uncaught OSError exits 1 too.
let text = read_secret_file(path).map_err(|message| format!("domain: {message}"))?;
values.insert(secret.clone(), Value::String(text));
}

for name in required.iter().filter(|name| is_secret(name)) {
if !values.contains_key(name) {
let dashed = name.replace('_', "-");
// Off a terminal there is no prompt, so a missing secret is a
// domain error (exit 1), not a usage error.
if !io::stdin().is_terminal() {
return Err(format!(
"domain: --{dashed}-file or --{dashed}-stdin is required when not on a terminal"
));
}
continue;
}
let missing: Vec<String> = required
.iter()
.filter(|name| {
!is_secret(name)
&& !is_nullable(properties.get(*name))
&& !values.contains_key(*name)
})
.map(|name| format!("--{}", name.replace('_', "-")))
.collect();
if !missing.is_empty() {
return Err(format!("usage: {}", missing.join(", ")));
return Err(format!("--{dashed}-file or --{dashed}-stdin is required"));
}
}
Ok(Value::Object(values))
Expand Down Expand Up @@ -1959,8 +1968,10 @@ mod tests {
&mut no_dispatch,
);
assert_eq!(result.exit_code, EXIT_USAGE, "{}", result.stdout);
// argparse writes a usage error to stderr, and stdout stays empty.
assert!(result.stdout.is_empty(), "{}", result.stdout);
assert!(
result.stdout.contains("invalid choice"),
result.stderr.contains("invalid choice"),
"{}",
result.stderr
);
Expand Down Expand Up @@ -2504,9 +2515,9 @@ mod tests {
);
assert_eq!(bad.exit_code, EXIT_USAGE, "{}", bad.stdout);
assert!(
bad.stdout.contains("invalid int value: '-1.5'"),
bad.stderr.contains("invalid int value: '-1.5'"),
"{}",
bad.stdout
bad.stderr
);
}

Expand Down Expand Up @@ -2599,6 +2610,6 @@ mod tests {
&mut no_dispatch,
);
assert_eq!(result.exit_code, EXIT_USAGE);
assert!(result.stdout.contains("invalid choice"));
assert!(result.stderr.contains("invalid choice"));
}
}
Loading
Loading