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

feat: add disableProvenance option to GrafanaAlertRuleGroupSpec #1715

Merged

Conversation

chenlujjj
Copy link
Contributor

To resolve issue #1710

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2024

CLA assistant check
All committers have signed the CLA.

@theSuess
Copy link
Member

Thanks for the quick PR! From the previous discussion, I think we can call the option editable. The terraform provider is auto-generated, which comes with the quirk of having hard to understand fields. Since we can simplify this for the user by calling it editable instead, I'd do that.

What's the take of the other maintainers on this?

@chenlujjj chenlujjj force-pushed the feat/alertrule-disableprovenance-option branch from 807a92f to 92132ed Compare October 17, 2024 11:50
@chenlujjj
Copy link
Contributor Author

@theSuess Thanks for the advice, I've updated the code

Copy link
Member

@theSuess theSuess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this against the newest Grafana version, and it does not work as the header does not actually use the value but just checks for presence of any value.

Which version did you test against?

@chenlujjj
Copy link
Contributor Author

@theSuess Actually I didn't run e2e test, I thought it's a straightforward change. I'll follow the contribute guide to run the test and fix it.

@theSuess
Copy link
Member

Not sure if the e2e tests will catch this (since they can only introspect the kubernetes resources) but some manual testing should be enough

@chenlujjj chenlujjj force-pushed the feat/alertrule-disableprovenance-option branch from 92132ed to cd353f4 Compare October 23, 2024 14:44
@chenlujjj
Copy link
Contributor Author

Hello @theSuess , I've updated the code and run manual test in my local env, here are the test results:

  • No editable field or editable: true, the rule group can be edited or deleted in UI
  • editable: false, the rule group cannot be edited or deleted in UI
image

One thing worthy note is that the editable can be changed from true to false, but cannot from false to true, in which case the request will return 500:

logger=context userId=1 orgId=1 uname=root t=2024-10-24T07:13:58.237291685Z level=error msg="cannot change provenance from 'api' to ''" error="cannot change provenance from 'api' to ''" remote_addr=172.18.0.1 traceID=
logger=context userId=1 orgId=1 uname=root t=2024-10-24T07:13:58.237353217Z level=error msg="Request Completed" method=PUT path=/api/v1/provisioning/alert-rules/ce1tedpihg0lca status=500 remote_addr=172.18.0.1 time_ms=11 duration=11.370494ms size=68 referer= handler=/api/v1/provisioning/alert-rules/:UID status_source=server

@chenlujjj chenlujjj force-pushed the feat/alertrule-disableprovenance-option branch from cd353f4 to cd05d8f Compare October 24, 2024 07:31
@Baarsgaard
Copy link
Contributor

Baarsgaard commented Oct 24, 2024

The error on enabling editing seems quite bad.
Ideally, if the editable field changes, the operator would delete and apply the AlertRuleGroup to ensure desired state is achieved.
Or alternatively make the field immutable, preventing edits?

@theSuess
Copy link
Member

I'd go with the immutable field, it's conceptually easier and is more robust - thanks for continuing to work on this!

@chenlujjj
Copy link
Contributor Author

+1 for making the field immutable

@chenlujjj chenlujjj force-pushed the feat/alertrule-disableprovenance-option branch 2 times, most recently from 8e1499f to 838284e Compare October 28, 2024 11:52
@chenlujjj
Copy link
Contributor Author

Manually tested in local environment, the immutability works:

❯ k apply -f testalertgroup.yaml
The GrafanaAlertRuleGroup "group2" is invalid: spec.editable: Invalid value: "boolean": Editable field is immutable

@theSuess theSuess force-pushed the feat/alertrule-disableprovenance-option branch from 838284e to 3b1458f Compare October 29, 2024 09:44
@theSuess theSuess force-pushed the feat/alertrule-disableprovenance-option branch from 92b080e to c36c375 Compare October 29, 2024 09:54
@theSuess
Copy link
Member

I've changed the wording a bit and renamed a variable for clarity - thanks for working on this!

@theSuess theSuess added this pull request to the merge queue Oct 29, 2024
Merged via the queue into grafana:master with commit ab57184 Oct 29, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants