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

#1141 Added virtual_disks to nb_inventory.py #1188

Merged
merged 25 commits into from
Apr 16, 2024

Conversation

tkn2023
Copy link
Contributor

@tkn2023 tkn2023 commented Mar 7, 2024

Added virtual_disks support for inventory.

Related Issue

#1141

New Behavior

This adds virtual_disks to the inventory file. Multiple disks are also supported.

Contrast to Current Behavior

Due to the new feature of virtual disks this was not included in the inventory.

Discussion: Benefits and Drawbacks

  • With this addition you are able to use virtual_disks in an automatic script

Changes to the Documentation

virtual_disks is now added to nb_inventory and can be used with "virtual_disks: True"

Proposed Release Note Entry

Virtual_disks has been added to nb_inventory.py and can be used with "virtual_disks: True"

Double Check

Please put an x into the brackets (like [x]) if you've completed that task.

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

@tkn2023 tkn2023 marked this pull request as ready for review March 11, 2024 14:13
@sc68cal
Copy link
Contributor

sc68cal commented Mar 11, 2024

This needs to add to the integration tests where we have some devices in the JSON file that have virtual disks, to exercise this code.

Copy link
Contributor

@sc68cal sc68cal left a comment

Choose a reason for hiding this comment

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

needs integration tests

Added virtual_disks
added virtual_disks
Updated virtual_disks data in inventory file
corrected syntax error
syntax error corrected
removed duplicate virtual disks
No vm disk in test enviroment test
@tkn2023 tkn2023 marked this pull request as draft March 13, 2024 07:19
@tkn2023
Copy link
Contributor Author

tkn2023 commented Mar 20, 2024

This needs to add to the integration tests where we have some devices in the JSON file that have virtual disks, to exercise this code.

I have not found a JSON file that have virtual disks in them?

@sc68cal
Copy link
Contributor

sc68cal commented Mar 21, 2024

You need to add to existing JSON files or create new ones

tkn2023 and others added 10 commits March 22, 2024 07:28
add virtual disks to test100-vm
added virtual disks to deploy
added to inventory
changed virtual machine to id
* Added virtual_disks to deploy

* Added virtual_disks to inventory

* Update data.json with virtual disks

* Update test_nb_inventory.py

* Update test-inventory.yml

* fix integration tests

---------

Co-authored-by: = <=>
@tkn2023 tkn2023 marked this pull request as ready for review April 11, 2024 05:31
},
"display": "Ethernet1/1",
"display": "Ethernet2/1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, why were these changed? Your commit message does not indicate the reason

Copy link
Contributor

@sc68cal sc68cal Apr 11, 2024

Choose a reason for hiding this comment

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

The reason I ask is because it appears that you changed existing devices in these files instead of creating adding new devices in the inventory that tests your specific feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inventory files were created with the .hacking.
I have just adjusted these files according to the output that was generated with it. I added virtual disks to the netbox_deploy file and have not changed any existing devices.
I used a clean install of netbox with netbox-docker and done everything according to Contribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thank you for the explanation

sc68cal

This comment was marked as resolved.

@sc68cal sc68cal dismissed their stale review April 15, 2024 04:34

automatically generated inventory

@rodvand rodvand merged commit 8084fbd into netbox-community:devel Apr 16, 2024
8 checks passed
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.

None yet

4 participants