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

Enforce selecting ref from network_obj based on network view #157

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

philipsd6
Copy link
Contributor

Since there can be multiple network views containing the same networks, we need to ensure that the correct network in the network_obj list is picked based on a network view name, instead of simply taking it from the first one in the list. The 'default' view is used by default.

Fixes: #156

@ranjishmp
Copy link
Collaborator

Hi @hemanthKa677, please review this PR

Copy link
Collaborator

@hemanthKa677 hemanthKa677 left a comment

Choose a reason for hiding this comment

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

Hi @philipsd6 , Can you please provide us information on how you are debugging this both files? As there are no playbooks to test these features.

@philipsd6
Copy link
Contributor Author

philipsd6 commented Oct 20, 2022

Well, I didn't update any test fixtures because there were no test fixtures in the first place for the lookup plugins. I'll try to take a look to see how we can add test fixtures for lookup plugins and add that. However, this is a small easily reviewed change where the only new implication is that the lookup could fail if an Infoblox system doesn't have a 'default' network view and the user doesn't specify the desired view.

But fwiw, I tested the code directly in our Infoblox test sytem - replicating the error that occurs when the 'default' view is not the first one in the list, and then having it run correctly with the code I added to the plugin.

Copy link
Collaborator

@hemanthKa677 hemanthKa677 left a comment

Choose a reason for hiding this comment

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

I have tested changes locally. Please make these changes, add the network_view attribute in both examples of lookup files. Please write network instead net in the both files, for better understanding the code. Beside this everything looks good.

@philipsd6 philipsd6 force-pushed the ref_named_network_view branch from a26160b to 51eb186 Compare October 24, 2022 18:19
@philipsd6
Copy link
Contributor Author

@hemanthKa677 Done as requested.

Copy link
Collaborator

@hemanthKa677 hemanthKa677 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Collaborator

@hemanthKa677 hemanthKa677 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Collaborator

@hemanthKa677 hemanthKa677 left a comment

Choose a reason for hiding this comment

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

@philipsd6 Please find that CI build checks are failing with some unit and sanity tests, Can you please fix those? In the sanity test devel, it is failing not only with pep8 linting issue, but also because of preferred_primaries in the nios_nsgroup.py it is given default as empty list, but it is actually None.

@hemanthKa677 hemanthKa677 merged commit 5976756 into infobloxopen:master Nov 9, 2022
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.

Error "Can not find requested number of networks" using nios_next_network lookup
3 participants