Rework of gitlab_project_variable over gitlab_group_variable (#4086)

* Rework of gitlab_project_variable over gitlab_group_variable

* Linting and removed unused example

* Fix test 2

* Sync from review of gitlab_project_variable #4038

* Linting, default protected True, value optional

* Next version is 4.5.0

* Roll back protected default true, and value not required

* Apply suggestions from code review

Missing check_mode

Co-authored-by: Markus Bergholz <git@osuv.de>

* Fix one unit test, comment test that requires premium gitlab

* Add changelog

* Update plugins/modules/source_control/gitlab/gitlab_group_variable.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/4086-rework_of_gitlab_proyect_variable_over_gitlab_group_variable.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Added conditional gitlab_premium_tests variable when required

* Allow delete without value

* Fix variable name

* Linting

* Value should not be required in doc

* Linting missing new-line

* Update changelogs/fragments/4086-rework_of_gitlab_proyect_variable_over_gitlab_group_variable.yml

Co-authored-by: Markus Bergholz <git@osuv.de>

Co-authored-by: Markus Bergholz <git@osuv.de>
Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/4086-rework_of_gitlab_proyect_variable_over_gitlab_group_variable.yml

@@ -0,0 +1,9 @@
+bugfixes:
+  - >
+    gitlab_group_variable - allow to set same variable name under different environment scopes.
+    Due this change, the return value ``group_variable`` differs from previous version in check mode.
+    It was counting ``updated`` values, because it was accidentally overwriting environment scopes (https://github.com/ansible-collections/community.general/pull/4038).
+  - gitlab_group_variable - fix idempotent change behaviour for float and integer variables (https://github.com/ansible-collections/community.general/pull/4038).
+  - gitlab_group_variable - add missing documentation about GitLab versions that support ``environment_scope`` and ``variable_type`` (https://github.com/ansible-collections/community.general/pull/4038).
+minor_changes:
+  - gitlab_group_variable - new ``variables`` parameter (https://github.com/ansible-collections/community.general/pull/4038 and https://github.com/ansible-collections/community.general/issues/4074).

plugins/modules/source_control/gitlab/gitlab_group_variable.py

@@ -48,6 +48,8 @@ options:
       - When the list element is a simple key-value pair, set masked and protected to false.
       - When the list element is a dict with the keys I(value), I(masked) and I(protected), the user can
         have full control about whether a value should be masked, protected or both.
+      - Support for group variables requires GitLab >= 9.5.
+      - Support for environment_scope requires GitLab Premium >= 13.11.
       - Support for protected values requires GitLab >= 9.3.
       - Support for masked values requires GitLab >= 11.10.
       - A I(value) must be a string or a number.
@@ -56,6 +58,46 @@ options:
         See GitLab documentation on acceptable values for a masked variable (U(https://docs.gitlab.com/ce/ci/variables/#masked-variables)).
     default: {}
     type: dict
+  variables:
+    version_added: 4.5.0
+    description:
+      - A list of dictionaries that represents CI/CD variables.
+      - This modules works internal with this sructure, even if the older I(vars) parameter is used.
+    default: []
+    type: list
+    elements: dict
+    suboptions:
+      name:
+        description:
+          - The name of the variable.
+        type: str
+        required: true
+      value:
+        description:
+          - The variable value.
+          - Required when I(state=present).
+        type: str
+      masked:
+        description:
+          - Wether variable value is masked or not.
+        type: bool
+        default: false
+      protected:
+        description:
+          - Wether variable value is protected or not.
+        type: bool
+        default: false
+      variable_type:
+        description:
+          - Wether a variable is an environment variable (C(env_var)) or a file (C(file)).
+        type: str
+        choices: [ "env_var", "file" ]
+        default: env_var
+      environment_scope:
+        description:
+          - The scope for the variable.
+        type: str
+        default: '*'
 notes:
 - Supports I(check_mode).
 '''
@@ -68,23 +110,15 @@ EXAMPLES = r'''
     api_token: secret_access_token
     group: scodeman/testgroup/
     purge: false
-    vars:
-      ACCESS_KEY_ID: abc123
-      SECRET_ACCESS_KEY: 321cba
-
-- name: Set or update some CI/CD variables
-  community.general.gitlab_group_variable:
-    api_url: https://gitlab.com
-    api_token: secret_access_token
-    group: scodeman/testgroup/
-    purge: false
-    vars:
-      ACCESS_KEY_ID: abc123
-      SECRET_ACCESS_KEY:
+    variables:
+      - name: ACCESS_KEY_ID
+        value: abc123
+      - name: SECRET_ACCESS_KEY
         value: 3214cbad
         masked: true
         protected: true
         variable_type: env_var
+        environment_scope: production
 
 - name: Delete one variable
   community.general.gitlab_group_variable:
@@ -125,13 +159,11 @@ group_variable:
 '''
 
 import traceback
-
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
 from ansible.module_utils.api import basic_auth_argument_spec
 from ansible.module_utils.six import string_types
 from ansible.module_utils.six import integer_types
 
-
 GITLAB_IMP_ERR = None
 try:
     import gitlab
@@ -143,6 +175,44 @@ except Exception:
 from ansible_collections.community.general.plugins.module_utils.gitlab import auth_argument_spec, gitlab_authentication
 
 
+def vars_to_variables(vars, module):
+    # transform old vars to new variables structure
+    variables = list()
+    for item, value in vars.items():
+        if (isinstance(value, string_types) or
+           isinstance(value, (integer_types, float))):
+            variables.append(
+                {
+                    "name": item,
+                    "value": str(value),
+                    "masked": False,
+                    "protected": False,
+                    "variable_type": "env_var",
+                }
+            )
+
+        elif isinstance(value, dict):
+            new_item = {"name": item, "value": value.get('value')}
+
+            new_item = {
+                "name": item,
+                "value": value.get('value'),
+                "masked": value.get('masked'),
+                "protected": value.get('protected'),
+                "variable_type": value.get('variable_type'),
+            }
+
+            if value.get('environment_scope'):
+                new_item['environment_scope'] = value.get('environment_scope')
+
+            variables.append(new_item)
+
+        else:
+            module.fail_json(msg="value must be of type string, integer, float or dict")
+
+    return variables
+
+
 class GitlabGroupVariables(object):
 
     def __init__(self, module, gitlab_instance):
@@ -163,103 +233,150 @@ class GitlabGroupVariables(object):
             vars_page = self.group.variables.list(page=page_nb)
         return variables
 
-    def create_variable(self, key, value, masked, protected, variable_type):
-        if self._module.check_mode:
-            return
-        return self.group.variables.create({
-            "key": key,
-            "value": value,
-            "masked": masked,
-            "protected": protected,
-            "variable_type": variable_type,
-        })
-
-    def update_variable(self, key, var, value, masked, protected, variable_type):
-        if var.value == value and var.protected == protected and var.masked == masked and var.variable_type == variable_type:
-            return False
-
+    def create_variable(self, var_obj):
         if self._module.check_mode:
             return True
+        var = {
+            "key": var_obj.get('key'),
+            "value": var_obj.get('value'),
+            "masked": var_obj.get('masked'),
+            "protected": var_obj.get('protected'),
+            "variable_type": var_obj.get('variable_type'),
+        }
+        if var_obj.get('environment_scope') is not None:
+            var["environment_scope"] = var_obj.get('environment_scope')
 
-        if var.protected == protected and var.masked == masked and var.variable_type == variable_type:
-            var.value = value
-            var.save()
-            return True
-
-        self.delete_variable(key)
-        self.create_variable(key, value, masked, protected, variable_type)
+        self.group.variables.create(var)
         return True
 
-    def delete_variable(self, key):
+    def update_variable(self, var_obj):
         if self._module.check_mode:
-            return
-        return self.group.variables.delete(key)
+            return True
+        self.delete_variable(var_obj)
+        self.create_variable(var_obj)
+        return True
+
+    def delete_variable(self, var_obj):
+        if self._module.check_mode:
+            return True
+        self.group.variables.delete(var_obj.get('key'), filter={'environment_scope': var_obj.get('environment_scope')})
+        return True
 
 
-def native_python_main(this_gitlab, purge, var_list, state, module):
+def compare(requested_variables, existing_variables, state):
+    # we need to do this, because it was determined in a previous version - more or less buggy
+    # basically it is not necessary and might results in more/other bugs!
+    # but it is required  and only relevant for check mode!!
+    # logic represents state 'present' when not purge. all other can be derived from that
+    # untouched => equal in both
+    # updated => name and scope are equal
+    # added => name and scope does not exist
+    untouched = list()
+    updated = list()
+    added = list()
+
+    if state == 'present':
+        existing_key_scope_vars = list()
+        for item in existing_variables:
+            existing_key_scope_vars.append({'key': item.get('key'), 'environment_scope': item.get('environment_scope')})
+
+        for var in requested_variables:
+            if var in existing_variables:
+                untouched.append(var)
+            else:
+                compare_item = {'key': var.get('name'), 'environment_scope': var.get('environment_scope')}
+                if compare_item in existing_key_scope_vars:
+                    updated.append(var)
+                else:
+                    added.append(var)
+
+    return untouched, updated, added
+
+
+def native_python_main(this_gitlab, purge, requested_variables, state, module):
 
     change = False
     return_value = dict(added=list(), updated=list(), removed=list(), untouched=list())
 
     gitlab_keys = this_gitlab.list_all_group_variables()
-    existing_variables = [x.get_id() for x in gitlab_keys]
+    before = [x.attributes for x in gitlab_keys]
 
-    for key in var_list:
-        if not isinstance(var_list[key], (string_types, integer_types, float, dict)):
-            module.fail_json(msg="Value of %s variable must be of type string, integer, float or dict, passed %s" % (key, var_list[key].__class__.__name__))
+    gitlab_keys = this_gitlab.list_all_group_variables()
+    existing_variables = [x.attributes for x in gitlab_keys]
 
-    for key in var_list:
+    # preprocessing:filter out and enrich before compare
+    for item in existing_variables:
+        item.pop('group_id')
 
-        if isinstance(var_list[key], (string_types, integer_types, float)):
-            value = var_list[key]
-            masked = False
-            protected = False
-            variable_type = 'env_var'
-        elif isinstance(var_list[key], dict):
-            value = var_list[key].get('value')
-            masked = var_list[key].get('masked', False)
-            protected = var_list[key].get('protected', False)
-            variable_type = var_list[key].get('variable_type', 'env_var')
+    for item in requested_variables:
+        item['key'] = item.pop('name')
+        item['value'] = str(item.get('value'))
+        if item.get('protected') is None:
+            item['protected'] = False
+        if item.get('masked') is None:
+            item['masked'] = False
+        if item.get('environment_scope') is None:
+            item['environment_scope'] = '*'
+        if item.get('variable_type') is None:
+            item['variable_type'] = 'env_var'
 
-        if key in existing_variables:
-            index = existing_variables.index(key)
-            existing_variables[index] = None
+    if module.check_mode:
+        untouched, updated, added = compare(requested_variables, existing_variables, state)
 
-            if state == 'present':
-                single_change = this_gitlab.update_variable(
-                    key,
-                    gitlab_keys[index],
-                    value,
-                    masked,
-                    protected,
-                    variable_type,
-                )
-                change = single_change or change
-                if single_change:
-                    return_value['updated'].append(key)
-                else:
-                    return_value['untouched'].append(key)
+    if state == 'present':
+        add_or_update = [x for x in requested_variables if x not in existing_variables]
+        for item in add_or_update:
+            try:
+                if this_gitlab.create_variable(item):
+                    return_value['added'].append(item)
 
-            elif state == 'absent':
-                this_gitlab.delete_variable(key)
-                change = True
-                return_value['removed'].append(key)
+            except Exception:
+                if this_gitlab.update_variable(item):
+                    return_value['updated'].append(item)
 
-        elif key not in existing_variables and state == 'present':
-            this_gitlab.create_variable(key, value, masked, protected, variable_type)
-            change = True
-            return_value['added'].append(key)
+        if purge:
+            # refetch and filter
+            gitlab_keys = this_gitlab.list_all_group_variables()
+            existing_variables = [x.attributes for x in gitlab_keys]
+            for item in existing_variables:
+                item.pop('group_id')
 
-    existing_variables = list(filter(None, existing_variables))
-    if purge:
+            remove = [x for x in existing_variables if x not in requested_variables]
+            for item in remove:
+                if this_gitlab.delete_variable(item):
+                    return_value['removed'].append(item)
+
+    elif state == 'absent':
+        # value does not matter on removing variables.
+        # key and environment scope are sufficient
         for item in existing_variables:
-            this_gitlab.delete_variable(item)
-            change = True
-            return_value['removed'].append(item)
-    else:
-        return_value['untouched'].extend(existing_variables)
+            item.pop('value')
+            item.pop('variable_type')
+        for item in requested_variables:
+            item.pop('value')
+            item.pop('variable_type')
 
-    return change, return_value
+        if not purge:
+            remove_requested = [x for x in requested_variables if x in existing_variables]
+            for item in remove_requested:
+                if this_gitlab.delete_variable(item):
+                    return_value['removed'].append(item)
+
+        else:
+            for item in existing_variables:
+                if this_gitlab.delete_variable(item):
+                    return_value['removed'].append(item)
+
+    if module.check_mode:
+        return_value = dict(added=added, updated=updated, removed=return_value['removed'], untouched=untouched)
+
+    if len(return_value['added'] + return_value['removed'] + return_value['updated']) > 0:
+        change = True
+
+    gitlab_keys = this_gitlab.list_all_group_variables()
+    after = [x.attributes for x in gitlab_keys]
+
+    return change, return_value, before, after
 
 
 def main():
@@ -269,7 +386,15 @@ def main():
         group=dict(type='str', required=True),
         purge=dict(type='bool', required=False, default=False),
         vars=dict(type='dict', required=False, default=dict(), no_log=True),
-        state=dict(type='str', default="present", choices=["absent", "present"])
+        variables=dict(type='list', elements='dict', required=False, default=list(), options=dict(
+            name=dict(type='str', required=True),
+            value=dict(type='str', no_log=True),
+            masked=dict(type='bool', default=False),
+            protected=dict(type='bool', default=False),
+            environment_scope=dict(type='str', default='*'),
+            variable_type=dict(type='str', default='env_var', choices=["env_var", "file"])
+        )),
+        state=dict(type='str', default="present", choices=["absent", "present"]),
     )
 
     module = AnsibleModule(
@@ -280,6 +405,7 @@ def main():
             ['api_username', 'api_job_token'],
             ['api_token', 'api_oauth_token'],
             ['api_token', 'api_job_token'],
+            ['vars', 'variables'],
         ],
         required_together=[
             ['api_username', 'api_password'],
@@ -290,18 +416,46 @@ def main():
         supports_check_mode=True
     )
 
+    if not HAS_GITLAB_PACKAGE:
+        module.fail_json(msg=missing_required_lib("python-gitlab"), exception=GITLAB_IMP_ERR)
+
     purge = module.params['purge']
     var_list = module.params['vars']
     state = module.params['state']
 
-    if not HAS_GITLAB_PACKAGE:
-        module.fail_json(msg=missing_required_lib("python-gitlab"), exception=GITLAB_IMP_ERR)
+    if var_list:
+        variables = vars_to_variables(var_list, module)
+    else:
+        variables = module.params['variables']
+
+    if state == 'present':
+        if any(x['value'] is None for x in variables):
+            module.fail_json(msg='value parameter is required in state present')
 
     gitlab_instance = gitlab_authentication(module)
 
     this_gitlab = GitlabGroupVariables(module=module, gitlab_instance=gitlab_instance)
 
-    changed, return_value = native_python_main(this_gitlab, purge, var_list, state, module)
+    changed, raw_return_value, before, after = native_python_main(this_gitlab, purge, variables, state, module)
+
+    # postprocessing
+    for item in after:
+        item.pop('group_id')
+        item['name'] = item.pop('key')
+    for item in before:
+        item.pop('group_id')
+        item['name'] = item.pop('key')
+
+    untouched_key_name = 'key'
+    if not module.check_mode:
+        untouched_key_name = 'name'
+        raw_return_value['untouched'] = [x for x in before if x in after]
+
+    added = [x.get('key') for x in raw_return_value['added']]
+    updated = [x.get('key') for x in raw_return_value['updated']]
+    removed = [x.get('key') for x in raw_return_value['removed']]
+    untouched = [x.get(untouched_key_name) for x in raw_return_value['untouched']]
+    return_value = dict(added=added, updated=updated, removed=removed, untouched=untouched)
 
     module.exit_json(changed=changed, group_variable=return_value)
 

tests/integration/targets/gitlab_group_variable/tasks/main.yml

@@ -36,8 +36,9 @@
     api_url: "{{ gitlab_host }}"
     api_token: "{{ gitlab_login_token }}"
     group: "{{ gitlab_group_name }}"
-    vars:
-      ACCESS_KEY_ID: checkmode
+    variables:
+      - name: ACCESS_KEY_ID
+        value: checkmode
   register: gitlab_group_variable_state
 
 - name: state must be changed
@@ -219,6 +220,42 @@
     that:
       - gitlab_group_variable_state is not changed
 
+- name: change environment scope
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    vars:
+      ACCESS_KEY_ID:
+        environment_scope: testing
+        value: checkmode
+  register: gitlab_group_variable_state
+  when: gitlab_premium_tests is defined
+
+- name: state must be changed
+  assert:
+    that:
+      - gitlab_group_variable_state is changed
+  when: gitlab_premium_tests is defined
+
+- name: apply again the environment scope change 
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    vars:
+      ACCESS_KEY_ID:
+        environment_scope: testing
+        value: checkmode
+  register: gitlab_group_variable_state
+  when: gitlab_premium_tests is defined
+
+- name: state must not be changed
+  assert:
+    that:
+      - gitlab_group_variable_state is not changed
+  when: gitlab_premium_tests is defined
+
 - name: purge all variables at the beginning
   gitlab_group_variable:
     api_url: "{{ gitlab_host }}"
@@ -426,8 +463,8 @@
     api_url: "{{ gitlab_host }}"
     api_token: "{{ gitlab_login_token }}"
     group: "{{ gitlab_group_name }}"
-    vars:
-      my_test_var:
+    variables:
+      - name: my_test_var
         value: my_test_value
         variable_type: file
     purge: False
@@ -583,3 +620,84 @@
       - gitlab_group_variable_state.group_variable.untouched|length == 0
       - gitlab_group_variable_state.group_variable.removed|length == 40
       - gitlab_group_variable_state.group_variable.updated|length == 0
+
+- name: same vars, diff scope
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    purge: True
+    variables:
+      - name: SECRET_ACCESS_KEY
+        value: 3214cbad
+        masked: true
+        protected: true
+        variable_type: env_var
+        environment_scope: production
+      - name: SECRET_ACCESS_KEY
+        value: hello_world
+        masked: true
+        protected: true
+        variable_type: env_var
+        environment_scope: development
+  register: gitlab_group_variable_state
+  when: gitlab_premium_tests is defined
+
+- name: verify two vars
+  assert:
+    that:
+      - gitlab_group_variable_state.changed
+      - gitlab_group_variable_state.group_variable.added|length == 2
+      - gitlab_group_variable_state.group_variable.untouched|length == 0
+      - gitlab_group_variable_state.group_variable.removed|length == 0
+      - gitlab_group_variable_state.group_variable.updated|length == 0
+  when: gitlab_premium_tests is defined
+
+- name: throw error when state is present but no value is given
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    variables:
+      - name: delete_me
+  register: gitlab_group_variable_state
+  ignore_errors: yes
+
+- name: verify fail
+  assert:
+    that:
+      - gitlab_group_variable_state.failed
+      - gitlab_group_variable_state is not changed
+
+- name: set a new variable to delete it later
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    purge: True
+    variables:
+      - name: delete_me
+        value: ansible
+  register: gitlab_group_variable_state
+
+- name: verify the change
+  assert:
+    that:
+      - gitlab_group_variable_state.changed
+
+- name: delete variable without referencing its value
+  gitlab_group_variable:
+    api_url: "{{ gitlab_host }}"
+    api_token: "{{ gitlab_login_token }}"
+    group: "{{ gitlab_group_name }}"
+    state: absent
+    variables:
+      - name: delete_me
+  register: gitlab_group_variable_state
+
+- name: verify deletion
+  assert:
+    that:
+      - gitlab_group_variable_state.changed
+      - gitlab_group_variable_state.group_variable.removed|length == 1
+