Partially align notes printing and editing behavior with vault(1)
Marco Ricci

Marco Ricci commited on 2025-02-04 13:16:29
Zeige 2 geänderte Dateien mit 57 Einfügungen und 61 Löschungen.


Notes (if any) are printed after the derived passphrase is emitted.  The
`--notes` flag is an option, not a command: editing notes requires both
`--notes` and `--config`.  Any such edits happen after querying for
master passphrases or SSH keys to use.

Editing notes, even if a no-op, will at least register the service name
as a known service.

The `--unset` option now additionally accepts the "notes" argument.

(Caveat: vault(1) actually leaves the terminal in a bad state during
editing if called after querying a master passphrase or selecting an SSH
key.)
... ...
@@ -577,6 +577,7 @@ def derivepassphrase_export_vault(
577 577
         'space',
578 578
         'dash',
579 579
         'symbol',
580
+        'notes',
580 581
     ]),
581 582
     help=_msg.TranslatedString(
582 583
         _msg.Label.DERIVEPASSPHRASE_VAULT_UNSET_HELP_TEXT
... ...
@@ -913,7 +914,10 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
913 914
         cli_machinery.StorageManagementOption,
914 915
     ):
915 916
         for opt in options_in_group[group]:
916
-            if opt != params_by_str['--config']:
917
+            if opt not in {
918
+                params_by_str['--config'],
919
+                params_by_str['--notes'],
920
+            }:
917 921
                 for other_opt in options_in_group[
918 922
                     cli_machinery.PassphraseGenerationOption
919 923
                 ]:
... ...
@@ -927,6 +931,10 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
927 931
             for other_opt in options_in_group[
928 932
                 cli_machinery.ConfigurationOption
929 933
             ]:
934
+                if {opt, other_opt} != {
935
+                    params_by_str['--config'],
936
+                    params_by_str['--notes'],
937
+                }:
930 938
                     check_incompatible_options(opt, other_opt)
931 939
             for other_opt in options_in_group[
932 940
                 cli_machinery.StorageManagementOption
... ...
@@ -979,45 +987,7 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
979 987
             extra={'color': ctx.color},
980 988
         )
981 989
 
982
-    if edit_notes:
983
-        assert service is not None
984
-        configuration = get_config()
985
-        notes_instructions = _msg.TranslatedString(
986
-            _msg.Label.DERIVEPASSPHRASE_VAULT_NOTES_INSTRUCTION_TEXT
987
-        )
988
-        notes_marker = _msg.TranslatedString(
989
-            _msg.Label.DERIVEPASSPHRASE_VAULT_NOTES_MARKER
990
-        )
991
-        old_notes_value = (
992
-            configuration['services']
993
-            .get(service, cast('_types.VaultConfigServicesSettings', {}))
994
-            .get('notes', '')
995
-        )
996
-        text = '\n'.join([
997
-            str(notes_instructions),
998
-            str(notes_marker),
999
-            old_notes_value,
1000
-        ])
1001
-        notes_value = click.edit(text=text)
1002
-        if notes_value is not None:
1003
-            notes_lines = collections.deque(notes_value.splitlines(True))  # noqa: FBT003
1004
-            while notes_lines:
1005
-                line = notes_lines.popleft()
1006
-                if line.startswith(str(notes_marker)):
1007
-                    notes_value = ''.join(notes_lines)
1008
-                    break
1009
-            else:
1010
-                if not notes_value.strip():
1011
-                    err(
1012
-                        _msg.TranslatedString(
1013
-                            _msg.ErrMsgTemplate.USER_ABORTED_EDIT
1014
-                        )
1015
-                    )
1016
-            configuration['services'].setdefault(service, {})['notes'] = (
1017
-                notes_value.strip('\n')
1018
-            )
1019
-            put_config(configuration)
1020
-    elif delete_service_settings:
990
+    if delete_service_settings:  # noqa: PLR1702
1021 991
         assert service is not None
