-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Added config-context in hostvars #47394
Conversation
Hi @nikkytub, thank you for submitting this pull-request! |
@@ -198,6 +198,7 @@ def group_extractors(self): | |||
"device_roles": self.extract_device_role, | |||
"platforms": self.extract_platform, | |||
"device_types": self.extract_device_type, | |||
"config_contexts": self.extract_config_contexts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be config_context
? There can be only one context per device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Will change it.
def extract_config_contexts(self, host): | ||
try: | ||
url = urljoin(self.api_endpoint, "/api/dcim/devices/" + str(host["id"])) | ||
device_lookup = self._fetch_information(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you doing another fetch? When the host is fetched, the config_context attribute already exists in the host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because host shows only local context not the rendered context. For example-
random1": {
"config_context": [
null
],
"device_roles": [
"ACI IPN"
],
"device_types": [
"random1"
],
"manufacturers": [
"Cisco"
],
"racks": [
"XYZ"
],
"sites": [
"ABC"
],
"tenants": [
"ABC"
]
},
"random": {
"config_context": [
{
"cc": {
"net": {
"vpn_asn": 66000
}
}
}
],
"device_roles": [
"ACI IPN"
],
"device_types": [
"random"
],
"manufacturers": [
"Cisco"
],
"racks": [
"XYZ"
],
"sites": [
"ABC"
],
"tenants": [
"ABC"
]
However, if we would like to see the rendered context from the source contexts I had to do another fetch.
"random1": {
"config_context": [
{
"cc": {
"net": {
"vpn_asn": 65000
}
}
}
],
"device_roles": [
"ACI IPN"
],
"device_types": [
"random1"
],
"manufacturers": [
"Cisco"
],
"racks": [
"XYZ"
],
"sites": [
"ABC"
],
"tenants": [
"ABC"
]
},
"random": {
"config_context": [
{
"cc": {
"net": {
"vpn_asn": 66000
}
}
}
],
"device_roles": [
"ACI IPN"
],
"device_types": [
"random"
],
"manufacturers": [
"Cisco"
],
"racks": [
"XYZ"
],
"sites": [
"ABC"
],
"tenants": [
"ABC"
]
LGTM |
Could this also be also extended to include services assigned to the device?
|
@ollybee Please create a feature request explaining what will it bring? Why is it necessary? and then please send a separate pull request. |
@nikkytub this PR contains the following merge commits: Please rebase your branch to remove these commits. |
@nikkytub This PR was evaluated as a potentially problematic PR for the following reasons:
Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: |
f65a5a4
to
ecc71ce
Compare
ecc71ce
to
f5f34f8
Compare
@gundalow @FragmentedPacket Please review the changes |
LGTM |
@gundalow Could you please review and merge it? |
rebuild_merge |
@gundalow Thank you so much :) |
* Added config-contexts in hostvars * Changed config-contexts to config-context in hostvars
* Added config-contexts in hostvars * Changed config-contexts to config-context in hostvars
While this has merged, it has actually introduced a huge regression. In environments with large number of devices, it has to make a request for every single device in the environment which doesn't even complete in my case but gives a "too many open files" error here. How can we revise this to be opt-in, or perhaps figure out a way to get the context data in the original list? |
@mnaser Thank you for your feedback. I will make changes to use it as an opt-in. |
SUMMARY
config-context addition in hostvars in netbox dynamic inventory
Fixed #47391
ISSUE TYPE
COMPONENT NAME
config-context in hostvars in netbox dynamic inventory
ANSIBLE VERSION
ADDITIONAL INFORMATION
Before change
After change