Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a default value for Interface.type #97

Merged
merged 6 commits into from
Mar 20, 2023

Conversation

jmcgill298
Copy link
Contributor

Netbox and Nautobot have different choice options for the Interface.type field. This defaults to using the "other" type in Nautobot when the Netbox type is not supported by Nautobot.

Netbox and Nautobot have different choice options for the Interface.type
field. This defaults to using the "other" type in Nautobot when the
Netbox type is not supported by Nautobot.
@jmcgill298
Copy link
Contributor Author

@chadell what do you think of this for #50 ?

@chadell
Copy link
Contributor

chadell commented Mar 10, 2023

@chadell what do you think of this for #50 ?

I think it's a sane approach that supports any unexpected interface type not supported, and a pattern we can reuse in other cases where the enums are different.
Even in the case we sync more types (nautobot/nautobot#3377), we will always need this extra coverage.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nitpicks but this absolutely looks like a solid approach.

nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
@jmcgill298 jmcgill298 force-pushed the bugfix-50/interface-type-choices branch from ec75451 to 921f1ab Compare March 16, 2023 15:00
@jmcgill298 jmcgill298 marked this pull request as ready for review March 16, 2023 15:18
@jmcgill298
Copy link
Contributor Author

@chadell @glennmatthews I changed this up to centralize this code some, and some of these models won't have an other type until https://github.com/nautobot/nautobot/pull/3413/files is merged and in the Nautobot version being used, but the end result would be the same (other is just as invalid as the type currently being used)

The `_type_choices` variable should be a set of the valid choices for the type per
Nautobot.
"""
if "type" not in cls.__fields__ or "_type_choices" not in cls.__dict__:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the cleanest way to do this, or would something like if not hasattr(cls, "type") or not hasattr(cls, "_type_choices") work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the Pydantic definitions are not class attributes, and so hasattr(cls, "type") will fail. However, I think we can simplify this just to check for _type_choices and any implementation that opts in has to define a type field (which why define _type_choices without having a type?)

nautobot_netbox_importer/diffsync/models/abstract.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/dcim.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/tests/fixtures/netbox_dump.json Outdated Show resolved Hide resolved
# model: dcim.cable

#- fields:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y'know, I don't recall why the cable instances are commented out in this validation data. Is it because of the use of GenericForeignKeys making it hard to know in advance what the termination_*_id value will be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, and I felt like I've spent too much time on this PR already and didn't want to go down another rabbit hole

Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chadell chadell merged commit 6805b14 into develop Mar 20, 2023
@chadell chadell deleted the bugfix-50/interface-type-choices branch March 20, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants