Skip to content

Conversation

@akire0ne
Copy link

@akire0ne akire0ne commented Apr 15, 2024

Related Issue

Related to #1081

New Behavior

Makes oob_ip always available in the hostvars if it exists.

Discussion: Benefits and Drawbacks

Not sure why we make oob_ip available only when oob_ip_as_primary_ip is set.

I believe there is value to access this property without altering ansible_host.

Changes to the Documentation

None

Proposed Release Note Entry

  • oob_ip is now returned in hostvars if set
  • Add the ability to set out of band IP as ansible host.

Double Check

  • 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.

@akire0ne akire0ne changed the title feat: make oob_ip available regardless of oob_ip_as_primary_ip feat: make oob_ip available regardless of oob_ip_as_primary_ip Apr 15, 2024
@sc68cal
Copy link
Contributor

sc68cal commented Apr 15, 2024

@Shourai can you please take a look, since you implemented the original feature?

@Shourai
Copy link
Contributor

Shourai commented Apr 16, 2024

I have checked it out and tested it.
It's a really nice solution.

  • With oob_ip_as_primary_ip: false and oob_ip defined but primary ip undefined, the variable oob_ip will be available but ansible_host will not be.

  • With oob_ip_as_primary_ip: true and oob_ip defined but primary ip undefined, the variable oob_ip will be available and ansible_host will also be available.

It's useful if you have oob_ip defined and want to use that variable but not overwrite the primary_ip as ansible_host.

I had not thought about it since in our use-case we don't use the primary_ip and only use oob_ip for our devices.

Out of curiosity, @akire0ne, what use-cases do you have for the oob_ip variable?

@sc68cal
Copy link
Contributor

sc68cal commented Apr 16, 2024

That is a large matrix of behaviors. I don't know if I am a fan of this change.

@sc68cal
Copy link
Contributor

sc68cal commented Apr 16, 2024

Frankly I would rather have one configuration setting, where you either use the primary_ip of a device, or the oob_ip of a device for setting ansible_host, and leave it at that.

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.

Creates too complex behaviors

@akire0ne
Copy link
Author

akire0ne commented Apr 16, 2024

Folks, two different things here

  • Out-of-band IP is displayed on the Device UI in the same manner as Primary IPv4 and Primary IPv6 - So for me I would expect to be displayed in my hostvars as oob_ip , in the same fashion as primary_ip4 and primary_ip6are.
  • Ability to set oob_ip as a ansible_host - We don't have that need, but as @Shourai already implemented something with oob_ip so I didn't wanted to clash with this.

Whatever works for me as long oob_ip is available when set on the device.

@sc68cal are you suggesting something like an ansible_host_ip_source setting, which could be set to either primary_ip4 (default) or oob_ip ? Frankly i'm missing the "too complex" behavior here.

FYI our business case is to be able to fetch / identify OOBM / IPMI easily for our devices.

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.

OK. It took me a couple reads through your comments @akire0ne and @Shourai, because for some reason my brain was just not working properly and I was getting confused.

It was not because of anything you wrote, but for some reason it just didn't stick with me for some reason.

It's sad because the code is quite clear what is going on but I just wasn't getting it. Sorry.

This is good to go, thank you for the contribution and your patience, I will try and do better next time.

@sc68cal sc68cal merged commit 49486bd into netbox-community:devel Apr 17, 2024
@akire0ne
Copy link
Author

Thanks @sc68cal ! Very appreciated

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.

3 participants