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

Avoid sorting rules in rule-patching library #1852

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dgrisonnet
Copy link
Contributor

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

This change improves the rule-sanitizer library to avoid sorting all the
rules. This was originally done to simplify indexing rules with the same
names, but it turns out that integrating this lib can be complex to
review when all the rules of the manifests are reordered. Thus, it is
easier to just leave the file as it was initially sorted.

Type of change

What type of changes does your code introduce to the kube-prometheus? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. Later this will be copied to the changelog file.

Avoid sorting rules in rule-patching library

/cc @raptorsun

@dgrisonnet dgrisonnet force-pushed the improve-rule-patch branch 2 times, most recently from ea3b4dc to 93484f7 Compare August 28, 2022 16:24
@raptorsun
Copy link
Contributor

Test with a patch and the indexRules() function need some fix. In the returned array from indexRules() the index of every element is 0.
Tested with the following content in rule-patches.libsonnet:

{
  excludedRuleGroups: [],
  excludedRules: [],
  patchedRules: [
    {
      name: 'kubernetes-storage',
      rules: [
        {
          alert: 'KubePersistentVolumeFillingUp',
          index: 1,
          patches: 'index 1 patch',
        },
      ],
    },
  ],
}

In the output file kubernetes-prometheusRule.yaml the 2 rules for alert KubePersistentVolumeFillingUp has not changed.

This change improves the rule-sanitizer library to avoid sorting all the
rules. This was originally done to simplify indexing rules with the same
names, but it turns out that integrating this lib can be complex to
review when all the rules of the manifests are reordered. Thus, it is
easier to just leave the file as it was initially sorted.

Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
@dgrisonnet
Copy link
Contributor Author

It should be good now, I hadn't tested it enough and there was something wrong in the script.

@raptorsun
Copy link
Contributor

The new version looks good to me :)

@github-actions github-actions bot added the stale label Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants