-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
267ffd1
fix ilm policy deserialization
jloleysens 0991acb
reorder expected jest object to match actual
jloleysens c40e650
fix removal of wait for snapshot if it is not on the form
jloleysens e141966
add client integration test for policy serialization of unknown fields
jloleysens 57b7196
save on a few chars
jloleysens adf967d
added unit test for deserializer and serializer
jloleysens 79c1536
Implement feedback
jloleysens 65ea753
Updated serialization unit test coverage
jloleysens 8c53017
Merge branch 'master' into ilm/fix-serializer
kibanamachine c33e2b9
Merge branch 'master' into ilm/fix-serializer
kibanamachine 263023a
Merge branch 'master' into ilm/fix-serializer
kibanamachine 3e45301
fixed minor issue in how serialization tests are being set up
jloleysens File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
88 changes: 88 additions & 0 deletions
88
...nagement/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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', | ||
'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); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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!