Handle data type or empty string in module_utils #1143
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-woerner
reviewed
Sep 18, 2023
plugins/modules/ipapwpolicy.py
Outdated
Comment on lines
297
to
324
| maxlife = ansible_module.params_get_tipe( | ||
| "maxlife", int, allow_empty_string=True) | ||
| minlife = ansible_module.params_get_tipe( | ||
| "minlife", int, allow_empty_string=True) | ||
| history = ansible_module.params_get_tipe( | ||
| "history", int, allow_empty_string=True) | ||
| minclasses = ansible_module.params_get_tipe( | ||
| "minclasses", int, allow_empty_string=True) | ||
| minlength = ansible_module.params_get_tipe( | ||
| "minlength", int, allow_empty_string=True) | ||
| priority = ansible_module.params_get_tipe( | ||
| "priority", int, allow_empty_string=True) | ||
| maxfail = ansible_module.params_get_tipe( | ||
| "maxfail", int, allow_empty_string=True) | ||
| failinterval = ansible_module.params_get_tipe( | ||
| "failinterval", int, allow_empty_string=True) | ||
| lockouttime = ansible_module.params_get_tipe( | ||
| "lockouttime", int, allow_empty_string=True) | ||
| maxrepeat = ansible_module.params_get_tipe( | ||
| "maxrepeat", int, allow_empty_string=True) | ||
| maxsequence = ansible_module.params_get_tipe( | ||
| "maxsequence", int, allow_empty_string=True) | ||
| dictcheck = ansible_module.params_get_tipe( | ||
| "dictcheck", bool, allow_empty_string=True) | ||
| usercheck = ansible_module.params_get_tipe( | ||
| "usercheck", bool, allow_empty_string=True) | ||
| gracelimit = ansible_module.params_get_tipe( | ||
| "gracelimit", int, allow_empty_string=True) |
There was a problem hiding this comment.
All of these should be ansible_module.params_get_type(..). About: allow_empty_string=True this is the wrong parameter right now. The current allow_empty_string needs to be renamed to something like allow_empty_list_item.
rjeffman
force-pushed
the
global_handle_datatype
branch
from
8d7c2ba
to
1cb207e
Compare
rjeffman
force-pushed
the
global_handle_datatype
branch
from
1cb207e
to
1d55bc5
Compare
rjeffman
force-pushed
the
global_handle_datatype
branch
from
1d55bc5
to
8f8f976
Compare
rjeffman
force-pushed
the
global_handle_datatype
branch
from
8f8f976
to
c1efee7
Compare
rjeffman
force-pushed
the
global_handle_datatype
branch
from
c1efee7
to
0f11484
Compare
t-woerner
reviewed
Dec 8, 2023
| @@ -497,6 +497,49 @@ def module_params_get_lowercase(module, name, allow_empty_list_item=False): | |||
| return value | |||
|
|
|||
|
|
|||
| def module_params_get_type( | |||
There was a problem hiding this comment.
The module_params_get_type is misleading. Maybe something like module_params_get_with_type or module_params_get_converted or module_params_get_convert_to_type would be better.
t-woerner
reviewed
Dec 8, 2023
| @@ -1072,6 +1115,28 @@ def params_get_lowercase(self, name, allow_empty_list_item=False): | |||
| """ | |||
| return module_params_get_lowercase(self, name, allow_empty_list_item) | |||
|
|
|||
| def params_get_type( | |||
The parameter 'allow_empty_string' in 'module_params_get' is used to allow an item in a list to be an empty string. The problem is that the naming is misleading, as it is checking a list item rather than a string. This patch rename the parameter to 'allow_empty_list_item' so that it more clearly refers to list itens instead of standalone strings, and do not collide with future parameters that may test for empty strings which are not part of lists.
rjeffman
force-pushed
the
global_handle_datatype
branch
from
0f11484
to
a8d0171
Compare
rjeffman
force-pushed
the
global_handle_datatype
branch
from
a8d0171
to
eadab1d
Compare
varunmylaraiah
approved these changes
Dec 12, 2023
|
@t-woerner what do you think about calling the method "params_get_with_type_cast"? Not only this is more explicitly tells what the method is doing, but it also alows for new usage, such as I have the second example in a local "playground branch", and I'm getting fond of the solution for ensuring idempotence in the face of different value types. (Note: I converted the PR to "draft" so that it is not merged before we are aligned on the method name.) |
rjeffman
force-pushed
the
global_handle_datatype
branch
from
39cbf96
to
7e0e1d1
Compare
Some parameters, in modules, have a specific data type, but allow the use of an empty string to clear the parameter. By providing a method to retrieve the parameter with the correct data type, or optionally an empty string, allows for consistency of parameter handling between different modules.
Use the commom parameter type handling method for parameters that accept a value or an empty string.
Use the commom parameter type handling method for parameters that accept a value or an empty string.
Use the commom parameter type handling method for parameters that accept a value or an empty string.
rjeffman
force-pushed
the
global_handle_datatype
branch
from
7e0e1d1
to
34973c0
Compare
|
As discussed offline, I renamed the new method/function to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some parameters, in modules, have a specific data type, but allow the use of an empty string to clear the parameter.
By providing a method to retrieve the parameter with the correct data type, or optionally an empty string, allows for consistency of parameter handling between different modules.
Currently,
ipapwpolicyandipaidoverrideuserrequires such method, and both were modified to use the module_utils implementation.