-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Rename custom is_powered
property
#3403
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
base: master
Are you sure you want to change the base?
Conversation
I think changing the property is probably not a good idea given it is used a lot within this repo |
I believe it to be very beneficial, because a lot of the time people won't read the docs but just copy&paste some other type, and adjust it as needed, not realising that some fields are not imported into NetBox, carrying them over in a cargo-cult-like manner. Distinguishing fields from/for NetBox and repo-internal-only by using the underscore-prefix should make it obvious to anybody at a glance that later are different from all the others and thus probably "special" in some way. And since it shouldn't be used outside of the repo, renaming it should be pretty safe. But that's just my opinion. If you want it, I can remove the rename and only leave the doc fixes. |
HI @m-hau |
The leading underscore makes it obvious that it's a custom property, and is then consistent with the ports' _is_power_source. Removed it from device types if it's true (the default).
Done. Moved the doc-update to #3404, and left the property-rename here. |
@danner26 @DanSheps @ryanmerolle Thoughts on this? |
I need to review the ramifications of this. This is the first time I am seeing this and this is not a standard attribute |
This is a attribute specific to this repo for devices that aren't powered so that the power validation scripts pass successfully. |
I am more wondering if this is even needed or if there is a way to work this so that it doesn't check for that. Reason being is it is possible to have a chassis without PSU's or interfaces so it would be an "unpowered" device however we handle this fine. I think the better way to do this is;
|
So instead of explicitly defining a device type as non-/powered and based on that validating the allowed/required components, the powered status would be inferred from the defined components and based on that validate the components. Sounds reasonable, but has the disadvantage that there is no "escape hatch" for corner cases, where you would still want to manually override/set it. I did a quick-and-dirty implementation of this (applies to diff --git a/tests/device_types.py b/tests/device_types.py
index 24919397..95c9d139 100644
--- a/tests/device_types.py
+++ b/tests/device_types.py
@@ -75,6 +75,11 @@ class DeviceType:
def validate_power(self):
CUSTOM_POWER_SOURCE_PROPERTY = '_is_power_source'
+ # ignore the power status from the definition, instead infer it from the components
+ self.definition['is_powered'] = False
+ for property_name in ('console-ports', 'console-server-ports', 'power-ports', 'power-outlets', 'interfaces', 'module-bays'):
+ self.definition['is_powered'] |= bool(self.definition.get(property_name, False))
+
# Check if power-ports exists
if self.definition.get('power-ports', False):
# Verify that is_powered is not set to False. If so, there should not be any power-ports defined |
Smallfollow-up to #3391.Fix thepoe
to_is_power_source
rename for ports in the README, forgot that in the original PR.Rename the device-type-level custom property
is_powered
to_is_powered
. The leading underscore makes it obvious that it's a custom property, and is then consistent with the ports'_is_power_source
. Also remove it from device types if it's true (the default).