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

Fix CustomField.type Choices #134

Merged
merged 5 commits into from
Feb 19, 2024

Conversation

snaselj
Copy link
Contributor

@snaselj snaselj commented Feb 15, 2024

Closes: NaN

What's Changed

  • Fixed importing non-existing CustomField.type choices.

To Do

@snaselj snaselj force-pushed the u/snaselj-napps-239-other-custom-fields branch from f660b0c to dedc375 Compare February 19, 2024 13:13
@snaselj snaselj marked this pull request as ready for review February 19, 2024 13:55
Comment on lines +60 to +63
Nautobot 2.1 doesn't support custom field types:

- `object`
- `multi-object`
Copy link
Contributor

Choose a reason for hiding this comment

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

this functions is not limited to these specific types not supported, isn't it? I mean, if NetBox had a choice type abc and Nautobot doesn't have it, it will be changed to a text type?

And then, when it tries to import it, it fails with something like:

f510c5ff-e1f8-5adc-9fb5-30bc5791b42e Device 1 | ["Invalid value for custom field 'device_custom_object': Value must be a string"]

because the value is an object that is not a text. do I understand it correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, just one thing is different. The ValidationIssue means that the instance has been imported to Nautobot, but there is an issue in data.

Copy link
Contributor Author

@snaselj snaselj Feb 19, 2024

Choose a reason for hiding this comment

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

When changing CustomField.type from e.g. abc to text, the warning is logged only. Considering adding these import warnings to ValidationIssues as well, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry Jan, not following "the warning is logged only". What else could happen?
What does it imply adding these import warning to ValidationIssues?

Copy link
Contributor Author

@snaselj snaselj Feb 19, 2024

Choose a reason for hiding this comment

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

In this case, the following happens:

  1. Import of CustomField name="device_custom_object"

The field type is changed from object (not in Nautobot) to text in importer function. Only warning is logged for this change. This warning is not displayed in ValidationIssues.

When creating record in Nautobot, it passes validated_save() and no ValidationIssue is created for this record.

  1. Import of Device name="Device 1"

The final Nautobot record is saved, but doesn't pass validated_save(). For these case ValidationIssue is created.

ValidationIssues are displayed in the summary (both text and JSON), however warnings, are just logged to console. My proposal is to change this, so that these warnings from case 1. will be displayed in summary as another ValidationIssue.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, my proposal would be to log the issue from (1) into (2), but not stop saving the Device only because one of the attributes can't be imported (being an optional one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine, will do this as a separate PR.

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

@snaselj snaselj merged commit 79b469f into develop Feb 19, 2024
14 checks passed
@snaselj snaselj deleted the u/snaselj-napps-239-other-custom-fields branch February 19, 2024 16:04
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.

2 participants