Confine NGINX version fact to exclude Cisco Nexus switches

[murdok5]

Cisco Nexus Switches may run older version of NGINX internally in which the command 'nginx -v' disables the nxapi socket (deletes sock file) on second or later runs. Tested on CIsco NXOS 9k physical and virtual, and confirmed with Cisco engineers.

Nginx is an underlying component, and should not be managed by resources other than the Cisco module(s) for the Nexus devices, hence prevention of this fact running on the Nexus operating system.

[ekohl]

What an amazing bug. I can imagine it took quite some time to figure it out.

[ekohl]

Can we move this to confine? According to https://puppet.com/docs/facter/3.9/fact_overview.html#main-components-of-simple-resolutions you can repeat it multiple times.

[wyardley]

I think the syntax for confine is like this: https://puppet.com/docs/facter/3.9/custom_facts.html#confining-facts
Not 100% sure of the exact right confine statement for this condition or if you can use !; feel free to post on IRC / Slack if you have questions.
There should also probably be a spec test added for this case.

[murdok5]

Refactored code and cleaned up confine. Regarding spec tests, it is not required for the fact itself, as the addition just says dont run this fact on the Nexus Operating system. I also noticed the spec tests for the fact are simulations on string output, and not confinement.

[wyardley]

@murdok5 Generally, Vox likes to have spec tests for changes... in this case, the test would just test that the fact is undef when the facts match the confine statement, obviously the existing tests validate that it works in normal conditions.

[reidmv]

@wyardley As long as the confine is only checking equality against a string constant, I don't think there's anything of value to unit test.

We don't care about the fact value in the event the system is Windows or a Nexus device, we just want it not to evaluate. Unit tests against confine belong in Facter, not in custom facts that use it. The code we'd ostensibly unit test here is the code inside the confine block, but if all that code does is trivial inequality checking there's no test that can be written which isn't just repeating the same logic.

I would like to see the confines cleaned up a little to make it obvious that the only thing happening is an inequality check. Beyond that, I endorse merging. Code is trivial - not possible to write a unit test that adds value.

confine { Facter.value(:kernel) != 'windows' }
confine { Facter.value(:operatingsystem) != 'nexus' }

[wyardley]

@reidmv Fair enough. I was thinking mostly about the case where some changes to the fact itself could cause this to break again in the future, but I am fine with leaving them out.

Should it be using :os['name'] instead of operatingsystem?

Also, +1 on @ekohl's comments to the OP, great work figuring this out.

[murdok5]

@wyardley I chose the operatingsystem fact since as far as I have tested the issue is restricted to Nexus devices. Attached is a picture of the facts from one of my 9k test VMs for fact reference.

@reidmv thanks for clarification. I have updated the confine block to show single logic per line to improve readability and future additions if needed.

[wyardley]

@murdok5 sorry, I just meant using structured facts vs.
$facts['os']['name'] should be equivalent to the old flat $operatingsystem. However, referencing it from different types of code (rspec vs. ruby vs. Puppet DSL) can be kind of confusing at times. Anyway, this looks good as-is, merging. Thanks for the contribution, and sorry for all the back and forth.