New module: Gitlab integration #2129
Conversation
| base_spec['params'] = dict(required=False, type='dict', options=sub_arg_specs) | ||
| if '_events' in definitions[service]: | ||
| base_spec['events'] = dict(required=True, type='list', elements='str', choices=definitions[service]['_events']) | ||
| module = AnsibleModule(argument_spec=base_spec, **constraints) |
There was a problem hiding this comment.
I don't think AnsibleModule is meant to be used like this.
There was a problem hiding this comment.
You mean re-instanciating an AnsibleModule a second time in order to further validate parameters based on a first pass (because a complex dict parameter depends upon the value of another)... Then yes, I admit it's indeed exotic (and this is the first plugin to rely on such a construct).
That being said we already discussed the question of parameters (and the benefits of validation) (and so far I couldn't think of any negative side effect for the double AnsibleModule instanciation)
There was a problem hiding this comment.
I really don't think it is a good idea to do this. I also asked on #ansible-devel, and one of the core team members confirmed that this is something that they support. It's basically undefined behavior, and use at your own risk.
There was a problem hiding this comment.
I really don't think it is a good idea to do this. I also asked on #ansible-devel, and one of the core team members confirmed that this is something that they not support. It's basically undefined behavior, and use at your own risk.
Damn, I missed the most important word in this: this is something they do not support. While it works now, it can stop working at a random point in the future.
There was a problem hiding this comment.
Don't you think it's very convenient for the user and that if it were to stop working, we could adapt it quite easily?
The usability benefit are huge (and I think this could be useful to other module if it started to be supported in the future).
Shouldn't we try it? And if not, what would you suggest instead?
There was a problem hiding this comment.
Don't you think it's very convenient for the user
It definitely is! But that can (and should) also be achieved differently than relying on undefined behavior.
and that if it were to stop working, we could adapt it quite easily?
Well, it depends on who will do the work. Such cleanup work often falls back to collection maintainers because the people who originally implemented it are no longer around to clean it up.
The usability benefit are huge (and I think this could be useful to other module if it started to be supported in the future).
The benefits of extra validation are indeed great, but doing that by instantiating AnsibleModule twice is IMO the wrong solution for that problem.
And if not, what would you suggest instead?
Implement the validation manually, preferably as a module_utils so it can be used by other modules as well. Useful parts for that already exist in Ansible(-base/-core)'s module_utils, but unfortunately they change a lot between the Ansible versions (2.9, 2.10, 2.11) and thus most of them cannot be used. What can be used are the things in https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/validation.py that were already present in https://github.com/ansible/ansible/blob/stable-2.9/lib/ansible/module_utils/common/validation.py, which covers all check_type_xxx functions, and check_required_arguments.
|
In 372f6c5 I tried to add some tests. |
tests/unit/plugins/modules/source_control/gitlab/test_gitlab_service.py
Outdated
Show resolved
Hide resolved
|
Some months ago, GitLab |
Just wait a couple of more months until it is renamed something else again, and only then change it. Just joking ;) I guess it's better to update the module now before it has been released, since renaming the module, options, return values etc. later is a huge PITA, and having a different name than it is used in GitLab itself is also confusing to users. |
|
Done. |
|
This New Module PR contains a symbolic link from plugins/modules/ to the actual Python file. Since #4562 this is no longer necessary and will soon be flagged as an error. Instead you need to add an entry to meta/runtime.yml, similar to this one: https://github.com/ansible-collections/community.general/blob/main/meta/runtime.yml#L21-L22 See also our updated instructions on creating new modules: https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins |
| @@ -0,0 +1,2 @@ | |||
| shippable/posix/group1 | |||
There was a problem hiding this comment.
| shippable/posix/group1 | |
| azp/posix/1 |
(see #5344)
|
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 adds new files into this hierarchy. Please rebase with the current |
|
Do you still plan to update this PR according to the review comments? needs_info |
Sorry but I still disagree with the reviewers suggestion of reimplementing, badly, the I'm using a version of this module locally on a regular basis but I won't produce yet another PR containing bad/inefficient/unmaintainable/inelegant code... without a technical reason being given. Although I did the initial PR Jul 17th, 2020, the future of this module's [possible inclusion] is definitely not in my hands. Good luck to you and/or to other (still) motivated Ansible contributors. |
You can reuse it, but not in the way you are doing it right now. If you don't want to change the code, you will have to find someone else who can get your module merged. I don't merge things that are simply wrong from my point of view, sorry. |
|
needs_info |
... I'm happy you changed your mind but how do you suggest to use it differently? |
|
I didn't change my mind. Using internal things in production code is still a bad idea. |
|
Is this still going to be worked on? needs_info |
|
@drzraf This pullrequest is waiting for your response. Please respond or the pullrequest will be closed. |
|
4.5 years without a (useful) module being upstreamed... So much of a symbol 😆 |
|
Hi @drzraf are you still going to work on this? If not, could you please close this PR? |
SUMMARY
New module: Gitlab integration services
Fixes ansible/ansible#40053
Reroll of #667
ISSUE TYPE
COMPONENT NAME
community.general.gitlab_integration
ADDITIONAL INFORMATION
This module provides a way to setup Gitlab integration services using Ansible and supports all services defined by Gitlab.com API (and their corresponding parameters) as of
2020/07/092022/02/17.Known bugs:
state=absentto completely remove the service configuration instead of deactivating it. But some work happened recently on GitLab side, it may be fixed by now.EXTENDED DEFINITION OF SERVICE
AnsibleModule()and modifying the argument spec meanwhile.- The extended definition of services and their suboptions (name, required or not, type) is generated from Gitlab upstream file the 2021/03/29.