From a92be5999ccb552441c40c31831404a94d1438df Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Tue, 20 Aug 2024 00:14:02 +0100 Subject: [PATCH] Add some critical types to module_utils and fix logic (#270) This change adds minimal types and uncovers several logic bugs, where users of controller.get_one_or_many assumed it will not return a list. --- plugins/module_utils/arguments.py | 4 +- plugins/module_utils/common.py | 6 ++- plugins/module_utils/controller.py | 42 ++++++++++++++------ plugins/modules/activation.py | 11 ++--- plugins/modules/activation_info.py | 2 +- plugins/modules/controller_token.py | 2 +- plugins/modules/credential.py | 2 +- plugins/modules/credential_info.py | 2 +- plugins/modules/credential_type.py | 2 +- plugins/modules/credential_type_info.py | 4 +- plugins/modules/decision_environment.py | 4 +- plugins/modules/decision_environment_info.py | 8 ++-- plugins/modules/project.py | 9 +++-- plugins/modules/project_info.py | 16 ++++---- tests/sanity/ignore-2.15.txt | 15 +++++++ tests/sanity/ignore-2.16.txt | 15 +++++++ tests/sanity/ignore-2.17.txt | 12 ++++++ tests/sanity/ignore-2.18.txt | 1 + 18 files changed, 114 insertions(+), 43 deletions(-) create mode 100644 tests/sanity/ignore-2.15.txt create mode 100644 tests/sanity/ignore-2.16.txt create mode 100644 tests/sanity/ignore-2.17.txt create mode 120000 tests/sanity/ignore-2.18.txt diff --git a/plugins/module_utils/arguments.py b/plugins/module_utils/arguments.py index 23760346..76ee864f 100644 --- a/plugins/module_utils/arguments.py +++ b/plugins/module_utils/arguments.py @@ -3,13 +3,13 @@ # Copyright: Contributors to the Ansible project # Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause) -from __future__ import absolute_import, division, print_function +from __future__ import absolute_import, annotations, division, print_function __metaclass__ = type from ansible.module_utils.basic import env_fallback -AUTH_ARGSPEC = { +AUTH_ARGSPEC: dict[str, dict[str, object]] = { "controller_host": { "fallback": (env_fallback, ["CONTROLLER_HOST"]), "required": True, diff --git a/plugins/module_utils/common.py b/plugins/module_utils/common.py index 06e79f6c..f4263052 100644 --- a/plugins/module_utils/common.py +++ b/plugins/module_utils/common.py @@ -5,10 +5,14 @@ from __future__ import absolute_import, division, print_function +from typing import Any, Optional, Union + __metaclass__ = type -def to_list_of_dict(result): +def to_list_of_dict( + result: Optional[Union[dict[str, bool], list[dict[Any, Any]]]] +) -> list[dict]: if result is None: return [] if not isinstance(result, list): diff --git a/plugins/module_utils/controller.py b/plugins/module_utils/controller.py index 43e3f853..c01211c6 100644 --- a/plugins/module_utils/controller.py +++ b/plugins/module_utils/controller.py @@ -2,7 +2,13 @@ # Copyright: Contributors to the Ansible project # Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause) -from __future__ import absolute_import, division, print_function +from __future__ import absolute_import, annotations, division, print_function + +from typing import Any, List, NoReturn, Optional, Union + +from ansible.module_utils.basic import AnsibleModule + +from .client import Client __metaclass__ = type @@ -13,7 +19,7 @@ class Controller: IDENTITY_FIELDS = {"users": "username"} ENCRYPTED_STRING = "$encrypted$" - def __init__(self, client, module): + def __init__(self, client: Client, module: AnsibleModule) -> None: self.client = client self.module = module self.result = {"changed": False} @@ -24,7 +30,7 @@ def __init__(self, client, module): self.update_secrets = True @staticmethod - def get_name_field_from_endpoint(endpoint): + def get_name_field_from_endpoint(endpoint: str) -> str: return Controller.IDENTITY_FIELDS.get(endpoint, "name") def get_endpoint(self, endpoint, **kwargs): @@ -69,7 +75,7 @@ def get_item_name(self, item): msg = "Cant determine identity field for Undefined object." raise EDAError(msg) - def fail_wanted_one(self, response, endpoint, query_params): + def fail_wanted_one(self, response, endpoint, query_params) -> NoReturn: sample = response.json.copy() if len(sample["results"]) > 1: sample["results"] = sample["results"][:2] + ["...more results snipped..."] @@ -81,21 +87,30 @@ def fail_wanted_one(self, response, endpoint, query_params): msg = f"Request to {display_endpoint} returned {response.json['count']} items, expected 1" raise EDAError(msg) - def get_exactly_one(self, endpoint, name=None, **kwargs): - return self.get_one_or_many(endpoint, name=name, allow_none=False, **kwargs) + def get_exactly_one( + self, endpoint, name=None, **kwargs + ) -> Optional[dict[str, bool]]: + result = self.get_one_or_many( + endpoint, name=name, allow_none=False, want_one=True, **kwargs + ) + # typing: needed as get_one_or_many can also return lists, not expected + # to reach this ever. + if isinstance(result, list): # pragma: no cover + self.fail_wanted_one(result, endpoint, {}) + return result def resolve_name_to_id(self, endpoint, name): return self.get_exactly_one(endpoint, name)["id"] def get_one_or_many( self, - endpoint, - name=None, - allow_none=True, - check_exists=False, - want_one=True, - **kwargs, - ): + endpoint: str, + name: Optional[str] = None, + allow_none: bool = True, + check_exists: bool = False, + want_one: bool = True, + **kwargs: Any, + ) -> Union[None, dict[str, bool], List]: new_kwargs = kwargs.copy() response = None @@ -142,6 +157,7 @@ def get_one_or_many( if check_exists: self.result["id"] = response.json["results"][0]["id"] return self.result + return None def create_if_needed( self, diff --git a/plugins/modules/activation.py b/plugins/modules/activation.py index d50f2ab0..cdb48156 100644 --- a/plugins/modules/activation.py +++ b/plugins/modules/activation.py @@ -145,6 +145,7 @@ sample: 37 """ +from typing import Any from ansible.module_utils.basic import AnsibleModule @@ -154,7 +155,7 @@ from ..module_utils.errors import EDAError -def lookup(module, controller, endpoint, name): +def lookup(module: AnsibleModule, controller: Controller, endpoint, name): result = None try: result = controller.resolve_name_to_id(endpoint, name) @@ -163,7 +164,7 @@ def lookup(module, controller, endpoint, name): return result -def create_params(module, controller): +def create_params(module: AnsibleModule, controller: Controller) -> dict[str, Any]: activation_params = {} # Get the project id @@ -182,7 +183,7 @@ def create_params(module, controller): params = {"data": {"project_id": project_id}} if module.params.get("rulebook_name"): try: - rulebook = controller.get_one_or_many( + rulebook = controller.get_exactly_one( "rulebooks", name=module.params["rulebook_name"], **params ) except EDAError as e: @@ -270,7 +271,7 @@ def create_params(module, controller): return activation_params -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=True), description=dict(type="str"), @@ -324,7 +325,7 @@ def main(): # Attempt to find rulebook activation based on the provided name try: - activation = controller.get_one_or_many("activations", name=name) + activation = controller.get_exactly_one("activations", name=name) except EDAError as e: module.fail_json(msg=f"Failed to get rulebook activation: {e}") diff --git a/plugins/modules/activation_info.py b/plugins/modules/activation_info.py index 5eeb014d..d3b4493c 100644 --- a/plugins/modules/activation_info.py +++ b/plugins/modules/activation_info.py @@ -87,7 +87,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=False), ) diff --git a/plugins/modules/controller_token.py b/plugins/modules/controller_token.py index 2b185347..729a2022 100644 --- a/plugins/modules/controller_token.py +++ b/plugins/modules/controller_token.py @@ -85,7 +85,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(required=True), description=dict(), diff --git a/plugins/modules/credential.py b/plugins/modules/credential.py index 4233a611..4f506791 100644 --- a/plugins/modules/credential.py +++ b/plugins/modules/credential.py @@ -101,7 +101,7 @@ def lookup(module, controller, endpoint, name): return result -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=True), new_name=dict(type="str"), diff --git a/plugins/modules/credential_info.py b/plugins/modules/credential_info.py index 3d4ffb3d..b5c86638 100644 --- a/plugins/modules/credential_info.py +++ b/plugins/modules/credential_info.py @@ -83,7 +83,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=False), ) diff --git a/plugins/modules/credential_type.py b/plugins/modules/credential_type.py index db75019c..28bbd27d 100644 --- a/plugins/modules/credential_type.py +++ b/plugins/modules/credential_type.py @@ -90,7 +90,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=True), new_name=dict(type="str"), diff --git a/plugins/modules/credential_type_info.py b/plugins/modules/credential_type_info.py index 11b2c6d5..e8f744a0 100644 --- a/plugins/modules/credential_type_info.py +++ b/plugins/modules/credential_type_info.py @@ -4,7 +4,7 @@ # Copyright: Contributors to the Ansible project # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -from __future__ import absolute_import, division, print_function +from __future__ import absolute_import, annotations, division, print_function __metaclass__ = type @@ -81,7 +81,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(type="str", required=False), ) diff --git a/plugins/modules/decision_environment.py b/plugins/modules/decision_environment.py index fa6149ed..4380db2a 100644 --- a/plugins/modules/decision_environment.py +++ b/plugins/modules/decision_environment.py @@ -96,7 +96,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(required=True), new_name=dict(), @@ -155,7 +155,7 @@ def main(): credential_type = None if credential: try: - credential_type = controller.get_one_or_many( + credential_type = controller.get_exactly_one( "eda-credentials", name=credential ) except EDAError as eda_err: diff --git a/plugins/modules/decision_environment_info.py b/plugins/modules/decision_environment_info.py index 588250c9..5c380ce2 100644 --- a/plugins/modules/decision_environment_info.py +++ b/plugins/modules/decision_environment_info.py @@ -60,6 +60,8 @@ ] """ +from typing import Any + from ansible.module_utils.basic import AnsibleModule from ..module_utils.arguments import AUTH_ARGSPEC @@ -69,8 +71,8 @@ from ..module_utils.errors import EDAError -def main(): - argument_spec = dict( +def main() -> None: + argument_spec: dict[str, Any] = dict( name=dict(), ) @@ -90,7 +92,6 @@ def main(): controller = Controller(client, module) decision_environment_name = module.params.get("name") - ret = {} try: ret = controller.get_one_or_many( @@ -100,6 +101,7 @@ def main(): ) except EDAError as eda_err: module.fail_json(msg=str(eda_err)) + raise # https://github.com/ansible/ansible/pull/83814 module.exit_json(decision_environments=to_list_of_dict(ret)) diff --git a/plugins/modules/project.py b/plugins/modules/project.py index ba47fdc8..ee41f9f1 100644 --- a/plugins/modules/project.py +++ b/plugins/modules/project.py @@ -81,6 +81,8 @@ state: absent """ +from typing import Any + from ansible.module_utils.basic import AnsibleModule from ..module_utils.arguments import AUTH_ARGSPEC @@ -89,7 +91,7 @@ from ..module_utils.errors import EDAError -def main(): +def main() -> None: argument_spec = dict( name=dict(required=True), new_name=dict(), @@ -135,7 +137,7 @@ def main(): module.fail_json(msg=str(eda_err)) module.exit_json(**ret) - project_type_params = {} + project_type_params: dict[str, Any] = {} if description: project_type_params["description"] = description if url: @@ -144,11 +146,12 @@ def main(): credential_id = None if credential: try: - credential_id = controller.get_one_or_many( + credential_id = controller.get_exactly_one( "eda-credentials", name=credential ) except EDAError as eda_err: module.fail_json(msg=str(eda_err)) + raise # https://github.com/ansible/ansible/pull/83814 if credential_id is not None: # this is resolved earlier, so save an API call and don't do it again diff --git a/plugins/modules/project_info.py b/plugins/modules/project_info.py index 858d45a9..94034e09 100644 --- a/plugins/modules/project_info.py +++ b/plugins/modules/project_info.py @@ -73,6 +73,8 @@ """ # NOQA # pylint: disable=wrong-import-position, +from typing import Any + from ansible.module_utils.basic import AnsibleModule # pylint: disable=relative-beyond-top-level @@ -82,8 +84,8 @@ from ..module_utils.errors import EDAError -def main(): - argument_spec = dict( +def main() -> None: + argument_spec: dict[str, Any] = dict( name=dict(), ) @@ -104,19 +106,19 @@ def main(): project_name = module.params.get("name") - ret = {} + ret: list[Any] = [] try: - ret = controller.get_one_or_many( + result = controller.get_one_or_many( project_endpoint, name=project_name, want_one=False ) except EDAError as eda_err: module.fail_json(msg=str(eda_err)) - if ret is None: + if result is None: ret = [] - if not isinstance(ret, list): - ret = [ret] + if not isinstance(result, list): + ret = [result] module.exit_json(projects=ret) diff --git a/tests/sanity/ignore-2.15.txt b/tests/sanity/ignore-2.15.txt new file mode 100644 index 00000000..4601b093 --- /dev/null +++ b/tests/sanity/ignore-2.15.txt @@ -0,0 +1,15 @@ +plugins/module_utils/arguments.py future-import-boilerplate!skip +plugins/module_utils/common.py import-3.7!skip +plugins/module_utils/common.py import-3.8!skip +plugins/module_utils/controller.py future-import-boilerplate!skip +plugins/modules/activation.py import-3.7!skip +plugins/modules/activation.py import-3.8!skip +plugins/modules/activation_info.py import-3.7!skip +plugins/modules/activation_info.py import-3.8!skip +plugins/modules/credential_info.py import-3.7!skip +plugins/modules/credential_info.py import-3.8!skip +plugins/modules/credential_type_info.py future-import-boilerplate!skip +plugins/modules/credential_type_info.py import-3.7!skip +plugins/modules/credential_type_info.py import-3.8!skip +plugins/modules/decision_environment_info.py import-3.7!skip +plugins/modules/decision_environment_info.py import-3.8!skip diff --git a/tests/sanity/ignore-2.16.txt b/tests/sanity/ignore-2.16.txt new file mode 100644 index 00000000..4601b093 --- /dev/null +++ b/tests/sanity/ignore-2.16.txt @@ -0,0 +1,15 @@ +plugins/module_utils/arguments.py future-import-boilerplate!skip +plugins/module_utils/common.py import-3.7!skip +plugins/module_utils/common.py import-3.8!skip +plugins/module_utils/controller.py future-import-boilerplate!skip +plugins/modules/activation.py import-3.7!skip +plugins/modules/activation.py import-3.8!skip +plugins/modules/activation_info.py import-3.7!skip +plugins/modules/activation_info.py import-3.8!skip +plugins/modules/credential_info.py import-3.7!skip +plugins/modules/credential_info.py import-3.8!skip +plugins/modules/credential_type_info.py future-import-boilerplate!skip +plugins/modules/credential_type_info.py import-3.7!skip +plugins/modules/credential_type_info.py import-3.8!skip +plugins/modules/decision_environment_info.py import-3.7!skip +plugins/modules/decision_environment_info.py import-3.8!skip diff --git a/tests/sanity/ignore-2.17.txt b/tests/sanity/ignore-2.17.txt new file mode 100644 index 00000000..fc107fe3 --- /dev/null +++ b/tests/sanity/ignore-2.17.txt @@ -0,0 +1,12 @@ +plugins/module_utils/common.py import-3.7!skip +plugins/module_utils/common.py import-3.8!skip +plugins/modules/activation.py import-3.7!skip +plugins/modules/activation.py import-3.8!skip +plugins/modules/activation_info.py import-3.7!skip +plugins/modules/activation_info.py import-3.8!skip +plugins/modules/credential_info.py import-3.7!skip +plugins/modules/credential_info.py import-3.8!skip +plugins/modules/credential_type_info.py import-3.7!skip +plugins/modules/credential_type_info.py import-3.8!skip +plugins/modules/decision_environment_info.py import-3.7!skip +plugins/modules/decision_environment_info.py import-3.8!skip diff --git a/tests/sanity/ignore-2.18.txt b/tests/sanity/ignore-2.18.txt new file mode 120000 index 00000000..479a2b8a --- /dev/null +++ b/tests/sanity/ignore-2.18.txt @@ -0,0 +1 @@ +ignore-2.17.txt \ No newline at end of file