gitlab_group: fix issue 4577

[fzarifian]

SUMMARY

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

Fix https://github.com/ansible-collections/community.general/issues/4577

ADDITIONAL INFORMATION

[ansibullbot]

cc @Lunik @Shaps @dj-wasabi @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa click here for bot help

[Lunik]

If we can't set à default value but the API expect those values, we should mark them as required

[Lunik]

Can also fix #4577

[felixfontein]

You can also only require it on creation (and fail with a nicer error message), or dynamically pass a default on creation if no value is passed to the module.

[fzarifian]

@felixfontein the value is not mandatory, but passing the argument as null fails

[felixfontein]

Then forcing a default when one isn't needed is really a wrong fix. The correct fix would be to prevent null being passed when no value is specified.

[fzarifian]

    def create_or_update_group(self, name, parent, options):
        changed = False

        payload = {
            'name': name,
            'path': options['path'],
            'visibility': options['visibility'],
            'project_creation_level': options['project_creation_level'],
            'auto_devops_enabled': options['auto_devops_enabled'],
        }
        if options.get('project_creation_level'):
            payload['project_creation_level'] = options['project_creation_level']
        if options.get('subgroup_creation_level'):
            payload['subgroup_creation_level'] = options['subgroup_creation_level']
        if options.get('description'):
            payload['description'] = options['description']
        if options.get('require_two_factor_authentication'):
            payload['require_two_factor_authentication'] = options['require_two_factor_authentication']

        # Because we have already call userExists in main()
        if self.group_object is None:
            parent_id = self.get_group_id(parent)
            payload['parent_id'] = parent_id
            group = self.create_group(payload)

            # add avatar to group
            if options['avatar_path']:
                try:
                    group.avatar = open(options['avatar_path'], 'rb')
                except IOError as e:
                    self._module.fail_json(msg='Cannot open {0}: {1}'.format(options['avatar_path'], e))
            changed = True
        else:
            changed, group = self.update_group(self.group_object, payload)

        self.group_object = group
        if changed:
            if self._module.check_mode:
                self._module.exit_json(changed=True, msg="Successfully created or updated the group %s" % name)

            try:
                group.save()
            except Exception as e:
                self._module.fail_json(msg="Failed to update group: %s " % e)
            return True
        else:
            return False

[fzarifian]

Some typos in my payload, remaining 'subgroup_creation_level': options['subgroup_creation_level'], after putting it in optional : works great :)

This one resolve :

• the broken creation
• the idempotency for those optionnal values

After that, looots of questions about the avatar implemention, but that's not the PR subject...

[fzarifian]

the implementation of the check_mode is also interresting, but not the PR subject too

[fzarifian]

In reality, only two parameters are mandatory in gitlab implementation.

• name

• path (slugified name, which should be a method defined in module_utils, and not the code used because it does not handle the lowercase needed in slugify, that’s related to the think you can see in the main method ; I agree that the payload of the argspec should be used in the whole module rather than this implementation with string mapping 3 times in this module); note

• all others are not mandatory.

https://docs.gitlab.com/ee/api/groups.html#new-group

I think this is more than this PR to review the whole code and I don’t have time to do it for now, feel free to assign me the change request to refactor it

To close this one, I don’t undestand the changelog fragment very well, I need to read the documentation you put. Will come back with new submission when it’s ok for me.

— Fabien ZARIFIAN

Le 20 juin 2022 à 07:51, Felix Fontein @.***> a écrit :

 @felixfontein commented on this pull request.

Besides that, looks good. (Still needs a changelog fragment though.)

In plugins/modules/source_control/gitlab/gitlab_group.py:

@@ -197,23 +197,27 @@ def get_group_id(self, group): def create_or_update_group(self, name, parent, options): changed = False

•    # Same payload for update or create, only parent_id is needed on creation
  

•    payload = {
  

•        'name': name,
  

•        'path': options['path'],
  

So far path was only passed on creation, I would handle it the same as parent_id.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[eifelmicha]

@fzarifian Just stumbled across this. Before switching over to curl requests. What's the status of this fix?

[felixfontein]

Please note that in #5461 the collection repository was restructured to remove the directory tree in plugins/modules/, and the corresponding tree in tests/unit/plugins/modules/. Your PR modifies files in this directory structure, and unfortunately now has some conflicts, potentially because of this. Please rebase with the current main branch to remove the conflicts.

[felixfontein]

needs_info

[ansibullbot]

@fzarifian This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

[ansibullbot]

@fzarifian You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.

click here for bot help

[github-actions[bot]]

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.