Improve mapping #123

snaselj · 2024-01-23T13:51:02Z

Closes: NaN

What's Changed

Added extras.journalentry => extras.note mapping.
Added ipam.FHRPGroup => dcim.interfaceRedundancyGroup mapping.
Added disable_related_reference to configure_model to allow disabling references, e.g., for ObjectChange.
Added dict to empty values to cover the default value of the _custom_field_type field.
Added AUTO_ADD_FIELDS = ("content_types", "status"), based on their existence in Nautobot.
Added content type UIDs with source and target wrappers to the fields summary.
Added importer name to the fields summary.
Added check and fail if referenced record doesn't exist in source data, cache, or Nautobot.
Added fields.relation field definition.
Added --save-mappings-file to the management command to save mappings for later comparison.
Allowed PascalCase content types in configure_model and get_or_create_wrapper.
Fixed extras.tag.content_types assignment.
Fixed failing save() logging when a model instance is not str()able.
Fixed error when missing _custom_field_data field in the Nautobot model.
Fixed locations related content_types assignment.
Fixed the use of callables in default values.
Refactored definitions and importers to be better reusable and explanatory.
Refactored handle_sibling to assign the same importer.
Refactored internal type sets to NautobotField properties: is_reference, is_integer, is_auto_increment, is_content_type, can_import.
Renamed NautobotFieldWrapper => NautobotField.
Replaced InternalFieldType.GENERIC_FOREIGN_KEY with autodetected relation_and_type_importer.
Replaced InternalFieldType.DO_NOT_IMPORT_LAST_UPDATED with a disabled field import.
Replaced set_references_forwarding with the callable forward_references.
Merged objectchange and min test cases into custom test case.
Updated the process for creating test suites based on _INPUTS, _EXPECTED_SUMS, and fixture directories.

- Added `extras.journalentry` => `extras.note` mapping. - Added `ipam.FHRPGroup` => `dcim.interfaceRedundancyGroup`. - Added `disable_related_reference` to `configure_model` to allow disabling referencing e.g. for ObjectChange. - Added `dict` to empty values to cover default value of `_custom_field_type` field. - Added `AUTO_ADD_FIELDS = ("content_types", "status")`, based on existence in Nautobot. - Added content type ids with source and target wrappers to fields summary. - Added importer name to fields summary. - Added check and fail, if referenced record doesn't exist in source data, cache or Nautobot. - Added `fields.relation` field definition. - Added `--save-mappings-file` to management command to save mappings for later comparision. - Allowed PascalCase content types in `configure_model` and `get_or_create_wrapper` - Fixed `extras.tag.content_types` assignment. - Fixed failing `save()` logging when model instance is not `str()`able. - Fixed error when missing `_custom_field_data` field in Nautobot model. - Fixed locations `content_types` assignment. - Fixed using callables in default values. - Refactored definitions and importers to be better reusable and exaplanatory. - Refactored `handle_sibling` to assign the same importer. - Refactored internal type sets to NautobotField properties `is_reference`, `is_integer`, `is_auto_increment`, `is_content_type`, `can_import`. - Renamed `NautobotFieldWrapper` => `NautobotField`. - Replaced `InternalFieldType.GENERIC_FOREIGN_KEY` with autodetected `relation_and_type_importer`. - Replaced `InternalFieldType.DO_NOT_IMPORT_LAST_UPDATED` by disabling the field import. - Replaced `set_references_forwarding` with callable `forward_references`. - Merged `objectchange` and `min` test cases to `custom`. - Updated creating test suites based on `_INPUTS`, `_EXPECTED_SUMS` and fixture directories.

snaselj · 2024-01-24T21:01:48Z

@glennmatthews @chadell

Could you please check this PR?

glennmatthews

Is there anything you can be doing in future to make the PRs a bit smaller and more self-contained? This one is a mix of 10 new features with a half dozen distinct bug fixes and heavy refactoring on top, and I for one find it quite difficult to follow and review as a result.

glennmatthews · 2024-01-25T15:23:54Z

