CNV-80608: management: add delete alert rule API#942
Conversation
Add DELETE /api/v1/alerting/rules endpoint for bulk deletion of user-defined alert rules with full test coverage. Signed-off-by: Shirly Radco <sradco@redhat.com> Signed-off-by: João Vilaça <jvilaca@redhat.com> Signed-off-by: Aviv Litman <alitman@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@sradco: This pull request references CNV-80608 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@sradco: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hi @jgbernalp , @jan--f , @simonpasquier, I would appreciate your review of this PR |
| type: object | ||
| required: | ||
| - id | ||
| - status_code |
There was a problem hiding this comment.
This casing seems inconsistent, should we stick with camelCase?
| properties: | ||
| ruleIds: | ||
| type: array | ||
| minItems: 1 |
There was a problem hiding this comment.
should we set a max here?, it seems a large number of ids would execute sequential calls to the k8s api.
| ruleIds: | ||
| type: array | ||
| minItems: 1 | ||
| items: | ||
| type: string | ||
| description: List of stable alert rule IDs to delete. |
There was a problem hiding this comment.
maybe we can add maxItems
| ruleIds: | |
| type: array | |
| minItems: 1 | |
| items: | |
| type: string | |
| description: List of stable alert rule IDs to delete. | |
| ruleIds: | |
| type: array | |
| minItems: 1 | |
| maxItems: 100 | |
| items: | |
| type: string | |
| description: List of stable alert rule IDs to delete (at most 100 per request). |
if yes we will need to update user_defined_alert_rule_bulk_delete.go accordingly
| "github.com/openshift/monitoring-plugin/pkg/managementlabels" | ||
| ) | ||
|
|
||
| func (c *client) DeleteUserDefinedAlertRuleById(ctx context.Context, alertRuleId string) error { |
There was a problem hiding this comment.
It seems this function is also deleting platform managed rules, is the name correct or the logic needs adjustment?
| } else if len(newRules) != len(group.Rules) { | ||
| updated = true | ||
| } |
There was a problem hiding this comment.
is this needed, it seems the filterRulesById changes the updated value already. Can we simplify this logic to make it more readable?
| results := make([]DeleteAlertRuleResult, 0, len(payload.RuleIds)) | ||
|
|
||
| for _, rawId := range payload.RuleIds { | ||
| id, err := parseParam(rawId, "ruleId") |
There was a problem hiding this comment.
IIUC, this is parsed from a JSON input. why do we need to decode the rawId?
4 participants
Add DELETE /api/v1/alerting/rules endpoint
for bulk deletion of user-defined alert
rules with full test coverage.
Also removes
.github/workflows/unit-tests.yamlas the OpenShift org runs all tests via Prow.
Signed-off-by: Shirly Radco sradco@redhat.com
Signed-off-by: João Vilaça jvilaca@redhat.com
Signed-off-by: Aviv Litman alitman@redhat.com
Co-authored-by: AI Assistant noreply@cursor.com