Skip to content

Commit 2ae41a5

Browse files
JiaweiWuJiaweiWu
committed
Address comments
1 parent c226f51 commit 2ae41a5

7 files changed

Lines changed: 258 additions & 9 deletions

File tree

‎x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import { validateFlapping } from './v1';
9+
10+
describe('validateFlapping', () => {
11+
test('should error if look back window exceeds the lower bound', () => {
12+
const result = validateFlapping({
13+
look_back_window: 0,
14+
status_change_threshold: 10,
15+
});
16+
17+
expect(result).toEqual('look back window must be between 2 and 20');
18+
});
19+
20+
test('should error if look back window exceeds the upper bound', () => {
21+
const result = validateFlapping({
22+
look_back_window: 50,
23+
status_change_threshold: 10,
24+
});
25+
26+
expect(result).toEqual('look back window must be between 2 and 20');
27+
});
28+
29+
test('should error if status change threshold exceeds the lower bound', () => {
30+
const result = validateFlapping({
31+
look_back_window: 10,
32+
status_change_threshold: 1,
33+
});
34+
35+
expect(result).toEqual('status change threshold must be between 2 and 20');
36+
});
37+
38+
test('should error if status change threshold exceeds the upper bound', () => {
39+
const result = validateFlapping({
40+
look_back_window: 10,
41+
status_change_threshold: 50,
42+
});
43+
44+
expect(result).toEqual('status change threshold must be between 2 and 20');
45+
});
46+
47+
test('should error if status change threshold is greater than the look back window', () => {
48+
const result = validateFlapping({
49+
look_back_window: 10,
50+
status_change_threshold: 15,
51+
});
52+
53+
expect(result).toEqual(
54+
'lookBackWindow (10) must be equal to or greater than statusChangeThreshold (15)'
55+
);
56+
});
57+
});

‎x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ const validateCanUpdateFlapping = (
5151
return;
5252
}
5353

54+
// If updated flapping is undefined then don't do anything, it's not being updated
55+
if (updateFlapping === undefined) {
56+
return;
57+
}
58+
5459
// If both versions are falsy, allow it even if its changing between undefined and null
5560
if (!originalFlapping && !updateFlapping) {
5661
return;
@@ -154,12 +159,6 @@ async function updateWithOCC<Params extends RuleParams = never>(
154159

155160
validateCanUpdateFlapping(isFlappingEnabled, originalFlapping, initialData.flapping);
156161

157-
if (!isFlappingEnabled && !isEqual(originalFlapping, initialData.flapping)) {
158-
throw Boom.badRequest(
159-
`Error updating rule: can not update rule flapping if global flapping is disabled`
160-
);
161-
}
162-
163162
let validationPayload: ValidateScheduleLimitResult = null;
164163
if (enabled && schedule.interval !== data.schedule.interval) {
165164
validationPayload = await validateScheduleLimit({

‎x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ describe('transformRuleAttributesToRuleDomain', () => {
9292
updatedBy: 'user',
9393
apiKey: MOCK_API_KEY,
9494
apiKeyOwner: 'user',
95+
flapping: {
96+
lookBackWindow: 20,
97+
statusChangeThreshold: 20,
98+
},
9599
};
96100

97101
it('transforms the actions correctly', () => {
@@ -106,6 +110,13 @@ describe('transformRuleAttributesToRuleDomain', () => {
106110
isSystemAction
107111
);
108112

113+
expect(res.flapping).toMatchInlineSnapshot(`
114+
Object {
115+
"lookBackWindow": 20,
116+
"statusChangeThreshold": 20,
117+
}
118+
`);
119+
109120
expect(res.actions).toMatchInlineSnapshot(`
110121
Array [
111122
Object {

‎x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ describe('transformRuleDomainToRule', () => {
6767
updatedBy: 'user',
6868
apiKey: MOCK_API_KEY,
6969
apiKeyOwner: 'user',
70+
flapping: {
71+
lookBackWindow: 20,
72+
statusChangeThreshold: 20,
73+
},
7074
};
7175

7276
it('should transform rule domain to rule', () => {
@@ -100,6 +104,10 @@ describe('transformRuleDomainToRule', () => {
100104
revision: 0,
101105
updatedBy: 'user',
102106
apiKeyOwner: 'user',
107+
flapping: {
108+
lookBackWindow: 20,
109+
statusChangeThreshold: 20,
110+
},
103111
});
104112
});
105113

@@ -135,6 +143,10 @@ describe('transformRuleDomainToRule', () => {
135143
revision: 0,
136144
updatedBy: 'user',
137145
apiKeyOwner: 'user',
146+
flapping: {
147+
lookBackWindow: 20,
148+
statusChangeThreshold: 20,
149+
},
138150
});
139151
});
140152

@@ -172,6 +184,10 @@ describe('transformRuleDomainToRule', () => {
172184
revision: 0,
173185
updatedBy: 'user',
174186
apiKeyOwner: 'user',
187+
flapping: {
188+
lookBackWindow: 20,
189+
statusChangeThreshold: 20,
190+
},
175191
});
176192
});
177193
});

‎x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import { RuleDomain } from '../types';
9+
import { transformRuleDomainToRuleAttributes } from './transform_rule_domain_to_rule_attributes';
10+
11+
describe('transformRuleDomainToRuleAttributes', () => {
12+
const MOCK_API_KEY = Buffer.from('123:abc').toString('base64');
13+
14+
const defaultAction = {
15+
id: '1',
16+
uuid: 'test-uuid',
17+
group: 'default',
18+
actionTypeId: 'test',
19+
params: {},
20+
};
21+
22+
const rule: RuleDomain<{}> = {
23+
id: 'test',
24+
enabled: false,
25+
name: 'my rule name',
26+
tags: ['foo'],
27+
alertTypeId: 'myType',
28+
consumer: 'myApp',
29+
schedule: { interval: '1m' },
30+
actions: [defaultAction],
31+
params: {},
32+
mapped_params: {},
33+
createdBy: 'user',
34+
createdAt: new Date('2019-02-12T21:01:22.479Z'),
35+
updatedAt: new Date('2019-02-12T21:01:22.479Z'),
36+
legacyId: 'legacyId',
37+
muteAll: false,
38+
mutedInstanceIds: [],
39+
snoozeSchedule: [],
40+
scheduledTaskId: 'task-123',
41+
executionStatus: {
42+
lastExecutionDate: new Date('2019-02-12T21:01:22.479Z'),
43+
status: 'pending' as const,
44+
},
45+
throttle: null,
46+
notifyWhen: null,
47+
revision: 0,
48+
updatedBy: 'user',
49+
apiKey: MOCK_API_KEY,
50+
apiKeyOwner: 'user',
51+
flapping: {
52+
lookBackWindow: 20,
53+
statusChangeThreshold: 20,
54+
},
55+
};
56+
57+
test('should transform rule domain to rule attribute', () => {
58+
const result = transformRuleDomainToRuleAttributes({
59+
rule,
60+
actionsWithRefs: [
61+
{
62+
group: 'default',
63+
actionRef: 'action_0',
64+
actionTypeId: 'test',
65+
uuid: 'test-uuid',
66+
params: {},
67+
},
68+
],
69+
params: {
70+
legacyId: 'test',
71+
paramsWithRefs: {},
72+
},
73+
});
74+
75+
expect(result).toMatchInlineSnapshot(`
76+
Object {
77+
"actions": Array [
78+
Object {
79+
"actionRef": "action_0",
80+
"actionTypeId": "test",
81+
"group": "default",
82+
"params": Object {},
83+
"uuid": "test-uuid",
84+
},
85+
],
86+
"alertTypeId": "myType",
87+
"apiKey": "MTIzOmFiYw==",
88+
"apiKeyOwner": "user",
89+
"consumer": "myApp",
90+
"createdAt": "2019-02-12T21:01:22.479Z",
91+
"createdBy": "user",
92+
"enabled": false,
93+
"executionStatus": Object {
94+
"lastExecutionDate": "2019-02-12T21:01:22.479Z",
95+
"status": "pending",
96+
},
97+
"flapping": Object {
98+
"lookBackWindow": 20,
99+
"statusChangeThreshold": 20,
100+
},
101+
"legacyId": "test",
102+
"muteAll": false,
103+
"mutedInstanceIds": Array [],
104+
"name": "my rule name",
105+
"notifyWhen": null,
106+
"params": Object {},
107+
"revision": 0,
108+
"schedule": Object {
109+
"interval": "1m",
110+
},
111+
"scheduledTaskId": "task-123",
112+
"snoozeSchedule": Array [],
113+
"tags": Array [
114+
"foo",
115+
],
116+
"throttle": null,
117+
"updatedAt": "2019-02-12T21:01:22.479Z",
118+
"updatedBy": "user",
119+
}
120+
`);
121+
});
122+
});

‎x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,7 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
636636
await resetRulesSettings(supertest, 'space1');
637637
});
638638

639-
it('should allow flapping to be updated', async () => {
639+
it('should allow flapping to be created', async () => {
640640
const response = await supertest
641641
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`)
642642
.set('kbn-xsrf', 'foo')

‎x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,13 +294,53 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
294294
},
295295
})
296296
.expect(400);
297+
});
298+
299+
it('should allow rule to be updated when global flapping is off if not updating flapping', async () => {
300+
const response = await supertest
301+
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`)
302+
.set('kbn-xsrf', 'foo')
303+
.send(
304+
getTestRuleData({
305+
flapping: {
306+
look_back_window: 5,
307+
status_change_threshold: 5,
308+
},
309+
})
310+
);
311+
312+
objectRemover.add(Spaces.space1.id, response.body.id, 'rule', 'alerting');
313+
314+
await supertest
315+
.post(`${getUrlPrefix(Spaces.space1.id)}/internal/alerting/rules/settings/_flapping`)
316+
.set('kbn-xsrf', 'foo')
317+
.send({
318+
enabled: false,
319+
look_back_window: 5,
320+
status_change_threshold: 5,
321+
});
297322

298-
// Can still update when global flapping is off if not updating flapping
299323
await supertest
300324
.put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`)
301325
.set('kbn-xsrf', 'foo')
302326
.send({
303-
name: 'bcd',
327+
name: 'updated name 1',
328+
tags: ['foo'],
329+
params: {
330+
foo: true,
331+
},
332+
schedule: { interval: '12s' },
333+
actions: [],
334+
throttle: '1m',
335+
notify_when: 'onThrottleInterval',
336+
})
337+
.expect(200);
338+
339+
await supertest
340+
.put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`)
341+
.set('kbn-xsrf', 'foo')
342+
.send({
343+
name: 'updated name 2',
304344
tags: ['foo'],
305345
params: {
306346
foo: true,
@@ -309,6 +349,10 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
309349
actions: [],
310350
throttle: '1m',
311351
notify_when: 'onThrottleInterval',
352+
flapping: {
353+
look_back_window: 5,
354+
status_change_threshold: 5,
355+
},
312356
})
313357
.expect(200);
314358
});

0 commit comments

Comments
 (0)