From e03f61fe26b2398e3b7b66ead1ab1069d333eaf5 Mon Sep 17 00:00:00 2001 From: Virginia Cepeda Date: Mon, 26 Jun 2023 17:44:26 -0300 Subject: [PATCH] Alerting: don't copy original rule uid when cloning (#70645) * Prevent using original rule uid when cloning * Use uid from url instead of a new param in the alert rule form * Add function to clone rule and tests for it * Fix lint & tests --------- Co-authored-by: Sonia Aguilar --- .../alerting/unified/CloneRuleEditor.test.tsx | 140 +++++++++++++++++- .../alerting/unified/CloneRuleEditor.tsx | 32 ++-- .../alerting/unified/ExistingRuleEditor.tsx | 2 +- .../unified/RuleEditorGrafanaRules.test.tsx | 1 - .../components/rule-editor/AlertRuleForm.tsx | 8 +- .../alerting/unified/types/rule-form.ts | 1 - .../__snapshots__/rule-form.test.ts.snap | 2 - .../alerting/unified/utils/rule-form.ts | 2 - 8 files changed, 163 insertions(+), 25 deletions(-) diff --git a/public/app/features/alerting/unified/CloneRuleEditor.test.tsx b/public/app/features/alerting/unified/CloneRuleEditor.test.tsx index 8179d22313a..a40da75e65b 100644 --- a/public/app/features/alerting/unified/CloneRuleEditor.test.tsx +++ b/public/app/features/alerting/unified/CloneRuleEditor.test.tsx @@ -10,12 +10,25 @@ import { config, setBackendSrv, setDataSourceSrv } from '@grafana/runtime'; import { backendSrv } from 'app/core/services/backend_srv'; import { AlertManagerCortexConfig } from 'app/plugins/datasource/alertmanager/types'; import 'whatwg-fetch'; +import { RuleWithLocation } from 'app/types/unified-alerting'; -import { RulerGrafanaRuleDTO } from '../../../types/unified-alerting-dto'; +import { + RulerAlertingRuleDTO, + RulerGrafanaRuleDTO, + RulerRecordingRuleDTO, + RulerRuleDTO, +} from '../../../types/unified-alerting-dto'; -import { CloneRuleEditor } from './CloneRuleEditor'; +import { cloneRuleDefinition, CloneRuleEditor } from './CloneRuleEditor'; import { ExpressionEditorProps } from './components/rule-editor/ExpressionEditor'; -import { mockDataSource, MockDataSourceSrv, mockRulerAlertingRule, mockRulerGrafanaRule, mockStore } from './mocks'; +import { + mockDataSource, + MockDataSourceSrv, + mockRulerAlertingRule, + mockRulerGrafanaRule, + mockRulerRuleGroup, + mockStore, +} from './mocks'; import { mockAlertmanagerConfigResponse } from './mocks/alertmanagerApi'; import { mockSearchApiResponse } from './mocks/grafanaApi'; import { mockRulerRulesApiResponse, mockRulerRulesGroupApiResponse } from './mocks/rulerApi'; @@ -222,4 +235,125 @@ describe('CloneRuleEditor', function () { }); }); }); + + describe('cloneRuleDefinition', () => { + it("Should change the cloned rule's name accordingly for Grafana rules", () => { + const rule: RulerGrafanaRuleDTO = mockRulerGrafanaRule( + { + for: '1m', + labels: { severity: 'critical', region: 'nasa' }, + annotations: { [Annotation.summary]: 'This is a very important alert rule' }, + }, + { uid: 'grafana-rule-1', title: 'First Grafana Rule', data: [] } + ); + + const originalRule: RuleWithLocation = { + ruleSourceName: 'my-prom-ds', + namespace: 'namespace-one', + group: mockRulerRuleGroup(), + rule, + }; + + const clonedRule: RuleWithLocation = cloneRuleDefinition(originalRule); + + const grafanaRule: RulerGrafanaRuleDTO = clonedRule.rule as RulerGrafanaRuleDTO; + + expect(originalRule.rule.grafana_alert.title).toEqual('First Grafana Rule'); + expect(grafanaRule.grafana_alert.title).toEqual('First Grafana Rule (copy)'); + }); + + it("Should change the cloned rule's name accordingly for Ruler rules", () => { + const rule: RulerAlertingRuleDTO = mockRulerAlertingRule({ + for: '1m', + alert: 'First Ruler Rule', + expr: 'vector(1) > 0', + labels: { severity: 'critical', region: 'nasa' }, + annotations: { [Annotation.summary]: 'This is a very important alert rule' }, + }); + + const originalRule: RuleWithLocation = { + ruleSourceName: 'my-prom-ds', + namespace: 'namespace-one', + group: mockRulerRuleGroup(), + rule, + }; + + const clonedRule: RuleWithLocation = cloneRuleDefinition(originalRule); + + const alertingRule: RulerAlertingRuleDTO = clonedRule.rule as RulerAlertingRuleDTO; + + expect(originalRule.rule.alert).toEqual('First Ruler Rule'); + expect(alertingRule.alert).toEqual('First Ruler Rule (copy)'); + }); + + it("Should change the cloned rule's name accordingly for Recording rules", () => { + const rule: RulerRecordingRuleDTO = { + record: 'instance:node_num_cpu:sum', + expr: 'count without (cpu) (count without (mode) (node_cpu_seconds_total{job="integrations/node_exporter"}))', + labels: { type: 'cpu' }, + }; + + const originalRule: RuleWithLocation = { + ruleSourceName: 'my-prom-ds', + namespace: 'namespace-one', + group: mockRulerRuleGroup(), + rule, + }; + + const clonedRule: RuleWithLocation = cloneRuleDefinition(originalRule); + + const recordingRule: RulerRecordingRuleDTO = clonedRule.rule as RulerRecordingRuleDTO; + + expect(originalRule.rule.record).toEqual('instance:node_num_cpu:sum'); + expect(recordingRule.record).toEqual('instance:node_num_cpu:sum (copy)'); + }); + + it('Should remove the group for provisioned Grafana rules', () => { + const rule: RulerGrafanaRuleDTO = mockRulerGrafanaRule( + { + for: '1m', + labels: { severity: 'critical', region: 'nasa' }, + annotations: { [Annotation.summary]: 'This is a very important alert rule' }, + }, + { uid: 'grafana-rule-1', title: 'First Grafana Rule', data: [], provenance: 'foo' } + ); + + const originalRule: RuleWithLocation = { + ruleSourceName: 'my-prom-ds', + namespace: 'namespace-one', + group: mockRulerRuleGroup(), + rule, + }; + + const clonedRule: RuleWithLocation = cloneRuleDefinition(originalRule); + + expect(originalRule.group.name).toEqual('group1'); + expect(clonedRule.group.name).toEqual(''); + }); + + it('The cloned rule should not contain a UID property', () => { + const rule: RulerGrafanaRuleDTO = mockRulerGrafanaRule( + { + for: '1m', + labels: { severity: 'critical', region: 'nasa' }, + annotations: { [Annotation.summary]: 'This is a very important alert rule' }, + }, + { uid: 'grafana-rule-1', title: 'First Grafana Rule', data: [] } + ); + + const originalRule: RuleWithLocation = { + ruleSourceName: 'my-prom-ds', + namespace: 'namespace-one', + group: mockRulerRuleGroup(), + rule, + }; + + const clonedRule: RuleWithLocation = cloneRuleDefinition(originalRule); + + const grafanaRule: RulerGrafanaRuleDTO = clonedRule.rule as RulerGrafanaRuleDTO; + + expect(originalRule.rule.grafana_alert.uid).toEqual('grafana-rule-1'); + expect(grafanaRule.grafana_alert.uid).toEqual(''); + }); + }); }); diff --git a/public/app/features/alerting/unified/CloneRuleEditor.tsx b/public/app/features/alerting/unified/CloneRuleEditor.tsx index ba82b6555b6..249261b064a 100644 --- a/public/app/features/alerting/unified/CloneRuleEditor.tsx +++ b/public/app/features/alerting/unified/CloneRuleEditor.tsx @@ -6,7 +6,7 @@ import { locationService } from '@grafana/runtime/src'; import { Alert, LoadingPlaceholder } from '@grafana/ui/src'; import { useDispatch } from '../../../types'; -import { RuleIdentifier } from '../../../types/unified-alerting'; +import { RuleIdentifier, RuleWithLocation } from '../../../types/unified-alerting'; import { RulerRuleDTO } from '../../../types/unified-alerting-dto'; import { AlertRuleForm } from './components/rule-editor/AlertRuleForm'; @@ -30,18 +30,9 @@ export function CloneRuleEditor({ sourceRuleId }: { sourceRuleId: RuleIdentifier } if (rule) { - const ruleClone = cloneDeep(rule); - changeRuleName( - ruleClone.rule, - generateCopiedName(getRuleName(ruleClone.rule), ruleClone.group.rules.map(getRuleName)) - ); + const ruleClone = cloneRuleDefinition(rule); const formPrefill = rulerRuleToFormValues(ruleClone); - // Provisioned alert rules have provisioned alert group which cannot be used in UI - if (isGrafanaRulerRule(rule.rule) && Boolean(rule.rule.grafana_alert.provenance)) { - formPrefill.group = ''; - } - return ; } @@ -74,3 +65,22 @@ function changeRuleName(rule: RulerRuleDTO, newName: string) { rule.record = newName; } } + +export function cloneRuleDefinition(rule: RuleWithLocation) { + const ruleClone = cloneDeep(rule); + changeRuleName( + ruleClone.rule, + generateCopiedName(getRuleName(ruleClone.rule), ruleClone.group.rules.map(getRuleName)) + ); + + if (isGrafanaRulerRule(ruleClone.rule)) { + ruleClone.rule.grafana_alert.uid = ''; + + // Provisioned alert rules have provisioned alert group which cannot be used in UI + if (Boolean(ruleClone.rule.grafana_alert.provenance)) { + ruleClone.group = { name: '', rules: ruleClone.group.rules }; + } + } + + return ruleClone; +} diff --git a/public/app/features/alerting/unified/ExistingRuleEditor.tsx b/public/app/features/alerting/unified/ExistingRuleEditor.tsx index c59d0632f78..cab88173af3 100644 --- a/public/app/features/alerting/unified/ExistingRuleEditor.tsx +++ b/public/app/features/alerting/unified/ExistingRuleEditor.tsx @@ -62,5 +62,5 @@ export function ExistingRuleEditor({ identifier, id }: ExistingRuleEditorProps) return Sorry! You do not have permission to edit this rule.; } - return ; + return ; } diff --git a/public/app/features/alerting/unified/RuleEditorGrafanaRules.test.tsx b/public/app/features/alerting/unified/RuleEditorGrafanaRules.test.tsx index d9d959098c6..8954ded9477 100644 --- a/public/app/features/alerting/unified/RuleEditorGrafanaRules.test.tsx +++ b/public/app/features/alerting/unified/RuleEditorGrafanaRules.test.tsx @@ -163,7 +163,6 @@ describe('RuleEditor grafana managed rules', () => { is_paused: false, no_data_state: 'NoData', title: 'my great new rule', - uid: '', }, }, ], diff --git a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx index 5785be51c95..95c7537b1cf 100644 --- a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx @@ -72,10 +72,9 @@ const AlertRuleNameInput = () => { type Props = { existing?: RuleWithLocation; prefill?: Partial; // Existing implies we modify existing rule. Prefill only provides default form values - id?: string; }; -export const AlertRuleForm = ({ existing, prefill, id }: Props) => { +export const AlertRuleForm = ({ existing, prefill }: Props) => { const styles = useStyles2(getStyles); const dispatch = useDispatch(); const notifyApp = useAppNotification(); @@ -83,8 +82,9 @@ export const AlertRuleForm = ({ existing, prefill, id }: Props) => { const [showEditYaml, setShowEditYaml] = useState(false); const [evaluateEvery, setEvaluateEvery] = useState(existing?.group.interval ?? MINUTE); - const routeParams = useParams<{ type: string }>(); + const routeParams = useParams<{ type: string; id: string }>(); const ruleType = translateRouteParamToRuleType(routeParams.type); + const uidFromParams = routeParams.id; const returnTo: string = (queryParams['returnTo'] as string | undefined) ?? '/alerting/list'; const [showDeleteModal, setShowDeleteModal] = useState(false); @@ -252,7 +252,7 @@ export const AlertRuleForm = ({ existing, prefill, id }: Props) => { )} - + )} diff --git a/public/app/features/alerting/unified/types/rule-form.ts b/public/app/features/alerting/unified/types/rule-form.ts index 8291878edaa..aa445b1793d 100644 --- a/public/app/features/alerting/unified/types/rule-form.ts +++ b/public/app/features/alerting/unified/types/rule-form.ts @@ -11,7 +11,6 @@ export enum RuleFormType { export interface RuleFormValues { // common name: string; - uid: string; type?: RuleFormType; dataSourceName: string | null; group: string; diff --git a/public/app/features/alerting/unified/utils/__snapshots__/rule-form.test.ts.snap b/public/app/features/alerting/unified/utils/__snapshots__/rule-form.test.ts.snap index c208aea1ff1..bfd1da8464e 100644 --- a/public/app/features/alerting/unified/utils/__snapshots__/rule-form.test.ts.snap +++ b/public/app/features/alerting/unified/utils/__snapshots__/rule-form.test.ts.snap @@ -15,7 +15,6 @@ exports[`formValuesToRulerGrafanaRuleDTO should correctly convert rule form valu "is_paused": false, "no_data_state": "NoData", "title": "", - "uid": "", }, "labels": { "": "", @@ -54,7 +53,6 @@ exports[`formValuesToRulerGrafanaRuleDTO should not save both instant and range "is_paused": false, "no_data_state": "NoData", "title": "", - "uid": "", }, "labels": { "": "", diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 47dc2df3d74..bb5f3af1ad6 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -131,7 +131,6 @@ export function formValuesToRulerGrafanaRuleDTO(values: RuleFormValues): Postabl return { grafana_alert: { title: name, - uid: values.uid, condition, no_data_state: noDataState, exec_err_state: execErrState, @@ -157,7 +156,6 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF return { ...defaultFormValues, name: ga.title, - uid: ga.uid, type: RuleFormType.grafana, group: group.name, evaluateEvery: group.interval || defaultFormValues.evaluateEvery,