nautobot_netbox_importer/diffsync/models/base.py

+        fields={
+            "postchange_data": "object_data",
+            # TBD: This should be defined on Nautobot side
+            "time": fields.force(),


What does this do?

This is a workaround for fields, that are not saved to Nautobot regularly, and needs special care.

https://github.com/nautobot/nautobot-app-netbox-importer/blob/u/snaselj-napps-172-mappings/nautobot_netbox_importer/generator/fields.py#L106

https://github.com/nautobot/nautobot-app-netbox-importer/blob/u/snaselj-napps-172-mappings/nautobot_netbox_importer/generator/nautobot.py#L496

glennmatthews · 2024-01-25T15:30:26Z

nautobot_netbox_importer/tests/fixtures/3.5/samples/extras.tag.json

@@ -9,7 +9,9 @@
    "name": "November",
    "color": "2f6a31",
    "description": "",
-    "content_types": []
+    "content_types": [
+      56


Huh. I would have thought that if you're using --natural-foreign to generate the NetBox dump that these would be strings, not integer PKs?

These samples are Nautobot data samples to test final Nautobot data against. Generated without --natural-foreign

https://github.com/nautobot/nautobot-app-netbox-importer/blob/u/snaselj-napps-172-mappings/nautobot_netbox_importer/tests/test_import.py#L302

glennmatthews · 2024-01-25T15:32:24Z

nautobot_netbox_importer/tests/test_import.py

    "tenancy.tenantgroup": 1,
-    "tenancy.tenant": 5,
-    "users.user": 1,
+    "users.user": 6,


I still feel like object counts are the absolute minimum test introspection that we should be doing on the migrated data. Are you doing any validation of specific object fields post-import for correctness as well? Because we should be.

We are testing final Nautobot data against stored samples, as described above. The test is here:

https://github.com/nautobot/nautobot-app-netbox-importer/blob/u/snaselj-napps-172-mappings/nautobot_netbox_importer/tests/test_import.py#L292

glennmatthews · 2024-01-25T15:32:57Z

nautobot_netbox_importer/tests/fixtures/3.6.min/samples/dcim.locationtype.json

Why are these old tests being removed? Are they not still relevant to test the importer?

min and objectchange tests has been merged to custom test.

custom test is covering cases, that are not covered by public NetBox dumps.

snaselj · 2024-01-26T04:36:56Z

Is there anything you can be doing in future to make the PRs a bit smaller and more self-contained? This one is a mix of 10 new features with a half dozen distinct bug fixes and heavy refactoring on top, and I for one find it quite difficult to follow and review as a result.

You are right @glennmatthews , this is not nice to review, sorry for it. Will do better next time.

chadell · 2024-01-26T14:12:56Z

nautobot_netbox_importer/diffsync/models/dcim.py

-            "termination_b": "termination_b",
-        },
-    )
+    adapter.configure_model("dcim.cable")


why the fields are not needed anymore?

Thanks to the refactoring, these fields are autodetected now:

https://github.com/nautobot/nautobot-app-netbox-importer/pull/123/files#diff-fe77f230ddce96ba9a9d7b349e5e7acfe3c00818cd1818c543d2c1f29504d39dR913

chadell · 2024-01-26T14:13:12Z

nautobot_netbox_importer/diffsync/models/dcim.py

    )
    adapter.configure_model(
        "dcim.interface",
        fields={
-            "status": "status",


why status is not needed anymore?

Refactoring allows auto-adding fields that are crucial, as status is.

https://github.com/nautobot/nautobot-app-netbox-importer/pull/123/files#diff-6b479358489e04d98598db52da0a5cce8c2636d21202d851e16589d38820e84eR102

https://github.com/nautobot/nautobot-app-netbox-importer/pull/123/files#diff-fe77f230ddce96ba9a9d7b349e5e7acfe3c00818cd1818c543d2c1f29504d39dR412

chadell · 2024-01-26T14:14:03Z

nautobot_netbox_importer/diffsync/models/ipam.py

@@ -38,3 +35,7 @@ def setup(adapter: SourceAdapter) -> None:
            "role": fields.role(adapter, "ipam.role"),
        },
    )
