New module: Gitlab integration #2129
Conversation
ansibullbot
added
affects_2.10
gitlab
module
| 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 |
ansibullbot
added
community_review
needs_revision
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. |
ansibullbot
added
community_review
and removed
needs_revision
| def mimic_ansible_module_validation(specs, constraints): | ||
| from ansible.module_utils.basic import ModuleArgumentSpecValidator, _load_params | ||
| from ansible.module_utils.errors import UnsupportedError | ||
| import os |
There was a problem hiding this comment.
These imports must be done on the global level, not locally in this function.
(This will also likely show that this does not work with Ansible 2.9 and ansible-base 2.10.)
Also you should not use _load_params. That's an internal API.
Finally, you should not re-validate the whole argument spec, but only the relevant option.
There was a problem hiding this comment.
- Ok about
global level function import
re-validate the whole argument spec, but only the relevant option
But ... this implies, again, more code, more complexity than revalidating it (which does not hurt).
- About
you should not use _load_params, please please, have a look at the its inline documentation:
We cannot guarantee API stability for it (it may change between versions) however we will try not to break it gratuitously. It is certainly more future-proof to call this function and consume its outputs than to implement the logic inside it as a copy in your own code.
=> The programmer who actually wrote this function tells us exactly the contrary : if you need me, then use me.
It feels bad how far fetched this debate about AnsibleModule is leading us.
My intent to do proper validation of arguments, of being strict and DRY ends up in more code, more workarounds, more complication, more maintenance hurdle, more duplication of the private code base because... well the private method is private so lets copy the LoC instead of calling it.
The documentation of _load_params is exactly telling us that : Yes, I'm private but you know, it's better to reuse me than to rewrite me (and it's basically the same about the whole AnsibleModule which wasn't misused at all but just instantiating twice what was never forbidden in the first place ).
I feel we are loosing the big picture and the goal by blindly overfocusing on strict unquestioned "norms" (interpreted for the worst instead of the better) and that I'm pushed into a path that will lead to complex, bloated and inelegant code (that won't be merged either).
There was a problem hiding this comment.
But ... this implies, again, more code, more complexity than revalidating it (which does not hurt).
It does not.
- About
you should not use _load_params, please please, have a look at the its inline documentation:
It's still something that should really not be used in module code. It's ok to use in tests though.
| if error: | ||
| msg = validation_result.errors.msg | ||
| if isinstance(error, UnsupportedError): | ||
| msg = "Unsupported parameters for ({name}) {kind}: {msg}".format(name=os.path.basename(__file__), kind='module', msg=msg) |
There was a problem hiding this comment.
There is no need to use os.path.basename() here.
There was a problem hiding this comment.
Why?
(lib/ansible/module_utils/basic.py does it that way and I stuck to the standard error output)
There was a problem hiding this comment.
Because this is a specific module, not a module utils.
|
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 |
ansibullbot
added
the
needs_rebase
|
Do you still plan to update this PR according to the review comments? needs_info |
ansibullbot
added
the
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. |
ansibullbot
removed
the
needs_info
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 |
ansibullbot
added
the
needs_info
...
I'm happy you changed your mind but how do you suggest to use it differently? |
ansibullbot
removed
the
needs_info
|
I didn't change my mind. Using internal things in production code is still a bad idea. |
ansibullbot
added
needs_ci
|
Is this still going to be worked on? needs_info |
ansibullbot
added
the
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 😆 |
ansibullbot
removed
the
needs_info
|
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.