Migrate Rule module to new API

[lgetwan]

I migrated the rule module to the new API and made it a bit nicer.
Will update this description soon. Just wanted to save my work before the weekend. ;-)

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?

Rule module is a bit ugly and only ~23% idempotent.

What is the new behavior?

Rule module

• is nicer,
• uses the new API,
• is ~87% idempotent.
• The rules lookup module accepts filtering for folders, now.

Other information

100% idempotency will be reached, once the value_raw is handled differently by Checkmk and its REST API.

[lgetwan]

@msekania: You already spent a lot of time diving into the rule module and its flaws. Can you have a look at my completely rewritten rule module and test it, if you have some time?

[msekania]

@lgetwan, yes I'll do

@msekania: You already spent a lot of time diving into the rule module and its flaws. Can you have a look at my completely rewritten rule module and test it, if you have some time?

[msekania]

@lgetwan,

I've gone through the code and have more than a few suggestions for improvement and cleanup. I'll do the code review comments right after.

As for the rest, overwriting, it's good overall, but I don't like that the functionality has been reduced compared to the existing version. Namely, the check whether a rule already exists no longer takes place if rule_id is not specified. I understand that the existing version was not perfect in this regard, but it was still useful and all my use cases relied on this functionality. Currently there is also no way to compensate the "removed" functionality with the lookup rule plugin.

I am currently working to bring that functionality back, if you do not mind.

[msekania]

I cannot make any changes to this branch otherwise I could apply the changes directly or via merge request

[msekania]

I think, this should read

        desired_location = desired.get("location")

[lgetwan]

I don't think so. "location" is a parameter inside of the rule, not a top level one.

[lgetwan]

Hi @msekania

Thanks a lot for your comprehensive review!
I need some time to reply to the single findings though, as my calendar is currently packed with customer calls.

As for the rest, overwriting, it's good overall, but I don't like that the functionality has been reduced compared to the existing version. Namely, the check whether a rule already exists no longer takes place if rule_id is not specified. I understand that the existing version was not perfect in this regard, but it was still useful and all my use cases relied on this functionality. Currently there is also no way to compensate the "removed" functionality with the lookup rule plugin.

I am currently working to bring that functionality back, if you do not mind.

I know that my changes are breaking existing stuff. I decided to do that, because the current module is kind of unreliable and unpredictable in its behavior, and so I wanted to move the module and its use cases to the direction of how the API is intended to be used. I will discuss with my colleagues if we might bring back the previous behavior at least as a side-feature.
Or probably (as you also wrote) it would help extending the rules lookup module, so it includes conditions and raw_value to search for. Would that be an option for you? If not, How should the rules lookup module behave, so you can keep your current use cases?

Best regards
Lars

[msekania]

Hi @lgetwan,

General problem which I have with a new version (without modification) is that that one creates all the time the same rule, because there is no any mechanisms that allows to check and stop recreation of the same rule again and again.
Currently one requires to provide rule_id in order to compare, edit, or move the rule. This will stay as it is.

I would appreciate if one would have a mechanism to try to deduce the rule_id if user is not providing it from the rest of parameters. Positioning in folder issues one can solve by introducing extra position value "any" in addition to "top", "bottom", "before", and "after", which will ignore precise position check within folder in is_equal check,

I have finished implementation of this features, and it's more or less like what I would like to have. I also found some extra bugs in current version.

In which form I should provide the changes?

Best regards,
Michael

[robin-checkmk]

In which form I should provide the changes?

@msekania may I suggest a branch based on this branch? That should allow you to open a PR onto this one.

PR on top of this PR: #586

[lgetwan]

I will check that tomorrow.

[lgetwan]

The last commit solves the problem for 2.2.0, but I still don't get why it doesn't work for 2.3.0...

[lgetwan]

@robin-checkmk & @msekania,
That was a hard piece of work considering that the diff is quite small, but now I'm pretty sure, that the idempotency in 2.3.0 is as "good" as in 2.2.0. Can you run your tests, again, as well, please?

[msekania]

@lgetwan,

there are some more improvements see #602 , I could not test 2.3.0 yet, but similar fixes might be due for 2.3.0 as well.

I also have a question concerning your last changes.
What if I intentionally provide empty list just to clean up already set one?
I think this changes should also go similar to what I have done for comment and description in #602

[msekania]

@lgetwan,

I have an Idea how to incorporate logic of your changes in what I wrote above. I'll need hour or two at most.
Of course you can drop them if you do not like them, no problem.

[msekania]

@lgetwan

@lgetwan,

I have an Idea how to incorporate logic of your changes in what I wrote above. I'll need hour or two at most. Of course you can drop them if you do not like them, no problem.

DONE

[lgetwan]

Ok, now idempotency looks quite good with 2.2.0 and 2.3.0, at least for my own tests and the integration tests.
Thanks again for #602, @msekania!
Can you run your tests against the newest version of this PR?

[msekania]

@lgetwan

Ok, now idempotency looks quite good with 2.2.0 and 2.3.0, at least for my own tests and the integration tests. Thanks again for #602, @msekania! Can you run your tests against the newest version of this PR?

I really love solution with _normalize_rule!

I have tested with 2.2.0 and it works for all my rules too.
Unfortunately, I cannot test 2.3.0 for the moment.

[robin-checkmk]

I can confirm ~85% idempotency as well for 2.2 and 2.3.
The only hiccup is, that "comment": "{{ ansible_date_time.iso8601 }} Set by Ansible" will not work, as you always get a different comment. But I think that is fine. In that case one needs to use the rule_id.

The only challenges that remain from my point of view are:

1. Verify that the module documentation is up-to-date with the recent changes.
2. Decide whether this is a breaking change or not and how to release it.