Keycloak module cleanup and consistency (#3280)

* Consistent Modules - Rename updated_?? to desired_?? in all the keycloak modules.

* Consistent Modules - Rename the comments, and add whitespace so that all the modules are a lot more consistent between each other.

* Consistent Modules - Remove final elif where a final else doesn't exist.

This is to address the inconsistency between the other modules.

Whilst I can see it being more descriptive, there should be a final "else:" to cater if the values is neither 'absent' or 'present'.

* Consistent Modules - Use dict() instead of {} like most of the other keycloak modules.

* Consistent Modules - Update keycloak authentication so that the if ordering is consistent for no-item.

* Consistent Modules - Move the 'Filter and map' process to always occur before getting an existing item.

* Consistent Modules - Be consistent with how to initialse before_?? and set it to dict() if it is None.

* Consistent Modules - Add module.exit_?? in the locations as per the other modules.

* Consistent Modules - Represent result['diff'] using dict(before=.., after=...) as per all the other modules.

* Consistent Modules - Add / Move location of when result['end_state'] is getting defined.

* Consistent modules - Add result['changed'] = False where we do nothing and exit because item exists.

* Consistent Modules - Set the value result['changed'] to True earlier so it shows up when in checking mode only.

* Consistent Modules - test for equality with a dict to assert there was no realm in the first place as per the other modules.

* Consistent Modules - Address the spelling.

* Consistent Modules - keycloak_group - Remove result['group'] as result['end_state'] is the consistent value used in the other modules.

* Consistent Modules - Order the lines in the section, Do nothing and exit consistently.

* Consistent Modules - Add result['end_state'] and still add deprecated `flow` return value.

* Consistent Modules - Add missing return documentation for `msg`.

* Consistent Modules - Tweak whitespace in the RETURN variable.

* Consistent Modules - Add result['group'] in addition to deprecated result['group'] response.

* Consistent Modules - Add return property, 'contains' to address test errors.

* Consistent Modules - Rename updated_?? to desired_?? in new modules since initial PR.

* Consistent Modules - Rename the comments, and add whitespace so that all the (recently added) modules are a lot more consistent between each other.

* Consistent Modules - Make indentation consistent within the response document.

* Consistent Modules - Use B(DEPRECATED) in a seperate line in the description.

* Consistent Modules - Add a lot of full stops to sentences.

* Consistent Modules - Use C(...) and I(...) formatting methods.

* Consistent Modules - Use "on success" everywhere for end_state response documentation.

* Consistent Modules - Update the documents for RETURN.proposed, RETURN.existing, RETURN.end_state to be the same.

* Consistent Modules - Add fragment.

* Remove period after short_description.

* Update changelog fragment.

* Consistent Modules - PRFeedback - Remove `module.exit_json(**result)` within the `Delete` section of the if statement.

There's a exit_json(..) immediately after.

* Consistent Modules - PRFeedback - Use `if not x_repr` instead of `if x_repr == dict()`.

* keycloak_authentication - Add a sample of the output.

* Replace `dict()` with `{}` for all the keycloak modules.

* Add the requested deprecated notices

* Update changelogs/fragments/3280-keycloak-module-cleanup-and-consistency.yml

Co-authored-by: Pierre Dumuid <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>

changelogs/fragments/3280-keycloak-module-cleanup-and-consistency.yml

@@ -0,0 +1,6 @@
+deprecated_features:
+  - keycloak_authentication - the return value ``flow`` is now deprecated and will be removed in community.general 6.0.0; use ``end_state`` instead (https://github.com/ansible-collections/community.general/pull/3280).
+  - keycloak_group - the return value ``group`` is now deprecated and will be removed in community.general 6.0.0; use ``end_state`` instead (https://github.com/ansible-collections/community.general/pull/3280).
+
+minor_changes:
+  - keycloak_* modules - refactor many of the ``keycloak_*`` modules to have similar structures, comments, and documentation (https://github.com/ansible-collections/community.general/pull/3280).

plugins/modules/identity/keycloak/keycloak_authentication.py

@@ -9,11 +9,15 @@
 DOCUMENTATION = '''
 ---
 module: keycloak_authentication
+
 short_description: Configure authentication in Keycloak
+
 description:
     - This module actually can only make a copy of an existing authentication flow, add an execution to it and configure it.
     - It can also delete the flow.
+
 version_added: "3.3.0"
+
 options:
     realm:
         description:
@@ -79,6 +83,7 @@
         default: false
         description:
             - If C(true), allows to remove the authentication flow and recreate it.
+
 extends_documentation_fragment:
 - community.general.keycloak
 
@@ -162,10 +167,74 @@
 '''
 
 RETURN = '''
+msg:
+    description: Message as to what action was taken.
+    returned: always
+    type: str
+
 flow:
-  description: JSON representation for the authentication.
-  returned: on success
-  type: dict
+    description:
+      - JSON representation for the authentication.
+      - Deprecated return value, it will be removed in community.general 6.0.0. Please use the return value I(end_state) instead.
+    returned: on success
+    type: dict
+    sample: {
+      "alias": "Copy of first broker login",
+      "authenticationExecutions": [
+        {
+          "alias": "review profile config",
+          "authenticationConfig": {
+            "alias": "review profile config",
+            "config": { "update.profile.on.first.login": "missing" },
+            "id": "6f09e4fb-aad4-496a-b873-7fa9779df6d7"
+          },
+          "configurable": true,
+          "displayName": "Review Profile",
+          "id": "8f77dab8-2008-416f-989e-88b09ccf0b4c",
+          "index": 0,
+          "level": 0,
+          "providerId": "idp-review-profile",
+          "requirement": "REQUIRED",
+          "requirementChoices": [ "REQUIRED", "ALTERNATIVE", "DISABLED" ]
+        }
+      ],
+      "builtIn": false,
+      "description": "Actions taken after first broker login with identity provider account, which is not yet linked to any Keycloak account",
+      "id": "bc228863-5887-4297-b898-4d988f8eaa5c",
+      "providerId": "basic-flow",
+      "topLevel": true
+    }
+
+end_state:
+    description: Representation of the authentication after module execution.
+    returned: on success
+    type: dict
+    sample: {
+      "alias": "Copy of first broker login",
+      "authenticationExecutions": [
+        {
+          "alias": "review profile config",
+          "authenticationConfig": {
+            "alias": "review profile config",
+            "config": { "update.profile.on.first.login": "missing" },
+            "id": "6f09e4fb-aad4-496a-b873-7fa9779df6d7"
+          },
+          "configurable": true,
+          "displayName": "Review Profile",
+          "id": "8f77dab8-2008-416f-989e-88b09ccf0b4c",
+          "index": 0,
+          "level": 0,
+          "providerId": "idp-review-profile",
+          "requirement": "REQUIRED",
+          "requirementChoices": [ "REQUIRED", "ALTERNATIVE", "DISABLED" ]
+        }
+      ],
+      "builtIn": false,
+      "description": "Actions taken after first broker login with identity provider account, which is not yet linked to any Keycloak account",
+      "id": "bc228863-5887-4297-b898-4d988f8eaa5c",
+      "providerId": "basic-flow",
+      "topLevel": true
+    }
 '''
 
 from ansible_collections.community.general.plugins.module_utils.identity.keycloak.keycloak \
@@ -271,9 +340,11 @@ def create_or_update_executions(kc, config, realm='master'):
 def main():
     """
     Module execution
+
     :return:
     """
     argument_spec = keycloak_argument_spec()
+
     meta_args = dict(
         realm=dict(type='str', required=True),
         alias=dict(type='str', required=True),
@@ -292,6 +363,7 @@ def main():
         state=dict(choices=["absent", "present"], default='present'),
         force=dict(type='bool', default=False),
     )
+
     argument_spec.update(meta_args)
 
     module = AnsibleModule(argument_spec=argument_spec,
@@ -301,13 +373,15 @@ def main():
                            )
 
     result = dict(changed=False, msg='', flow={})
+
     # Obtain access token, initialize API
     try:
         connection_header = get_token(module.params)
     except KeycloakError as e:
         module.fail_json(msg=str(e))
 
     kc = KeycloakAPI(module, connection_header)
+
     realm = module.params.get('realm')
     state = module.params.get('state')
     force = module.params.get('force')
@@ -323,35 +397,54 @@ def main():
     }
 
     auth_repr = kc.get_authentication_flow_by_alias(alias=new_auth_repr["alias"], realm=realm)
-    if auth_repr == {}:  # Authentication flow does not exist
-        if state == 'present':  # If desired state is present
+
+    # Cater for when it doesn't exist (an empty dict)
+    if not auth_repr:
+        if state == 'absent':
+            # Do nothing and exit
+            if module._diff:
+                result['diff'] = dict(before='', after='')
+            result['changed'] = False
+            result['end_state'] = {}
+            result['flow'] = result['end_state']
+            result['msg'] = new_auth_repr["alias"] + ' absent'
+            module.exit_json(**result)
+
+        elif state == 'present':
+            # Process a creation
             result['changed'] = True
+
             if module._diff:
                 result['diff'] = dict(before='', after=new_auth_repr)
+
             if module.check_mode:
                 module.exit_json(**result)
+
             # If copyFrom is defined, create authentication flow from a copy
             if "copyFrom" in new_auth_repr and new_auth_repr["copyFrom"] is not None:
                 auth_repr = kc.copy_auth_flow(config=new_auth_repr, realm=realm)
             else:  # Create an empty authentication flow
                 auth_repr = kc.create_empty_auth_flow(config=new_auth_repr, realm=realm)
+
             # If the authentication still not exist on the server, raise an exception.
             if auth_repr is None:
                 result['msg'] = "Authentication just created not found: " + str(new_auth_repr)
                 module.fail_json(**result)
+
             # Configure the executions for the flow
             create_or_update_executions(kc=kc, config=new_auth_repr, realm=realm)
+
             # Get executions created
             exec_repr = kc.get_executions_representation(config=new_auth_repr, realm=realm)
             if exec_repr is not None:
                 auth_repr["authenticationExecutions"] = exec_repr
-            result['flow'] = auth_repr
-        elif state == 'absent':  # If desired state is absent.
-            if module._diff:
-                result['diff'] = dict(before='', after='')
-            result['msg'] = new_auth_repr["alias"] + ' absent'
-    else:  # The authentication flow already exist
-        if state == 'present':  # if desired state is present
+            result['end_state'] = auth_repr
+            result['flow'] = result['end_state']
+
+    else:
+        if state == 'present':
+            # Process an update
+
             if force:  # If force option is true
                 # Delete the actual authentication flow
                 result['changed'] = True
@@ -370,25 +463,35 @@ def main():
                     result['msg'] = "Authentication just created not found: " + str(new_auth_repr)
                     module.fail_json(**result)
             # Configure the executions for the flow
+
             if module.check_mode:
                 module.exit_json(**result)
             changed, diff = create_or_update_executions(kc=kc, config=new_auth_repr, realm=realm)
             result['changed'] |= changed
+
             if module._diff:
                 result['diff'] = diff
+
             # Get executions created
             exec_repr = kc.get_executions_representation(config=new_auth_repr, realm=realm)
             if exec_repr is not None:
                 auth_repr["authenticationExecutions"] = exec_repr
-            result['flow'] = auth_repr
-        elif state == 'absent':  # If desired state is absent
+            result['end_state'] = auth_repr
+            result['flow'] = result['end_state']
+
+        else:
+            # Process a deletion (because state was not 'present')
             result['changed'] = True
-            # Delete the authentication flow alias.
+
             if module._diff:
                 result['diff'] = dict(before=auth_repr, after='')
+
             if module.check_mode:
                 module.exit_json(**result)
+
+            # delete it
             kc.delete_authentication_flow_by_id(id=auth_repr["id"], realm=realm)
+
             result['msg'] = 'Authentication flow: {alias} id: {id} is deleted'.format(alias=new_auth_repr['alias'],
                                                                                       id=auth_repr["id"])