1022 992
         configuration = get_config()
1023 993
         if service in configuration['services']:
... ...
@@ -1370,7 +1340,7 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1370 1340
                             _msg.WarnMsgTemplate.GLOBAL_PASSPHRASE_INEFFECTIVE
1371 1341
                         )
1372 1342
                     logger.warning(w_msg, extra={'color': ctx.color})
1373
-            if not view.maps[0] and not unset_settings:
1343
+            if not view.maps[0] and not unset_settings and not edit_notes:
1374 1344
                 err_msg = _msg.TranslatedString(
1375 1345
                     _msg.ErrMsgTemplate.CANNOT_UPDATE_SETTINGS_NO_SETTINGS,
1376 1346
                     settings_type=_msg.TranslatedString(
... ...
@@ -1409,6 +1379,38 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1409 1379
             assert _types.is_vault_config(configuration), (
1410 1380
                 f'Invalid vault configuration: {configuration!r}'
1411 1381
             )
1382
+            if edit_notes:
1383
+                assert service is not None
1384
+                notes_instructions = _msg.TranslatedString(
1385
+                    _msg.Label.DERIVEPASSPHRASE_VAULT_NOTES_INSTRUCTION_TEXT
1386
+                )
1387
+                notes_marker = _msg.TranslatedString(
1388
+                    _msg.Label.DERIVEPASSPHRASE_VAULT_NOTES_MARKER
1389
+                )
1390
+                old_notes_value = subtree.get('notes', '')
1391
+                text = '\n'.join([
1392
+                    str(notes_instructions),
1393
+                    str(notes_marker),
1394
+                    old_notes_value,
1395
+                ])
1396
+                notes_value = click.edit(text=text)
1397
+                if notes_value is not None:
1398
+                    notes_lines = collections.deque(
1399
+                        notes_value.splitlines(True)  # noqa: FBT003
1400
+                    )
1401
+                    while notes_lines:
1402
+                        line = notes_lines.popleft()
1403
+                        if line.startswith(str(notes_marker)):
1404
+                            notes_value = ''.join(notes_lines)
1405
+                            break
1406
+                    else:
1407
+                        if not notes_value.strip():
1408
+                            err(
1409
+                                _msg.TranslatedString(
1410
+                                    _msg.ErrMsgTemplate.USER_ABORTED_EDIT
1411
+                                )
1412
+                            )
1413
+                    subtree['notes'] = notes_value.strip('\n')
1412 1414
             put_config(configuration)
1413 1415
         else:
1414 1416
             assert service is not None
... ...
@@ -1457,7 +1459,12 @@ def derivepassphrase_vault(  # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915
1457 1459
                 )
1458 1460
                 raise click.UsageError(str(err_msg))
1459 1461
             kwargs.pop('key', '')
1462
+            service_notes = (
1463
+                f'\n{settings["notes"]}\n\n' if 'notes' in settings else ''
1464
+            )
1460 1465
             result = vault.Vault(**kwargs).generate(service)
1466
+            if service_notes.strip():
1467
+                click.echo(service_notes, err=True, color=ctx.color)
1461 1468
             click.echo(result.decode('ASCII'), color=ctx.color)
1462 1469
 
1463 1470
 
... ...
@@ -96,7 +96,6 @@ CONFIGURATION_OPTIONS: list[tuple[str, ...]] = [
96 96
     ('--clear',),
97 97
 ]
98 98
 CONFIGURATION_COMMANDS: list[tuple[str, ...]] = [
99
-    ('--notes',),
100 99
     ('--delete',),
101 100
     ('--delete-globals',),
102 101
     ('--clear',),
... ...
@@ -136,15 +135,7 @@ INCOMPATIBLE: dict[tuple[str, ...], IncompatibleConfiguration] = {
136 135
         CONFIGURATION_COMMANDS + STORAGE_OPTIONS, True, DUMMY_PASSPHRASE
137 136
     ),
138 137
     ('--notes',): IncompatibleConfiguration(
139
-        [
140
-            ('--config',),
141
-            ('--delete',),
142
-            ('--delete-globals',),
143
-            ('--clear',),
144
-            *STORAGE_OPTIONS,
145
-        ],
146
-        True,
147
-        None,
138
+        CONFIGURATION_COMMANDS + STORAGE_OPTIONS, True, None
148 139
     ),
149 140
     ('--config', '-p'): IncompatibleConfiguration(
150 141
         [('--delete',), ('--delete-globals',), ('--clear',), *STORAGE_OPTIONS],
... ...
@@ -1872,11 +1863,6 @@ class TestCLI:
1872 1863
             map(is_harmless_config_import_warning, caplog.record_tuples)
1873 1864
         ), 'unexpected error output'
1874 1865
 
1875
-    @pytest.mark.xfail(  # pragma: no cover
1876
-        reason='not implemented yet',
1877
-        raises=AssertionError,
1878
-        strict=True,
1879
-    )
1880 1866
     def test_207_service_with_notes_actually_prints_notes(
1881 1867
         self,
1882 1868
     ) -> None:
... ...
@@ -2487,7 +2473,7 @@ contents go here
2487 2473
             monkeypatch.setattr(click, 'edit', lambda *a, **kw: edit_result)  # noqa: ARG005
2488 2474
             result_ = runner.invoke(
2489 2475
                 cli.derivepassphrase_vault,
2490
-                ['--notes', '--', 'sv'],
2476
+                ['--config', '--notes', '--', 'sv'],
2491 2477
                 catch_exceptions=False,
2492 2478
             )
2493 2479
             result = tests.ReadableResult.parse(result_)
... ...
@@ -2521,7 +2507,7 @@ contents go here
2521 2507
             monkeypatch.setattr(click, 'edit', lambda *a, **kw: None)  # noqa: ARG005
2522 2508
             result_ = runner.invoke(
2523 2509
                 cli.derivepassphrase_vault,
2524
-                ['--notes', '--', 'sv'],
2510
+                ['--config', '--notes', '--', 'sv'],
2525 2511
                 catch_exceptions=False,
2526 2512
             )
2527 2513
             result = tests.ReadableResult.parse(result_)
... ...
@@ -2530,7 +2516,10 @@ contents go here
2530 2516
                 encoding='UTF-8'
2531 2517
             ) as infile:
2532 2518
                 config = json.load(infile)
2533
-            assert config == {'global': {'phrase': 'abc'}, 'services': {}}
2519
+            assert config == {
2520
+                'global': {'phrase': 'abc'},
2521
+                'services': {'sv': {}},
2522
+            }
2534 2523
 
2535 2524
     # TODO(the-13th-letter): Keep this behavior or not, with or without
2536 2525
     # warning?
... ...
@@ -2558,7 +2547,7 @@ contents go here
2558 2547
             monkeypatch.setattr(click, 'edit', lambda *a, **kw: 'long\ntext')  # noqa: ARG005
2559 2548
             result_ = runner.invoke(
2560 2549
                 cli.derivepassphrase_vault,
2561
-                ['--notes', '--', 'sv'],
2550
+                ['--config', '--notes', '--', 'sv'],
2562 2551
                 catch_exceptions=False,
2563 2552
             )
2564 2553
             result = tests.ReadableResult.parse(result_)
... ...
@@ -2592,7 +2581,7 @@ contents go here
2592 2581
             monkeypatch.setattr(click, 'edit', lambda *a, **kw: '\n\n')  # noqa: ARG005
2593 2582
             result_ = runner.invoke(
2594 2583
                 cli.derivepassphrase_vault,
2595
-                ['--notes', '--', 'sv'],
2584
+                ['--config', '--notes', '--', 'sv'],
2596 2585
                 catch_exceptions=False,
2597 2586
             )
2598 2587
             result = tests.ReadableResult.parse(result_)
2599 2588