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

[ILM] Policy form should not throw away data #83077

Merged
merged 12 commits into from
Nov 20, 2020
Prev Previous commit
Next Next commit
Implement feedback
- move serializer function around a little bit
- move serialize migrate and allocate function out of serializer
  file
  • Loading branch information
jloleysens committed Nov 11, 2020
commit 79c153630d7156f76661f3a69eaf04ad0c08d374
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export { createSerializer } from './serializer';
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { isEmpty } from 'lodash';

import { SerializedActionWithAllocation } from '../../../../../../common/types';

import { DataAllocationMetaFields } from '../../types';

export const serializeMigrateAndAllocateActions = (
{ dataTierAllocationType, allocationNodeAttribute }: DataAllocationMetaFields,
newActions: SerializedActionWithAllocation = {},
originalActions: SerializedActionWithAllocation = {}
): SerializedActionWithAllocation => {
const { allocate, migrate, ...otherActions } = newActions;

// First copy over all non-allocate and migrate actions.
const actions: SerializedActionWithAllocation = { ...otherActions };

// The UI only knows about include, exclude and require, so copy over all other values.
if (allocate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As a test, I pretended to be someone making changes to this code who didn't understand why we were copying "other actions" which aren't defined by our type system. I commented out lines 24 through 29 and replaced line 21 with:

// Hey, this is more efficient! I guess the original dev was just being extra safe?
const actions: SerializedActionWithAllocation = { ...newActions };

All of our tests still passed. I know this is a bit of a contrived example because it's likely someone would catch this change in CR or the dev would (hopefully) ask questions about the code, but if we're able to make this kind of significant implementation change without failing a test then I think there's a gap in the test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this totally makes sense to me to add this coverage! I've added 6 additional test cases for how we might alter the form state, removing a field and how we expect this to come through when serialized.

const { include, exclude, require, ...otherSettings } = allocate;
if (!isEmpty(otherSettings)) {
actions.allocate = { ...otherSettings };
}
}

switch (dataTierAllocationType) {
case 'node_attrs':
if (allocationNodeAttribute) {
const [name, value] = allocationNodeAttribute.split(':');
actions.allocate = {
// copy over any other allocate details like "number_of_replicas"
...actions.allocate,
require: {
[name]: value,
},
};
} else {
// The form has been configured to use node attribute based allocation but no node attribute
// was selected. We fall back to what was originally selected in this case. This might be
// migrate.enabled: "false"
actions.migrate = originalActions.migrate;
}

// copy over the original include and exclude values until we can set them in the form.
if (!isEmpty(originalActions?.allocate?.include)) {
actions.allocate = {
...actions.allocate,
include: { ...originalActions?.allocate?.include },
};
}

if (!isEmpty(originalActions?.allocate?.exclude)) {
actions.allocate = {
...actions.allocate,
exclude: { ...originalActions?.allocate?.exclude },
};
}
break;
case 'none':
actions.migrate = { enabled: false };
break;
default:
}
return actions;
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,71 +6,15 @@

import { produce } from 'immer';

import { isEmpty, merge } from 'lodash';
import { merge } from 'lodash';

import { SerializedPolicy, SerializedActionWithAllocation } from '../../../../../common/types';
import { SerializedPolicy } from '../../../../../../common/types';

import { defaultPolicy } from '../../../constants';
import { defaultPolicy } from '../../../../constants';

import { FormInternal, DataAllocationMetaFields } from '../types';
import { FormInternal } from '../../types';

const serializeAllocateAction = (
{ dataTierAllocationType, allocationNodeAttribute }: DataAllocationMetaFields,
newActions: SerializedActionWithAllocation = {},
originalActions: SerializedActionWithAllocation = {}
): SerializedActionWithAllocation => {
const { allocate, migrate, ...rest } = newActions;
// First copy over all non-require|include|exclude and migrate actions.
const actions: SerializedActionWithAllocation = { ...rest };

// We only set include, exclude and require here, so copy over all other values
if (allocate) {
const { include, exclude, require, ...restAllocate } = allocate;
if (!isEmpty(restAllocate)) {
actions.allocate = { ...restAllocate };
}
}

switch (dataTierAllocationType) {
case 'node_attrs':
if (allocationNodeAttribute) {
const [name, value] = allocationNodeAttribute.split(':');
actions.allocate = {
// copy over any other allocate details like "number_of_replicas"
...actions.allocate,
require: {
[name]: value,
},
};
} else {
// The form has been configured to use node attribute based allocation but no node attribute
// was selected. We fall back to what was originally selected in this case. This might be
// migrate.enabled: "false"
actions.migrate = originalActions.migrate;
}

// copy over the original include and exclude values until we can set them in the form.
if (!isEmpty(originalActions?.allocate?.include)) {
actions.allocate = {
...actions.allocate,
include: { ...originalActions?.allocate?.include },
};
}

if (!isEmpty(originalActions?.allocate?.exclude)) {
actions.allocate = {
...actions.allocate,
exclude: { ...originalActions?.allocate?.exclude },
};
}
break;
case 'none':
actions.migrate = { enabled: false };
break;
default:
}
return actions;
};
import { serializeMigrateAndAllocateActions } from './serialize_migrate_and_allocate_actions';

export const createSerializer = (originalPolicy?: SerializedPolicy) => (
data: FormInternal
Expand Down Expand Up @@ -115,7 +59,7 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => (
delete hotPhaseActions.forcemerge;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to comment out this line without a test failure.

}

if (!updatedPolicy.phases.hot!.actions?.set_priority && hotPhaseActions.set_priority) {
if (!updatedPolicy.phases.hot!.actions?.set_priority) {
delete hotPhaseActions.set_priority;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this line.

}
}
Expand All @@ -137,7 +81,7 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => (
delete warmPhase.min_age;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to comment out this line, and the tests still passed.

}

warmPhase.actions = serializeAllocateAction(
warmPhase.actions = serializeMigrateAndAllocateActions(
_meta.warm,
warmPhase.actions,
originalPolicy?.phases.warm?.actions
Expand Down Expand Up @@ -170,7 +114,7 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => (
coldPhase.min_age = `${updatedPolicy.phases.cold!.min_age}${_meta.cold.minAgeUnit}`;
}

coldPhase.actions = serializeAllocateAction(
coldPhase.actions = serializeMigrateAndAllocateActions(
_meta.cold,
coldPhase.actions,
originalPolicy?.phases.cold?.actions
Expand Down