Handle rule descriptions according to BIG-IP version#1308
Handle rule descriptions according to BIG-IP version#1308caphrim007 merged 2 commits intoF5Networks:developmentfrom
Conversation
Issues: Fixes F5Networks#1307 Problem: The 'description' parameter does not exist in policy rules prior to BIG-IP v12.1.0. Attempting to set the rule description for these earlier BIG-IP versions results in an error response. Analysis: If the BIG-IP version is earlier than v12.1.0, strip the description from policy rule. Tests: test_rules_description (f5/bigip/tm/ltm/test/functional/test_policy.py)
wojtek0806
left a comment
wojtek0806
left a comment
There was a problem hiding this comment.
Please rework this as per my comments
| tmos_ver = self._meta_data['bigip']._meta_data['tmos_version'] | ||
| if LooseVersion(tmos_ver) < LooseVersion('12.1.0'): | ||
| for r in patch.get('rules', []): | ||
| r.pop('description', None) |
There was a problem hiding this comment.
If we are going to make this change, and what it seems this might be one of many such "exceptions", it would be prudent to have this folded into _meta_data, for example _meta_data['optional_parameters'], similarly to:
https://github.com/F5Networks/f5-common-python/blob/development/f5/bigip/resource.py#L839
Also if we silently remove the description from **kwargs how would the caller know something went wrong? If the caller does not know that a certain attribute does not exist in the specific version, we should inform him by raising exception with appropriate method, rather than just removing.
The only exception to such rule is here, and this is due to the fact that we send the entire dict (minus __meta_data) to the device when we do update(), and state/session parameters are read only in specific situations, so the caller need not to be notified, unless the caller attempt to set them to an illegal value:
https://github.com/F5Networks/f5-common-python/blob/development/f5/bigip/tm/ltm/node.py#L95
https://github.com/F5Networks/f5-common-python/blob/development/f5/bigip/tm/ltm/pool.py#L131
We raise if the parameter is set to illegal value by the caller:
https://github.com/F5Networks/f5-common-python/blob/development/f5/bigip/tm/ltm/pool.py#L209
There was a problem hiding this comment.
The above behavior of update() will be modified once i get a chance, the issue is already opened up for it:
#1063
Created a metadata entry to denote the optional policy parameters and created a helper function to filter out these version-specific optionals.
wojtek0806
left a comment
wojtek0806
left a comment
There was a problem hiding this comment.
i would remove logging and raise exception, after that is done this can be merged
| for parm in parms: | ||
| value = r.pop(parm, None) | ||
| if value is not None: | ||
| logger.info( |
There was a problem hiding this comment.
I would suggest raising an error, it is easier to handle by the caller
There was a problem hiding this comment.
This is taken from our dev playbook, which is what we are adhering to, so what has changed? specifying a non existent parameter is a failure
Failures generate exceptions
If the SDK enters a surprising or unexpected state it raises an exception. That's it. It's not generally up to the SDK to implement decision logic that handles edge-cases.. EXCEPT where the SDK is smoothing known issues in the device REST server. (See below.)
| self._meta_data['attribute_registry'] = temp | ||
| self._meta_data['optional_parameters'] = {'rules': ['description']} | ||
|
|
||
| def _filter_version_specific_options(self, tmos_ver, **kwargs): |
There was a problem hiding this comment.
i think the point was missed here, i was talking about streamlining this into resource.py, but fine since this is a bigger job, just open an issue for it and assign it to me, i will do this later with update() rework
|
I look at the description issue as a form of deprecation. Deprecation needs raise no exception. Indeed, the code will continue on, but along the way will need to tweak things because of the deprecated code path So for now I'm ok with this |
3 participants
Issues:
Fixes #1307
Problem:
The 'description' parameter does not exist in policy rules prior
to BIG-IP v12.1.0. Attempting to set the rule description for these
earlier BIG-IP versions results in an error response.
Analysis:
If the BIG-IP version is earlier than v12.1.0, strip the description from
policy rule.
Tests:
test_rules_description (f5/bigip/tm/ltm/test/functional/test_policy.py)