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 get() by modelname #118

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

glennmatthews
Copy link
Collaborator

This appears to be a regression in 1.5.0, which I noticed in working on nautobot/nautobot-app-netbox-importer#75 - the pattern diffsync.get("modelname", {identifier: value, ...}) was failing with an error:

ValueError: Invalid args: (customfieldchoice, {'field': {'name': 'Custom Choices Field'}, 'value': 'alpha'}): either customfieldchoice should be a class/instance or {'field': {'name': 
'Custom Choices Field'}, 'value': 'alpha'} should be a str

The root cause was that BaseStore._get_object_class_and_model() was looking for the model class by name on self rather than on self.diffsync, which as such would always fail since the model classes are an expected attribute of the DiffSync object not the Store.

I've added a unit test to reproduce the issue, fixed the issue, and clarified the error message.

Copy link
Collaborator

@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

@glennmatthews glennmatthews merged commit 51e56cc into develop Jun 30, 2022
@glennmatthews glennmatthews deleted the gfm-fix-get-by-modelname-and-identifiers branch June 30, 2022 17:17
@glennmatthews glennmatthews mentioned this pull request Jun 30, 2022
glennmatthews added a commit that referenced this pull request Jun 30, 2022
* New Example06 - IP Prefixes (#111)

* first draft of example06

Co-authored-by: Glenn Matthews <[email protected]>

* Attempt to fix the read the docs pipeline. (#115)

* Attempt to fix the read the docs pipeline.

* Yamllint.

Co-authored-by: Leo Kirchner <[email protected]>

* Update CODEOWNERS (#113)

* Fix get() by modelname (#118)

* Update example05 (#107)

* Update example05

* Use site as children

* Add update after adding children

* Add pylint disable until Redis code is in

* Update example

* simplify

* wip

* wip

* Update example

* Take redis from main

* imprort order

* yml

* update readme

* Use diffsync from pypi

* Apply suggestions from code review

Co-authored-by: Glenn Matthews <[email protected]>

* Code review

* replace bash by python exec

* Rename dockerfile to Dockerfile

* Update docs source

Co-authored-by: Glenn Matthews <[email protected]>

* Update CHANGELOG and bump version

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants