Fix for the "config generate" action generating invalid configuration when upgrading deprecated command hooks (#1091).

NEWS

@@ -1,6 +1,8 @@
 2.0.6.dev0
  * #1068: Fix a warning from LVM about leaked file descriptors.
  * #1086: Fix for the "spot" check breaking when the "--progress" flag is used.
+ * #1091: Fix for the "config generate" action generating invalid configuration when upgrading
+   deprecated command hooks.
  * Add support for Borg 2's "s3:" and "b2:" repository URLs, so you can backup to S3 or B2 cloud
    storage services even without using Rclone.
 

borgmatic/config/generate.py

@@ -51,7 +51,7 @@ def schema_to_sample_configuration(schema, source_config=None, level=0, parent_i
         add_comments_to_configuration_sequence(config, schema, indent=(level * INDENT))
     elif borgmatic.config.schema.compare_types(schema_type, {'object'}):
         if source_config and isinstance(source_config, list) and isinstance(source_config[0], dict):
-            source_config = dict(collections.ChainMap(*source_config))
+            source_config = source_config[0]
 
         config = (
             ruamel.yaml.comments.CommentedMap(
@@ -71,7 +71,7 @@ def schema_to_sample_configuration(schema, source_config=None, level=0, parent_i
         )
         indent = (level * INDENT) + (SEQUENCE_INDENT if parent_is_sequence else 0)
         add_comments_to_configuration_object(
-            config, schema, source_config, indent=indent, skip_first=parent_is_sequence
+            config, schema, source_config, indent=indent, skip_first_field=parent_is_sequence
         )
     elif borgmatic.config.schema.compare_types(schema_type, SCALAR_SCHEMA_TYPES, match=all):
         return example
@@ -108,17 +108,30 @@ def comment_out_optional_configuration(rendered_config):
     '''
     lines = []
     optional = False
+    indent_characters = None
+    indent_characters_at_sentinel = None
 
     for line in rendered_config.split('\n'):
+        indent_characters = len(line) - len(line.lstrip())
+
         # Upon encountering an optional configuration option, comment out lines until the next blank
         # line.
         if line.strip().startswith(f'# {COMMENTED_OUT_SENTINEL}'):
             optional = True
+            indent_characters_at_sentinel = indent_characters
             continue
 
         # Hit a blank line, so reset commenting.
         if not line.strip():
             optional = False
+            indent_characters_at_sentinel = None
+        # Dedented, so reset commenting.
+        elif (
+            indent_characters_at_sentinel is not None
+            and indent_characters < indent_characters_at_sentinel
+        ):
+            optional = False
+            indent_characters_at_sentinel = None
 
         lines.append(comment_out_line(line) if optional else line)
 
@@ -198,25 +211,28 @@ COMMENTED_OUT_SENTINEL = 'COMMENT_OUT'
 
 
 def add_comments_to_configuration_object(
-    config, schema, source_config=None, indent=0, skip_first=False
+    config, schema, source_config=None, indent=0, skip_first_field=False
 ):
     '''
     Using descriptions from a schema as a source, add those descriptions as comments to the given
     configuration dict, putting them before each field. Indent the comment the given number of
     characters.
 
+    If skip_first_field is True, omit the comment for the initial field. This is useful for
+    sequences, where the comment for the first field goes before the sequence itself.
+
     And a sentinel for commenting out options that are neither in DEFAULT_KEYS nor the the given
     source configuration dict. The idea is that any options used in the source configuration should
     stay active in the generated configuration.
     '''
     for index, field_name in enumerate(config.keys()):
-        if skip_first and index == 0:
+        if skip_first_field and index == 0:
             continue
 
         field_schema = borgmatic.config.schema.get_properties(schema).get(field_name, {})
         description = field_schema.get('description', '').strip()
 
-        # If this isn't a default key, add an indicator to the comment flagging it to be commented
+        # If this isn't a default key, add an indicator to the comment, flagging it to be commented
         # out from the sample configuration. This sentinel is consumed by downstream processing that
         # does the actual commenting out.
         if field_name not in DEFAULT_KEYS and (
@@ -299,6 +315,12 @@ def generate_sample_configuration(
         source_config = load.load_configuration(source_filename)
         normalize.normalize(source_filename, source_config)
 
+        # The borgmatic.config.normalize.normalize() function tacks on an empty "bootstrap" if
+        # needed, so the hook gets used by default. But we don't want it to end up in the generated
+        # config unless the user has set it explicitly, as an empty "bootstrap:" won't validate.
+        if source_config and source_config.get('bootstrap') == {}:
+            del source_config['bootstrap']
+
     destination_config = merge_source_configuration_into_destination(
         schema_to_sample_configuration(schema, source_config), source_config
     )

tests/integration/config/test_generate.py

@@ -126,6 +126,45 @@ def test_schema_to_sample_configuration_comments_out_non_source_config_options_i
     assert 'field3' not in config[0].ca.items
 
 
+def test_schema_to_sample_configuration_comments_out_non_source_config_options_in_sequence_of_maps_with_different_subschemas():
+    schema = {
+        'type': 'array',
+        'items': {
+            'type': 'object',
+            'oneOf': [
+                {
+                    'properties': dict(
+                        [
+                            ('field1', {'type': 'string', 'example': 'Example 1'}),
+                            ('field2', {'type': 'string', 'example': 'Example 2'}),
+                        ]
+                    )
+                },
+                {
+                    'properties': dict(
+                        [
+                            ('field2', {'type': 'string', 'example': 'Example 2'}),
+                            ('field3', {'type': 'string', 'example': 'Example 3'}),
+                        ]
+                    )
+                },
+            ],
+        },
+    }
+    source_config = [{'field1': 'value'}, {'field3': 'value'}]
+
+    config = module.schema_to_sample_configuration(schema, source_config)
+
+    assert config == [
+        dict([('field1', 'Example 1'), ('field2', 'Example 2'), ('field3', 'Example 3')])
+    ]
+
+    # The first field in a sequence does not get commented.
+    assert 'field1' not in config[0].ca.items
+    assert 'COMMENT_OUT' in config[0].ca.items['field2'][1][-1]._value
+    assert 'COMMENT_OUT' in config[0].ca.items['field3'][1][-1]._value
+
+
 def test_comment_out_line_skips_blank_line():
     line = '    \n'
 
@@ -169,8 +208,11 @@ foo:
         - quux
 
 repositories:
-    - one
-    - two
+    - path: foo
+      # COMMENT_OUT
+      label: bar
+    - path: baz
+      label: quux
 
 # This comment should be kept.
 # COMMENT_OUT
@@ -185,8 +227,10 @@ other: thing
 #         - quux
 
 repositories:
-    - one
-    - two
+    - path: foo
+#       label: bar
+    - path: baz
+      label: quux
 
 # This comment should be kept.
 # other: thing
@@ -297,14 +341,28 @@ def test_add_comments_to_configuration_comments_out_non_source_config_options():
     assert 'baz' not in config.ca.items
 
 
-def test_add_comments_to_configuration_object_with_skip_first_does_not_comment_out_first_option():
+def test_add_comments_to_configuration_object_with_skip_first_field_does_not_comment_out_first_option():
     config = module.ruamel.yaml.comments.CommentedMap([('foo', 33), ('bar', 44), ('baz', 55)])
     schema = {
         'type': 'object',
         'properties': {'foo': {'description': 'Foo'}, 'bar': {'description': 'Bar'}},
     }
 
-    module.add_comments_to_configuration_object(config, schema, skip_first=True)
+    module.add_comments_to_configuration_object(config, schema, skip_first_field=True)
+
+    assert 'foo' not in config.ca.items
+    assert 'COMMENT_OUT' in config.ca.items['bar'][1][-1]._value
+    assert 'baz' not in config.ca.items
+
+
+def test_add_comments_to_configuration_object_with_skip_first_field_does_not_comment_out_first_option():
+    config = module.ruamel.yaml.comments.CommentedMap([('foo', 33), ('bar', 44), ('baz', 55)])
+    schema = {
+        'type': 'object',
+        'properties': {'foo': {'description': 'Foo'}, 'bar': {'description': 'Bar'}},
+    }
+
+    module.add_comments_to_configuration_object(config, schema, skip_first_field=True)
 
     assert 'foo' not in config.ca.items
     assert 'COMMENT_OUT' in config.ca.items['bar'][1][-1]._value
@@ -326,15 +384,39 @@ def test_generate_sample_configuration_does_not_raise():
     module.generate_sample_configuration(False, None, 'dest.yaml', 'schema.yaml')
 
 
-def test_generate_sample_configuration_with_source_filename_does_not_raise():
+def test_generate_sample_configuration_with_source_filename_omits_empty_bootstrap_field():
     builtins = flexmock(sys.modules['builtins'])
     builtins.should_receive('open').with_args('schema.yaml').and_return('')
     flexmock(module.ruamel.yaml).should_receive('YAML').and_return(
         flexmock(load=lambda filename: {})
     )
-    flexmock(module.load).should_receive('load_configuration')
+    flexmock(module.load).should_receive('load_configuration').and_return(
+        {'bootstrap': {}, 'foo': 'bar'}
+    )
     flexmock(module.normalize).should_receive('normalize')
-    flexmock(module).should_receive('schema_to_sample_configuration')
+    flexmock(module).should_receive('schema_to_sample_configuration').with_args(
+        object, {'foo': 'bar'}
+    ).once()
+    flexmock(module).should_receive('merge_source_configuration_into_destination')
+    flexmock(module).should_receive('render_configuration')
+    flexmock(module).should_receive('comment_out_optional_configuration')
+    flexmock(module).should_receive('write_configuration')
+
+    module.generate_sample_configuration(False, 'source.yaml', 'dest.yaml', 'schema.yaml')
+
+
+def test_generate_sample_configuration_with_source_filename_keeps_non_empty_bootstrap_field():
+    builtins = flexmock(sys.modules['builtins'])
+    builtins.should_receive('open').with_args('schema.yaml').and_return('')
+    flexmock(module.ruamel.yaml).should_receive('YAML').and_return(
+        flexmock(load=lambda filename: {})
+    )
+    source_config = {'bootstrap': {'stuff': 'here'}, 'foo': 'bar'}
+    flexmock(module.load).should_receive('load_configuration').and_return(source_config)
+    flexmock(module.normalize).should_receive('normalize')
+    flexmock(module).should_receive('schema_to_sample_configuration').with_args(
+        object, source_config
+    ).once()
     flexmock(module).should_receive('merge_source_configuration_into_destination')
     flexmock(module).should_receive('render_configuration')
     flexmock(module).should_receive('comment_out_optional_configuration')