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
added unit test for deserializer and serializer
  • Loading branch information
jloleysens committed Nov 11, 2020
commit adf967d06517d1d33594f8b2a4112289b522863a
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* 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 { cloneDeep, set } from 'lodash';
import { SerializedPolicy } from '../../../../../common/types';
import { deserializer } from './deserializer';
import { createSerializer } from './serializer';

describe('deserializer and serializer', () => {
it('preserves unknown policy settings', () => {
const originalPolicy: SerializedPolicy = {
name: 'test',
phases: {
hot: {
actions: {
rollover: {
max_age: '1d',
max_size: '10gb',
max_docs: 1000,
},
forcemerge: {
index_codec: 'best_compression',
max_num_segments: 22,
},
},
min_age: '12ms',
},
warm: {
actions: {
shrink: { number_of_shards: 12 },
allocate: {
number_of_replicas: 3,
},
set_priority: {
priority: 10,
},
},
},
cold: {
actions: {
allocate: { number_of_replicas: 12 },
freeze: {},
set_priority: {
priority: 12,
},
},
},
delete: {
actions: {
delete: {
delete_searchable_snapshot: true,
},
wait_for_snapshot: {
policy: 'test',
},
},
},
},
};

/**
* Next we insert unknown values at various places in the policy object
* where we know they may occur.
*/
[
'',
'phases.hot.actions',
'phases.hot.actions.rollover',
Copy link
Contributor

@cjcenizal cjcenizal Nov 12, 2020

Choose a reason for hiding this comment

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

Dumb question: is this list intended to be comprehensive? I noticed phases.hot.actions.forcemerge, phases.warm.actions.allocate, phases.delete.actions.wait_for_snapshot, and others like that missing and it made me wonder.

Consider someone reading the serializeMigrateAndAllocateActions code and wanting to understand that behavior -- what can we do with this test to communicate that behavior to them?

Also, do we want to verify that unknown settings on phases.hot (e.g. phases.hot.unknown) and the other phase objects are preserved?

Copy link
Contributor Author

@jloleysens jloleysens Nov 12, 2020

Choose a reason for hiding this comment

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

Yeah, I started writing this up and something didn't feel quite right because we would need to update this list in order for the check it is performing to remain thorough and reliable. I opted, instead, for creating a small bit of functionality that populates the entire policy with unknown values at all levels (except for one, but I left a comment about that). Let me know what you think!

'phases.warm.actions',
'phases.warm.actions.forcemerge',
'phases.cold.actions',
'phases.delete.actions',
].forEach((path) => {
set(originalPolicy, path, { unknown: 'value' });
});

const copyOfOriginalPolicy = cloneDeep(originalPolicy);
const formInternal = deserializer(copyOfOriginalPolicy);
const serializer = createSerializer(copyOfOriginalPolicy);

expect(serializer(formInternal)).toEqual(originalPolicy);

// Assert that the original policy is unaltered after deserialization and serialization
expect(originalPolicy).toEqual(copyOfOriginalPolicy);
});
});