Skip to content

Add aux_tags suboption for tags in tag_group module #752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
May 12, 2025

Conversation

meni2029
Copy link
Contributor

@meni2029 meni2029 commented Apr 3, 2025

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

The aux_tags suboption for the tags in the tag_group module is not available, while it was possible to declare it in previous collection version (i.e. 3.4.0).

What is the new behavior?

  • Added the aux_tags suboption for the tags in the tag_group module
  • The suboption is optional and default value is an empty list ([])
  • The aux_tags is now used in the changes_detected function

Other information

It has been tested on Checkmk 2.3.0p27. It might not fully work with 2.2 as, if I remember correctly, the API returns no "aux_tags" item if there's none selected. At least in 2.3, the API always returns it with empty list if none selected, so default => []:

"topic": "Notifications",
    "tags": [
        {
            "id": "yes",
            "title": "of course",
            "aux_tags": []
        }

@github-actions github-actions bot added the module:tag_group This affects the tag_group module label Apr 3, 2025
@robin-checkmk robin-checkmk self-assigned this Apr 3, 2025
@robin-checkmk
Copy link
Member

Hi @meni2029 and thank you for your contribution! Looks good to me so far.
Do you think you can add one or two integration tests, which verify that the feature works as intended?
That would be highly appreciated! If not, I can take care of it as well.

@meni2029
Copy link
Contributor Author

Hi @robin-checkmk
Thanks for your reply. I will have a look at the integration tests.

@meni2029
Copy link
Contributor Author

Hi @robin-checkmk ,
I added integration tests. For the tests I create the aux_tags with direct API call, as there's no module for aux_tags.
Hopefully this is ok.
Cheers

@robin-checkmk
Copy link
Member

Hi @meni2029 and sorry for the delay. I have been brooding over for a few days now, so let me share my thoughts.
The feature itself looks fine to me. The issue I am struggling with is this: In the integration tests, you use a direct API all, as the module cannot create aux tags. This does not feel right.
Now, I do not want to burden you with either integrating this functionality into the tag_group module, or build a dedicated one. But I feel like that would be the right thing to do.
What do you think? As this works, it is fine to merge, but I am pondering the idea to queue this, until we can also manage aux tags. I am uncertain though. What do you thing?

@lgetwan
Copy link
Contributor

lgetwan commented May 8, 2025

Hi @meni2029,
on first sight, that looks like an easy to solve feature update. But, as often, writing the tests takes quite a bit of time. ;-)
Thanks for investing that time!

Regarding the add/update/delete auxtags feature, I'd prefer a separate module, as it is a completely separate group of API endpoints.
But the feature you added already might help users even without that separate module, so I'd recommend merging this PR even before that module exists.

@meni2029
Copy link
Contributor Author

meni2029 commented May 9, 2025

Hi @robin-checkmk , @lgetwan
Thanks for looking into that.
Regarding the integration tests using direct API calls, to me it is not much of an issue, as it is for the tests, not inside the actual module.
Of course it would be better to have a module for aux_tags, but not easy to create it. One challenge is the dependency between tag_group and aux_tags, when creating and also supressing.
@muehlings made a proposition a year ago: #542. I don't really have the time to dig into it.
On our side at the moment we handle aux_tags with direct API calls, and use tag_group module.
I think it makes sense to do this small step forward and merge this MR ;)

@robin-checkmk
Copy link
Member

Alright, points taken. Thanks for discussing this folks!
I might add a known_issues section to the changelog to clarify, that this change enables managing assignments of existing aux_tags to a tag_group but not management of said aux_tags, but I think then we are good to merge.

Thanks again!

@robin-checkmk robin-checkmk merged commit 6df3eca into Checkmk:devel May 12, 2025
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
module:tag_group This affects the tag_group module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants