nb_inventory - extract serial and asset_tag.

[bk2zsto]

Related Issue

New Behavior

Add serial and asset_tag extraction to netbox.netbox.nb_inventory

Contrast to Current Behavior

serial and asset_tag are currently not extracted

Discussion: Benefits and Drawbacks

serial and asset_tag are useful for inventory operations

A general facility to request arbitrary fields from devices seems possible but likely overkill

Changes to the Documentation

Proposed Release Note Entry

nb_inventory - add serial and asset_tag extraction.

Double Check

• [ x] I have read the comments and followed the CONTRIBUTING.md.
• [ x] I have explained my PR according to the information in the comments or in a linked issue.
• [ x] My PR targets the devel branch.

[bk2zsto]

I cant tell from the integration test log what needs to be corrected?

[rodvand]

I cant tell from the integration test log what needs to be corrected?

In the folder tests/integration/targets/inventory-vX.Y/files/ there are output inventories (.json) which expects to match with the nb_inventory configuration (.yml). As you now return a serial and asset_tag field on devices this needs to be added to the expected output. You should really also add a serial number/asset tag to one or multiple of the devices created in test/integration/netbox-deploy.py and make sure the expected inventory matches the actual inventory returned.

[bk2zsto]

My test-fu is not that strong but ...
I am hoping 1a2e238 can live under the original changelog fragment?
Either way, do I need to anything to trigger CI?

[sc68cal]

My test-fu is not that strong but ... I am hoping 1a2e238 can live under the original changelog fragment? Either way, do I need to anything to trigger CI?

I approved the CI checks, as you can see. Sorry, it required manual intervention.

[sc68cal]

Shouldn't these be return a default value of None if they are not set? Such as

host.get("serial", None)

because we cannot guarantee that the asset tag and serial are set? I see you have changed a lot of the test files and set the values to None, which just means that now all our test data has the serial and asset_tag attributes with None values but I see other instances in this file, like:

       site = host.get("site", None)
        if not isinstance(site, dict):
            # Device has no site
            return []

Basically, you need to have this code deal with instances where asset_tag and serial are not attributes that get returned from the NetBox API?

[bk2zsto]

Thanks for the feedback. Re: test files I was/am really just fumbling around trying things to get a good CI.
I'll add the default handling and make the test files rational again and see what haps.

[bk2zsto]

So the pipeline is/was appeased (3.3 seems to have barfed for something runner-related just now) but I am not completely sure about this:

1. the whole x-nullable of asset_tag vs. serial makes things wonky. Have confirmed (at least on 2.11) that the REST API returns both fields populated or not but asset_tag is null on empty and serial is "" on empty. I get that they are 2 different values but it still made my head hurt quite a bit trying to make the json files "correct"
2. as @sc68cal points out all the tests/integration/inventory-vX.Y/files/*.json files are changed by this PR. My thought was that since tests/integration/netbox-deploy.py is creating the inventory for all targets, all the jsons should match. Not a big deal to change them all at once by just hoovering them up with json.load, iterating _meta.hostvars and writing them back out with json.dump but is changing every file necessary and/or correct?
3. going over master and develop in Netbox I can't tell if the change to append " (asset_tag)" to display is going to stay around
4. the extract*_ methods in plugins/inventory/nb_inventory.py handle extraction with either host.get(propname,None) or try/except blocks. I think these are equivalent but not sure which is the preferred style for netbox-community/ansible_modules
5. noisy commit history as I was trying to get a good pipeline. More than willing to close this one our and resubmit cleaned up and/or welcome git-fu suggestions to squash the noise.

[sc68cal]

@rodvand is pretty good about squashing merging things so don't worry about the commit noise while you were learning, we'll take care of it.

Nice work on getting things passing

[bk2zsto]

Hi Sean, Verkewl! Do I need to do anything else re: your change/re-review? Thanks, - Bill

…

On Tue, Jan 24, 2023 at 11:14 AM Sean M. Collins ***@***.***> wrote: @rodvand <https://github.com/rodvand> is pretty good about squashing merging things so don't worry about the commit noise while you were learning, we'll take care of it. Nice work on getting things passing — Reply to this email directly, view it on GitHub <#826 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7Y6RWNCCSDUL4TXFFDZLDWT75VFANCNFSM6AAAAAAQHCASU4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sc68cal]

This looks pretty good to me but I would like some of the other maintainers to weigh in.

[bk2zsto]

Hi @sc68cal and @rodvand ,

This looks pretty good to me but I would like some of the other maintainers to weigh in.

I am trying to package some Ansible PBs for my team and would like to eliminate the dependency on my local fork.
You guys are the only ones I see on the PR so do other ppl need to take a look?
Let me know if I have more cleanup or if merging this has other problems.

Thanks,
Bill