+    adapter.configure_model(


Is this mapping "clean"? no need to do some inner attribute mapping?

Fields comments, group_id, auth_type and auth_key are not imported. Not sure, how to import these fields, doesn't seem to have their Nautobot counterparts.

nautobot_netbox_importer/diffsync/models/locations.py

chadell · 2024-01-26T14:15:52Z

nautobot_netbox_importer/diffsync/models/locations.py

+    field.handle_sibling("site", field.nautobot.name)
+    field.handle_sibling("region", field.nautobot.name)


why are these changes needed?

Need to call handle_sibling after set_importer to assign the same importer on sibling fields. This improves summary, where it's possible to see importer name on sibling fields. Doesn't affect the actual import logic.

chadell · 2024-01-26T14:16:37Z

nautobot_netbox_importer/diffsync/models/locations.py



 def setup(adapter: SourceAdapter) -> None:
    """Setup locations for NetBox importer."""
+
+    def forward_references(wrapper: SourceModelWrapper, references: SourceReferences) -> None:


a description of what is doing could help to understand it

Added a description.

chadell · 2024-01-26T14:16:50Z

nautobot_netbox_importer/diffsync/models/locations.py


    region_type_uid = location_type_wrapper.cache_record(
        {
            "id": "Region",
            "name": "Region",
            "nestable": True,
-            "content_types": None,


why are no needed any more?

The same as status fields, content_types are auto added based on existence in Nautobot.

https://github.com/nautobot/nautobot-app-netbox-importer/pull/123/files#diff-6b479358489e04d98598db52da0a5cce8c2636d21202d851e16589d38820e84eR103

chadell · 2024-01-26T14:17:41Z

nautobot_netbox_importer/diffsync/models/virtualization.py

@@ -24,7 +24,6 @@ def setup(adapter: SourceAdapter) -> None:
    adapter.configure_model(
        "virtualization.vminterface",
        fields={
-            "status": "status",


I understand that status is added to all the models with status, by default?

Correct, as described above.

chadell · 2024-01-26T14:20:02Z

Is there anything you can be doing in future to make the PRs a bit smaller and more self-contained? This one is a mix of 10 new features with a half dozen distinct bug fixes and heavy refactoring on top, and I for one find it quite difficult to follow and review as a result.

You are right @glennmatthews , this is not nice to review, sorry for it. Will do better next time.

the problem is that it takes longer to review than 3 PRs with well-defined changes. I hope to use this PR to understand what needs to be changed to support a change in NetBox/Nautobot, and I feel we lost this opportunity. It's not still clear to me

nautobot_netbox_importer/generator/fields.py

chadell · 2024-01-26T14:24:48Z

nautobot_netbox_importer/generator/fields.py

@@ -23,24 +37,12 @@ def role(adapter: SourceAdapter, source_content_type: str) -> SourceFieldDefinit
        role_wrapper = adapter.configure_model(
            source_content_type,
            nautobot_content_type="extras.role",
-            fields={"color": "color", "content_types": "content_types"},


why not necessary content_types?

Described above, content_types fields are auto added.

chadell · 2024-01-26T14:26:02Z

nautobot_netbox_importer/generator/base.py

 def get_nautobot_field_and_type(
    model: NautobotBaseModelType,
    field_name: str,
-) -> Tuple[Optional[NautobotField], InternalFieldType]:
+) -> Tuple[Optional[DjangoField], InternalFieldType]:


why do we move from NautoboField to DjangoField?

Related to the refactoring of GenericForeignKey fields. These fields are not needed anymore and it was the only fields that were not standard DjangoField. So it was possible to remove this type and replace it with the DjangoField type only.

https://github.com/nautobot/nautobot-app-netbox-importer/pull/123/files#diff-6b479358489e04d98598db52da0a5cce8c2636d21202d851e16589d38820e84eL42

Co-authored-by: Christian Adell <[email protected]>

snaselj · 2024-01-26T16:26:31Z

All comments are resolved now.

chadell · 2024-01-29T06:32:47Z

All comments are resolved now.

I'm going to do a local test, and if passes, I would move it forward

snaselj · 2024-01-29T11:59:45Z

@chadell Added demo execution documentation.

chadell · 2024-01-31T12:40:50Z

docs/dev/demo.md

+
+```bash
+invoke import-netbox \
+  --demo-version 3.6 \


why "demo"? what if I'm importing "actual" data?
could I know which versions are available?

ok, I get it. The demo-version is for testing, then we should make it more evident as an alternative to the --file option that we would use in a real environment

you could alos mention where thi demo data is coming from

Better description added.

chadell · 2024-01-31T12:41:15Z

docs/dev/demo.md

+```bash
+invoke import-netbox \
+  --demo-version 3.6 \
+  --save-mappings-file=generated-mappings.json \


what is the goal of this mappings.json?

Described lated in the document in it's own chapter.

chadell · 2024-01-31T12:47:33Z

docs/dev/demo.md

+
+### DiffSync
+
+The next step is to perform a DiffSync. The output will look similar to the following:


people may not konw what is Diffysync or how it works

chadell · 2024-01-31T12:50:14Z

docs/dev/demo.md

+
+The last part of the import process is to display a summary of the import [described in the user documentation](../user/summary.md).
+
+## Generated Mappings


the data mappings are generated on the fly depending on the NetBox data and Nautobot models, isn't it? so it's something thatcan't be determined a prior by the versions without the real data, isn't it?

chadell

LGTM
I have tested as described by the instructions and it looks great.
Looking forward to seeing a PR with ONLY adding support for NetBox 3.7 before getting the last good to go for release!

snaselj added 2 commits January 23, 2024 13:40

fix: Linters

c092192

snaselj marked this pull request as ready for review January 23, 2024 14:33

snaselj requested review from glennmatthews, lampwins and chadell as code owners January 23, 2024 14:33

glennmatthews reviewed Jan 25, 2024

View reviewed changes

chadell reviewed Jan 26, 2024

View reviewed changes

nautobot_netbox_importer/diffsync/models/locations.py Show resolved Hide resolved

chadell reviewed Jan 26, 2024

View reviewed changes

nautobot_netbox_importer/generator/fields.py Outdated Show resolved Hide resolved

chadell reviewed Jan 26, 2024

View reviewed changes

snaselj and others added 2 commits January 26, 2024 17:24

Update nautobot_netbox_importer/generator/fields.py

919d336

Co-authored-by: Christian Adell <[email protected]>

fix: Add description

4dee93d

snaselj added 4 commits January 29, 2024 11:42

fix: Use URLs in nautobot-server import_netbox

d6edef0

feat: invoke import-netbox task

1e96d1d

doc: Demo

730ad2a

fix: Linters

4d642a3

snaselj requested a review from chadell January 29, 2024 12:30

chadell reviewed Jan 31, 2024

View reviewed changes

chadell approved these changes Jan 31, 2024

View reviewed changes

fix: Better demo description

be0eb3e

snaselj merged commit 54af566 into develop Feb 1, 2024
8 checks passed

snaselj deleted the u/snaselj-napps-172-mappings branch February 1, 2024 09:56

		field.handle_sibling("site", field.nautobot.name)
		field.handle_sibling("region", field.nautobot.name)


		### DiffSync

		The next step is to perform a DiffSync. The output will look similar to the following:


		The last part of the import process is to display a summary of the import [described in the user documentation](../user/summary.md).

		## Generated Mappings

Improve mapping #123

Improve mapping #123

Conversation

snaselj commented Jan 23, 2024

Closes: NaN

What's Changed

snaselj commented Jan 24, 2024

glennmatthews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaselj commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadell commented Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaselj commented Jan 26, 2024

chadell commented Jan 29, 2024

snaselj commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadell left a comment

Choose a reason for hiding this comment

chadell commented Jan 26, 2024 •

edited

Loading