[FEED] Close similarity between modules

[robin-checkmk]

Hi @robin-tribe29,

somehow this close similarity of contact and host groups does not give me a rest. I have also checked and service group rest API interface is also quite similar. I do not know, whether this similarity will also stay in the future, but in case, I have a group module written that combines all this three group management in one (including test suite).

https://github.com/msekania/ansible-collection-tribe29.checkmk/tree/feature-groups

In current version, there is a group_type field which takes one of ["contact", "host", "service"] values, and the rest is provided either by group_name (instead of host_group_name, contact_group_name) and title or list of dict-s named 'groups' (instead of host_group, contact_group) with name and title entries. I can also add a backward compatibility features.

Alternatively, I can rewrite the module so that group_type field is removed and choice is made by providing:

• host_group_name or host_groups;
• contact_group_name or contact_groups;
• service_group_name or service_groups;

Should I make an another pull request? We can decide then how to proceed.

Best, Michael

Originally posted by @msekania in https://github.com/tribe29/ansible-collection-tribe29.checkmk/issues/168#issuecomment-1274386559

[lgetwan]

I don't think, that a similarity regarding the code justifies merging those three things together, because the actual items that are created, are of different kinds. And it wouldn't simplify the usage of the Ansible module from my point of view. What we could do (and should do in the future) is creating a file utils.py that contains all the code that is duplicate across the modules. That would make it easier to add new modules.

[msekania]

Hi Lars,

And it wouldn't simplify the usage of the Ansible module from my point of view.

I agree that it does not simplify Ansible part. It might also get problematic if REST API gets more complex and different for each group. With separate modules one can develop each part then separately.

What we could do (and should do in the future) is creating a file utils.py that contains all the code that is duplicate across the modules. That would make it easier to add new modules.

It's definitely a good idea, because there are several functions that repeat cross the modules not only along the groups.

What one can do for the moment, one can also add service groups module together with contact_groups or as a separate pull request. As I have already mentioned in comment for contact_groups, creation of the service module and test again reduces to copying of the module host_group.py to service_group.py file and test directory and running twice sed command on each file.

Best, Michael

[robin-checkmk]

@msekania please go for a dedicated PR this time, so we have a clean process. In this issue we will discuss how to move forward with the shared code.

[muehlings]

[...] What we could do (and should do in the future) is creating a file utils.py that contains all the code that is duplicate across the modules. That would make it easier to add new modules.

That's the approach I prefer across my other tools I am writing for Checkmk. Could be the right place to handle all API return codes and ansible messages (failed/changed/ok), too. I liked the http_code_mapping method when I first saw it, but did not invest the time to look everything up for my module.

[msekania]

http_code_mapping method

Is indeed a good feature! Definitely worth to adjust all the modules to it at some point.

I also agree that messaging should go to utils.py, which will simplify maintenance of standard messages for all modules.

Concerning the currently similar parts in contact, host, and service groups:

The question remains though, should one unify internal subroutines (current complexity of REST API allows this) and if yes should one deposit them in utils.py, or in yet another dedicated file. Anyway, internal structure one can adjust relatively easy without changing the way how the user uses these modules.

[robin-checkmk]

Thanks for your input guys. I am glad we agree on this in general. We did not find the time yet to understand how this endeavor would be performed properly, because there is probably best practices from the Ansible project, which we want to follow as well as some Checkmk-specific stuff.

If any of you feels like they have a good grip on this, feel free to propose a PR, and we can continue discussing there. Other than that, we have this on our agenda, and will update here, as soon as something happens.

[github-actions[bot]]

This issue has been stale for 60 days. It will close in 7 days.

[github-actions[bot]]

This issue has been stale for 60 days. It will close in 7 days.

[godspeed-you]

I support the approach (thanks @msekania vor linking this issued to this PR) to make use of the module_utils library. In my PR I tried to make first step by implementing the API and a first example as a poc. Originally, I also had type_defs in this PR but eventually decided to keep the change as small as possible. I would like to make other proposals as soon as we agreed on a general approach. I also have to admit, that I have not been aware of this issue and wanted to change the code of someone familiar. It's sometimes considered as very Rüde to massively change the code of others with the first contribution.

The structure would then be: api.py type_defs.py utils.py

[godspeed-you]

My proposal for the structure:

module_utils/
    utils/
        common.py
        http_codes.py
        ...
    types.py
modules/
    activation.py
    ...

An alternative could be to not make the utils over complex and just have a single file:

module_utils/
    utils.py
    types.py
modules/
    activation.py
    ...

At the moment, I have no feeling about the need for utils for the other modules. On the other side: It's not a big move, to change from utils.py to a utils folder afterward.

[lgetwan]

That sounds reasonable! So, let's start with a single utils.py file.

[github-actions[bot]]

This issue has been stale for 60 days. It will close in 7 days.