Inventory: Remove unnecessary lists around singular host vars #141 #155

DouglasHeriot · 2020-04-03T05:58:30Z

Implements #141
There's a new plurals option on the inventory. It is True by default to maintain backwards compatibility. When set to False, the group_by options and host_vars all change names to become singular, and unnecessary lists around singular vars are removed.

Once #153 is merged to start on #56 inventory unit tests, I'll add some unit tests for plurals too.

DouglasHeriot · 2020-04-06T16:08:52Z

This is still a work in progress, not actually ready to merge. Need to do a lot more testing.

DouglasHeriot · 2020-04-16T02:41:46Z

This is getting closer. Currently working on #56 adding integration tests as part of this to ensure the work on the plurals option doesn't cause any regressions.

I've found the community.aws collection has a good example of how to do this. https://github.com/ansible-collections/community.aws/tree/master/tests/integration/targets/script_inventory_ec2

Got caught for a little bit on ansible/ansible#68963

Until netbox-community/netbox#4496 is released

…ta (netbox-community#56) Also: * Update .gitignore and galaxy.yml build_ignore with various development files to ignore. * Added .editorconfig with indentation settings for .py and .yml files

…on-default options Includes testing the Constructed inherited options. Also added the same inventory from the past v0.2.0 release. I compared its output to the output of the actualy v0.2.0 release and can confirm nothing has changed - only added new host_vars (config_context, regions) since then, and changed the test data slightly. (ie. there should be no breaking change for existing users of this collection)

DouglasHeriot · 2020-04-21T15:24:21Z

Ok, @FragmentedPacket this is finally ready for a proper review.
Sorry it's become so large - the plurals option is a substantial change so I wanted to submit tests. Ended up going down the rabbit hole of improving all of the integration tests along the way.

Here's a summary of what this pull request includes:

Change request: Remove unnecessary arrays in inventory host vars #141 Introduce plurals option to allow removing unnecessary arrays, and rename vars/groups with correct singular/plural form
Add Tests: Netbox Inventory Plugin #56 Implement integration testing for the inventory plugin. The test is checking the output of ansible-inventory --list against known correct output
Inventory-bug: Changed the lookups to use slug where available #97 Fix compatibility with Ansible's TRANSFORM_INVALID_GROUP_CHARS = always setting.
Inventory: Add regions grouping #178 Implement region group_by option, and extra regions as host_vars
A small step towards Inventory Optimisation: Querying for services and interfaces in bulk rather than by device #143 performance improvement - don't call extract_interfaces, ipaddresses and services twice for every host

More details:

plurals defaults to True for backwards compatibility. (Tested by test-inventory-legacy.yml). Eventually we might want to add a warning and make the default False as it makes more sense.
When plurals is False, the host var device_role has been renamed to role to match role on VMs.
CONTRIBUTING.md has been updated with how I've set up my test environment. It also has details of how to update the inventory test output.
Existing integration tests for the modules have been moved into the tests/integration/targets directory, and changed slightly for compatibility with the ansible-test tool. The main benefit I see of this is we get tooling like code coverage checking sorted out automatically. As these are run like Ansible roles and not playbooks, the netbox.netbox prefix has to be added to all module calls.
Added travis testing against Netbox 2.8 with release version of Ansible. Stuck to Python 3.7 instead of 3.8 due to pylint sanity test fails on Python 3.8 ansible/ansible#67118 Using Netbox develop branch snapshot docker image due to django-rest-framework v3.11 dropped suppport for set_context in UniqueTogetherValidator netbox#4496
Consolidated integration tests - rather than have separate tests for Netbox v2.6, v2.7 and v2.8 (Introduced in Enhancement: Added option to not prepend group names with group_by option #147) I changed it to just v2.6 and latest. There's not yet anything different in 2.7 vs 2.8 so no point adding that complexity right now. Should help avoid mistakes like in Update rack width to comply with latest API documentation #168 the unit tests for 2.6 and 2.7 were updated, but not 2.8 as they didn't exist in the fork that was merged in.

.editorconfig

FragmentedPacket · 2020-04-24T01:14:17Z

@DouglasHeriot I will try and get to this during the weekend. Work has been kicking my butt so haven't had the mental capacity to walk through the PR.

Thanks for all your effort!

FragmentedPacket · 2020-04-26T03:58:17Z

plugins/inventory/nb_inventory.py

+                - If True, all host vars are contained inside single-element arrays for legacy compatibility. Group names will be plural (ie. "sites_mysite" instead of "site_mysite")
+            default: True
+            type: boolean
+            version_added: "0.1.11"


Update version to 0.2.1

plugins/inventory/nb_inventory.py

FragmentedPacket · 2020-04-26T05:22:36Z

plugins/inventory/nb_inventory.py

+        # Keep looping until the region has no parent
+        while region_id is not None:
+            region_slug = self.regions_lookup[region_id]
+            if region_slug in regions:


I tested this and appears it doesn't get stuck in a loop, but maybe my test data isn't correct for L710-L712

No this will never get stuck in a loop – wasn't sure how defensively I should be programming against the Netbox API. It would only be in theory if Netbox had a corrupt database that somehow had a loop in it.

Ok no problem. I don't feel strongly on either having it or not so we can leave it.

I've left this in but updated the comment to be more clear.

FragmentedPacket · 2020-04-26T05:32:42Z

.travis.yml

+  - ansible-test sanity -v --requirements --python $PYTHON_VER --skip-test pep8 --skip-test validate-modules
+
+  # Unit tests, with code coverage
+  - ansible-test units -vvvv --coverage --python $PYTHON_VER


After looking at the tests, what is the least amount of verbosity can we get away with these commands? The test output is pretty heavy with setup information for all this.

Yeah it's very verbose. I wasn't sure whether to go for enough information to debug any issues, or just enough to read and see where it failed. These could probably all be changed to just -v or -vv

Yes that is definitely something to consider. I'll leave that decision up to you.

I've changed it to -v. Should be enough for most cases.

* Mostly updated comments to be clearer. * Changed argument name * Reduced verbosity of travis tests

Netbox 2.8.1 release fixed netbox-community/netbox#4496

FragmentedPacket · 2020-04-27T13:06:11Z

@DouglasHeriot Thanks a lot for this. I can't tell you how much I appreciate your work on this. Great work and the quality is superb.

DouglasHeriot · 2020-04-27T13:28:19Z

@FragmentedPacket thanks for being prompt and easy to work with!
(The Ansible 2.10 release is going to come with a bit of pain, but being able to actually get stuff like this merged with solid testing is big win)

DouglasHeriot force-pushed the 141-plurals-optional branch from 9392cf4 to 432cf82 Compare April 3, 2020 06:14

DouglasHeriot force-pushed the 141-plurals-optional branch 2 times, most recently from 73581bf to f7d2db2 Compare April 14, 2020 02:03

This was referenced Apr 14, 2020

Inventory-bug: Changed the lookups to use slug where available #97

Merged

Inventory: Separate query_filters for devices and vms #140 #153

Merged

DouglasHeriot force-pushed the 141-plurals-optional branch 4 times, most recently from 3619d15 to c1a6ff7 Compare April 15, 2020 07:28

DouglasHeriot mentioned this pull request Apr 15, 2020

django-rest-framework v3.11 dropped suppport for set_context in UniqueTogetherValidator netbox-community/netbox#4496

Closed

DouglasHeriot force-pushed the 141-plurals-optional branch 10 times, most recently from b7d1015 to f8090f7 Compare April 17, 2020 05:00

DouglasHeriot mentioned this pull request Apr 17, 2020

Modules: Fix ansible-test sanity issues #180

Closed

DouglasHeriot force-pushed the 141-plurals-optional branch 7 times, most recently from f88a08a to 31f2f3b Compare April 21, 2020 00:32

Use netbox-docker snapshot image for 2.8

a26d5b0

Until netbox-community/netbox#4496 is released

DouglasHeriot force-pushed the 141-plurals-optional branch from 2435a51 to a26d5b0 Compare April 21, 2020 11:11

Add useful hacking scripts, including to update the inventory test da…

6e8beba

…ta (netbox-community#56) Also: * Update .gitignore and galaxy.yml build_ignore with various development files to ignore. * Added .editorconfig with indentation settings for .py and .yml files

DouglasHeriot force-pushed the 141-plurals-optional branch 4 times, most recently from d17e8c9 to 5536ebf Compare April 21, 2020 14:46

DouglasHeriot force-pushed the 141-plurals-optional branch from 5536ebf to a6825ce Compare April 21, 2020 15:15

DouglasHeriot mentioned this pull request Apr 22, 2020

Inventory bug: services queried by device name doesn't work for VMs, or names with spaces #142

Closed

FragmentedPacket reviewed Apr 24, 2020

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

Remove .editorconfig

f29d5e1

DouglasHeriot mentioned this pull request Apr 24, 2020

Inventory: group_by options for cluster, cluster_type and cluster_group #188

Closed

FragmentedPacket reviewed Apr 26, 2020

View reviewed changes

plugins/inventory/nb_inventory.py Show resolved Hide resolved

FragmentedPacket suggested changes Apr 26, 2020

View reviewed changes

DouglasHeriot added 2 commits April 27, 2020 16:48

Inventory: Minor updates in response to netbox-community#155 review

ffa0b49

* Mostly updated comments to be clearer. * Changed argument name * Reduced verbosity of travis tests

Run travis tests against Netbox 2.8 instead of develop

e72704b

Netbox 2.8.1 release fixed netbox-community/netbox#4496

DouglasHeriot requested a review from FragmentedPacket April 27, 2020 07:00

FragmentedPacket approved these changes Apr 27, 2020

View reviewed changes

FragmentedPacket merged commit 6b42638 into netbox-community:devel Apr 27, 2020

This was referenced Apr 27, 2020

Inventory: Add regions grouping #178

Closed

Change request: Remove unnecessary arrays in inventory host vars #141

Closed

DouglasHeriot mentioned this pull request May 4, 2020

Add Tests: Netbox Inventory Plugin #56

Closed

DouglasHeriot mentioned this pull request May 15, 2020

Unexpected result with force_valid_group_names #206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inventory: Remove unnecessary lists around singular host vars #141 #155

Inventory: Remove unnecessary lists around singular host vars #141 #155

DouglasHeriot commented Apr 3, 2020 •

edited

Loading

DouglasHeriot commented Apr 6, 2020

DouglasHeriot commented Apr 16, 2020

DouglasHeriot commented Apr 21, 2020 •

edited

Loading

FragmentedPacket commented Apr 24, 2020

FragmentedPacket Apr 26, 2020

DouglasHeriot Apr 27, 2020

FragmentedPacket Apr 26, 2020

DouglasHeriot Apr 26, 2020

FragmentedPacket Apr 26, 2020

DouglasHeriot Apr 27, 2020

FragmentedPacket Apr 26, 2020

DouglasHeriot Apr 26, 2020

FragmentedPacket Apr 26, 2020

DouglasHeriot Apr 27, 2020

FragmentedPacket commented Apr 27, 2020

DouglasHeriot commented Apr 27, 2020

Inventory: Remove unnecessary lists around singular host vars #141 #155

Inventory: Remove unnecessary lists around singular host vars #141 #155

Conversation

DouglasHeriot commented Apr 3, 2020 • edited Loading

DouglasHeriot commented Apr 6, 2020

DouglasHeriot commented Apr 16, 2020

DouglasHeriot commented Apr 21, 2020 • edited Loading

FragmentedPacket commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FragmentedPacket commented Apr 27, 2020

DouglasHeriot commented Apr 27, 2020

DouglasHeriot commented Apr 3, 2020 •

edited

Loading

DouglasHeriot commented Apr 21, 2020 •

edited

Loading