add support to fetch cluster device information from netbox. #1169
Conversation
corresponding to issue netbox-community#852
plugins/inventory/nb_inventory.py
Outdated
| try: | ||
| # cluster device does not have a slug | ||
| return host["device"]["name"] | ||
| except Exception: |
There was a problem hiding this comment.
Is there a more specific exception we could use like KeyError?
There was a problem hiding this comment.
Could this also use a more common pattern of checking if the key exists before accessing, instead of a try/except block?
There was a problem hiding this comment.
I don't know if i have understand it right, but have developed a solution without try/catch and with returning a suitable value in every case.
New version see below
- rearrange netbox data population to assign a device (test100) to a cluster (testcluster) - later assign this device to test100_vm as the device in cluster where it should be placed test-inventory-* - edit inventorys to respect the new bevaviour with the cluster_device data field
plugins/inventory/nb_inventory.py
Outdated
| def extract_cluster_device(self, host): | ||
| # cluster device does not have a slug | ||
| if host.get("device", None) is None: | ||
| return "" |
There was a problem hiding this comment.
Are you certain this is the correct thing to return? This doesn't seem like a good idea to me
plugins/inventory/nb_inventory.py
Outdated
| # cluster device does not have a slug | ||
| if host.get("device", None) is None: | ||
| return "" | ||
| return host.get("device", {"name": ""}).get("name", "") |
There was a problem hiding this comment.
This looks awful similar to the above return when there is no device key in the dictionary.
Overall I think this needs to be improved
There was a problem hiding this comment.
What is your opinion should we return, if there is no value assigned. Should it be null/None?
Also, is there a handy way to determine the object type of the current host. Then i can only return the cluster_device variable, when the object is an virtual machine.
There was a problem hiding this comment.
What is your opinion should we return, if there is no value assigned. Should it be null/None?
I would follow the pattern set by the other functions in this code. Specifically it appears that most would return (which is short hand for returning None) although some return an empty list where it makes sense to.
…a none type. if value is present return this value.
There was a problem hiding this comment.
i think now all possible errors (in my mind type error, and keys not present) of the host variable was handled.
Also only if there is a assigned cluster_device value its value is returned otherwise there is always a null value.
also i think the high level pattern of the other extractor functions is used.
please have in mind, that i'm not a programmer, possibly i don't understand your thoughts with my rare python understanding.
|
@sc68cal Think when the errors with integration tests was solved, the pull request is ready |
|
@joek-office I'll try and take a look next week, I'm a bit swamped at the moment. I agree that's an unusual error. |
Any news so far? |
|
It's taking me a bit of time to transfer to a new machine to do development work, bear with me |
|
@sc68cal |
|
I'm going to re-run the failing jobs and see if it was just a transient error |
|
Looks like a persistent error. Any ideas to solve the errors? |
Related Issue
#852
New Behavior
in ansible netbox inventory will exist a new hostvar with name cluster_device if corresponding information is set in netbox.
...
Contrast to Current Behavior
currently the information is missing in the netbox inventory
...
Discussion: Benefits and Drawbacks
Benefit: More Information can be used from netbox inventory
drawback: i see no one.
backwards-compatible: i think yes, but can't test how far in the past it gets.
i need this information to configure high availability and replication tasks in proxmox over api.
...
Changes to the Documentation
no changes needed in my opinion
...
Proposed Release Note Entry
add support to fetch cluster device information for virtual machine resources from netbox.
...
Double Check
develbranch.