Add Individual Resource Verification Override #67
Conversation
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
HonakerM
force-pushed
the
add_resource_verification
branch
from
639a2e3
to
c324526
Compare
HonakerM
force-pushed
the
add_resource_verification
branch
from
fd18435
to
b475934
Compare
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
|
@gabe-l-hart this is now readyto review! |
oper8/verify_resources.py
Outdated
| @@ -39,6 +45,7 @@ def verify_resource( | |||
| # Use a predfined _SESSION_NAMESPACE default instead of None to differentiate between | |||
| # nonnamespaced resources (which pass None) and those that use session.namespace | |||
| namespace: Optional[str] = _SESSION_NAMESPACE, | |||
| verify_function: RESOURCE_VERIFY_FUNCTION = None, | |||
There was a problem hiding this comment.
NIT: This should be Optional[RESOURCE_VERIFY_FUNCTION]. While you're at it, can you fix the other missing Optional[] type hints on the below args?
oper8/dag/node.py
Outdated
|
|
||
| def __init__(self, name: str, manifest: dict): | ||
| def __init__(self, name: str, manifest: dict, verify_func: Callable = None): |
There was a problem hiding this comment.
Should this use the full RESOURCE_VERIFY_FUNCTION signature rather than just Callable?
There was a problem hiding this comment.
Circular references :( I wasn't able to figure out a way to import verify_resources in the node dag.
oper8/dag/node.py
Outdated
| @property | ||
| def manifest(self) -> str: | ||
| """The resource manifest""" | ||
| return self.get_data()[0] |
There was a problem hiding this comment.
Interesting. I wonder if having a Tuple returned from get_data is going to break anything. I think I might have at least one place where I've invoked get_data directly, so this could be seen as an API breaking change.
oper8/managed_object.py
Outdated
| @@ -10,7 +11,7 @@ | |||
| class ManagedObject: # pylint: disable=too-many-instance-attributes | |||
| """Basic struct to represent a managed kubernetes object""" | |||
|
|
|||
| def __init__(self, definition): | |||
| def __init__(self, definition, verify_function: Callable = None): | |||
There was a problem hiding this comment.
Same question about Callable here. Also, if you do change it, maybe throw a type hint on definition too!
oper8/verify_resources.py
Outdated
| @@ -282,3 +289,9 @@ def is_expected_reason() -> bool: | |||
| return obj_reason == expected_reason | |||
|
|
|||
| return is_expected_status() and is_expected_reason() | |||
| return is_expected_status() and is_expected_reason() | |||
There was a problem hiding this comment.
VS Code likes to autoformat things :( I think it removed a newline?
There was a problem hiding this comment.
This diff shows six copies of the same return statement?
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
HonakerM
force-pushed
the
add_resource_verification
branch
from
8726eca
to
f5ea182
Compare
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
HonakerM
force-pushed
the
add_resource_verification
branch
from
5f73d3a
to
4275ad8
Compare
oper8/dag/node.py
Outdated
| return self.get_data() | ||
|
|
||
| @property | ||
| def verify_function(self) -> str: |
There was a problem hiding this comment.
Crap, one more NIT: The return on this should be Callable
There was a problem hiding this comment.
Good catch! I will also fix the manifest section as well
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Related Issue
Supports #66
Related PRs
This PR helps support #64
What this PR does / why we need it
This PR allows users to override
Special notes for your reviewer
If applicable**
What gif most accurately describes how I feel towards this PR?