Skip to content
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

Bugfix rule module folder handling #224

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Bugfix rule module folder handling #224

merged 1 commit into from
Jan 16, 2023

Conversation

lgetwan
Copy link
Contributor

@lgetwan lgetwan commented Jan 16, 2023

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?

#195

What is the new behavior?

  • When checking if the rule already exists, we now also take care of the rule's folder.
  • Creating equal rules in multiple folders is now possible.

Other information

@robin-checkmk robin-checkmk changed the base branch from main to devel January 16, 2023 12:16
@robin-checkmk robin-checkmk added bug Something isn't working module:rule This affects the rule module labels Jan 16, 2023
@lgetwan lgetwan merged commit 40fabd1 into devel Jan 16, 2023
@robin-checkmk robin-checkmk mentioned this pull request Jan 16, 2023
7 tasks
@geof77
Copy link
Contributor

geof77 commented Jan 18, 2023

Does not work very well.

I have tried a lot of different solutions. Here is my version that is working most of the time (as far as I've tested), although there might be security issues using eval ?:

def get_existing_rule(module, base_url, headers, ruleset, rule):
    # Get rules in ruleset
    rules = get_rules_in_ruleset(module, base_url, headers, ruleset)
    if rules is not None:
        # Loop through all rules
        for r in rules.get("value"):
            # Check if conditions, properties and values are the same
            if (
                # these are both strings so == works
                r["extensions"]["folder"] == rule["folder"]
                # == also works for dicts 
                and r["extensions"]["conditions"] == rule["conditions"]
                and r["extensions"]["properties"] == rule["properties"]
               # per API documentation value_raw must be a valid python object
               # so just eval() both and compare
                and eval(r["extensions"]["value_raw"]) == eval(rule["value_raw"])
            ):
                # If they are the same, return the ID
                return r["id"]
    return None

@geof77
Copy link
Contributor

geof77 commented Jan 18, 2023

Thinking a bit further, what should this function really compare?
In properties, "description" and "comment" are not "functional", only "disabled" is significant.
So a rule with exactly the same folder, conditions and value_raw would have the same effect, regardless of the description and comment. I would exclude these two from the comparison.
What do you think?

@geof77
Copy link
Contributor

geof77 commented Jan 18, 2023

So this would give us something like:

def get_existing_rule(module, base_url, headers, ruleset, rule):
    # Get rules in ruleset
    rules = get_rules_in_ruleset(module, base_url, headers, ruleset)
    if rules is not None:
        # Loop through all rules
        for r in rules.get("value"):
            e=r["extensions"]
            # Check if conditions, properties and values are the same
            if (
                e["folder"] == rule["folder"]
                and e["conditions"] == rule["conditions"]
                and e["properties"]["disabled"] == rule["properties"]["disabled"]
                and eval(e["value_raw"]) == eval(rule["value_raw"])
            ):
                # If they are the same, return the ID
                return r["id"]
    return None

@geof77
Copy link
Contributor

geof77 commented Jan 18, 2023

@lgetwan , @robin-tribe29 please review as the existing function breaks things and I can't get integration tests to succeed for PR #231

@lgetwan
Copy link
Contributor Author

lgetwan commented Jan 19, 2023

@geoff: Please see my comment in #186: #186 (comment)

@geof77
Copy link
Contributor

geof77 commented Jan 19, 2023

@lgetwan : The PR introduces an error in the examples. "folder" and "location" are mutually exclusive. "folder" is deprecated and the value is picked up from location.

@lgetwan
Copy link
Contributor Author

lgetwan commented Jan 20, 2023

@geof77: True! But gladly, your PR #223 fixed this. Thanks. :-)

@lgetwan lgetwan deleted the fix-195-rule-folder branch January 20, 2023 07:32
@lgetwan lgetwan deleted the fix-195-rule-folder branch January 20, 2023 07:32
robin-checkmk pushed a commit that referenced this pull request May 22, 2023
Bugfix rule module folder handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module:rule This affects the rule module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants