Skip to content

Commit cf78c6a

Browse files
Improved comments per review feedback from Martin.
1 parent 258084c commit cf78c6a

1 file changed

Lines changed: 16 additions & 4 deletions

File tree

rs/registry/admin/bin/main.rs

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1455,6 +1455,9 @@ struct ProposeToChangeNnsCanisterCmd {
14551455

14561456
#[clap(long)]
14571457
/// Whether to skip the canister's pre_upgrade hook. Only valid when mode is upgrade.
1458+
// This is not Option<bool>, because `--skip-pre-upgrade true` looks stupid.
1459+
// At the same time, it is fine that we do not support both `None` and
1460+
// `Some(false)`, because those end up having the same behavior.
14581461
skip_pre_upgrade: bool,
14591462

14601463
#[clap(long)]
@@ -1555,22 +1558,31 @@ impl ProposalAction for ProposeToChangeNnsCanisterCmd {
15551558
}
15561559
}
15571560

1558-
/// Constructs a CanisterUpgradeOptions from its constituents.
1559-
///
1560-
/// (Presumably, the pieces were passed via CLI flags.)
1561+
/// Constructs a CanisterUpgradeOptions from its flag values.
15611562
///
15621563
/// Returns None if skip_pre_upgrade is false and wasm_memory_persistence is
15631564
/// None, i.e. there is nothing to say.
15641565
fn assemble_canister_upgrade_options(
1566+
// These parameter types match the corresponding flags.
15651567
skip_pre_upgrade: bool,
15661568
wasm_memory_persistence: Option<WasmMemoryPersistence>,
15671569
) -> Option<GovernanceCanisterUpgradeOptions> {
15681570
let has_option = skip_pre_upgrade || wasm_memory_persistence.is_some();
15691571
if !has_option {
1572+
// It is not ok to always return an "empty" CanisterUpgradeOptions,
1573+
// because that is only allowed when `--mode upgrade`.
15701574
return None;
15711575
}
15721576

1573-
let skip_pre_upgrade = if skip_pre_upgrade { Some(true) } else { None };
1577+
let skip_pre_upgrade = if skip_pre_upgrade {
1578+
Some(true)
1579+
} else {
1580+
// Alternatively, we could use Some(false) here, but when false is
1581+
// passed to this function, that means that there was no
1582+
// `--skip-pre-upgrade` in the command. This better reflects that
1583+
// omission.
1584+
None
1585+
};
15741586

15751587
let wasm_memory_persistence = wasm_memory_persistence.map(|wasm_memory_persistence| {
15761588
WasmMemoryPersistenceProto::from(&wasm_memory_persistence) as i32

0 commit comments

Comments
 (0)