Re: [pbs-devel] [PATCH vma-to-pbs 04/10] Add the ability to provide credentials via files

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 25/03/2024 um 13:33 schrieb Max Carrara:
> On Wed Mar 20, 2024 at 3:15 PM CET, Filip Schauer wrote:
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>>  src/main.rs | 31 +++++++++++++++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/main.rs b/src/main.rs
>> index 8d95b11..b58387b 100644
>> --- a/src/main.rs
>> +++ b/src/main.rs
>> @@ -280,6 +280,18 @@ fn main() -> Result<()> {
>>                  .help("Encrypt the Backup")
>>                  .action(ArgAction::SetTrue),
>>          )
>> +        .arg(
>> +            Arg::new("password_file")
>> +                .long("password_file")
>> +                .value_name("PASSWORD_FILE")
>> +                .help("Password file"),
>> +        )
>> +        .arg(
>> +            Arg::new("key_password_file")
>> +                .long("key_password_file")
>> +                .value_name("KEY_PASSWORD_FILE")
>> +                .help("Key password file"),
>> +        )
>>          .arg(Arg::new("vma_file"))
>>          .get_matches();
> 
> One thing I had noticed when using the CLI: Would it maaaybe be nicer to
> use e.g. --password-file instead of --password_file? AFAIR @Wolfgang
> mentioned something regarding this off list.

yes, we use kebab-case for all new (CLI & API) options if anyhow possible, please!


btw. is clap really required here? Our usage in the past has shown that it's
rather a huge maintenance burden, and so I'd like to prefer using pico-args.
You can use the clap usage output as source for a manual implemented usage
though (extending that then in the future isn't that much work).

For a conversion example and some more background see:
https://git.proxmox.com/?p=pve-xtermjs.git;a=commitdiff;h=24d707d0506b120a085b06b5f2b6000696879a1e;hp=749ebb0907293a9f1cf0f5074e0a240f39f94f6f

Note though that I would not block clap usage for now, but there's a
significant possibility that I'll rip it out on the next clap major bump
in anyway.

> 
>>  
>> @@ -296,10 +308,25 @@ fn main() -> Result<()> {
>>      let encrypt = matches.get_flag("encrypt");
>>  
>>      let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string();
>> +    let password_file = matches.get_one::<String>("password_file");
>> +
>> +    let pbs_password = match password_file {
>> +        Some(password_file) => {
>> +            std::fs::read_to_string(password_file).expect("Could not read password file")
> 
> You changed this later and got rid of the undying, but that fix could've
> perhaps been in here already.

yeah, not that big of an issue on such initial additions, but if we somehow
can avoid intermediate breakage or the like it's always better.