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

Log warning for fields in NetBox data not supported by DS Model #101

Merged
merged 8 commits into from
May 23, 2023

Conversation

jmcgill298
Copy link
Contributor

No description provided.

@jmcgill298
Copy link
Contributor Author

@glennmatthews does this make sense to do? If so, is there a better way/place to do this?

@glennmatthews
Copy link
Contributor

My initial concern would be over how potentially noisy this can be. Does it only log once per model, or does it log for every instance of that model?

@jmcgill298
Copy link
Contributor Author

It logs per instance if any of the unsupported values have a value.

@chadell
Copy link
Contributor

chadell commented Apr 25, 2023

Could you share the output of this change with the current data set?
Also, what do you think about re-creating the unsupported attributes as Custom Fields? For now, it makes sense to make it easy (i.e. logging), but it could be an interesting feature to add in the future.

@glennmatthews
Copy link
Contributor

Logging once per instance feels like it could be potentially overwhelming for a large data set. Could we maybe instead log once per unsupported field, say on the first time that field is seen to have a non-empty/non-null value in the source data? Maybe as a future enhancement we could keep a counter of the number of non-empty/non-null values seen and discarded per field and report that in a summary at the end?

I also like Christian's idea of auto-creating custom fields to store data that is easily represented as such.

@glennmatthews
Copy link
Contributor

Other than that I think your implementation is on the right track here, doing this in the model __init__ seems like the logical place to do so.

@chadell
Copy link
Contributor

chadell commented Apr 26, 2023

Maybe we could get something in the middle? We could allow a summary and verbose mode, so we log per unsupported field on the first (with a counter as stretch) and, in the second, we output all the detailed data.
So, we leave the choice to the user

@jmcgill298 jmcgill298 force-pushed the unsupported_fields branch from cb87b0e to d2cd72c Compare May 3, 2023 22:03
@jmcgill298
Copy link
Contributor Author

@chadell @glennmatthews I refactored this in hopes to accommodate both points of feedback, outside of cleaning up some things, is there any concerns about this approach?

@jmcgill298 jmcgill298 marked this pull request as draft May 3, 2023 22:06
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.

General approach looks reasonable to me. A few questions.

nautobot_netbox_importer/diffsync/models/abstract.py Outdated Show resolved Hide resolved
nautobot_netbox_importer/diffsync/models/extras.py Outdated Show resolved Hide resolved
@jmcgill298 jmcgill298 force-pushed the unsupported_fields branch from db26a3f to d774804 Compare May 8, 2023 17:03
@jmcgill298 jmcgill298 marked this pull request as ready for review May 8, 2023 17:06
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 6a0a7a1 into develop May 23, 2023
@chadell chadell deleted the unsupported_fields branch May 23, 2023 04:46
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