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.

Sign up for GitHub

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

Jump to bottom

[ILM] Fix delete phase serialization bug #84870

Merged
merged 4 commits into from
Dec 4, 2020
Merged

[ILM] Fix delete phase serialization bug #84870

merged 4 commits into from
Dec 4, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Dec 3, 2020 •
edited
Loading

Summary

Fix #84830

Fix for 7.11 #84442

👆🏻we will need a separate fix to backport this to 7.10.x

Checklist

@jloleysens jloleysens added Feature:ILM v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 3, 2020
@jloleysens jloleysens requested a review from yuliacech December 3, 2020 08:55
@jloleysens jloleysens requested a review from a team as a code owner December 3, 2020 08:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

yuliacech
yuliacech reviewed Dec 3, 2020
View reviewed changes
Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Hi @jloleysens , thanks for fixing the serializer! Do you think it would be possible to view JSON request without providing the policy name as before? here's CJ's comment about it.
Also, I tested a related issue that delete phase doesn't add the delete action, and can confirm that a policy without delete action will not delete indices when reaching the phase.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Thanks for the review @yuliacech

Do you think it would be possible to view JSON request without providing the policy name as before? here's CJ's comment about it.

I'd like to punt on this change for now as I think it is separate and perhaps a bit more involved.

Also, I tested a related issue that delete phase doesn't add the delete action, and can confirm that a policy without delete action will not delete indices when reaching the phase.

Interesting! This is pre the serialization changes here, but I think it would be good to include the fix for this for sure.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
indexLifecycleManagement 212.4KB 214.4KB +2.0KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

yuliacech
yuliacech approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Hi @jloleysens , changes LGTM!
Agree to leave the JSON request view for later, do we need to open a separate issue for that? Thanks a lot for fixing the delete action as well, I also opened an issue in ES so that backend would reject empty actions request in the future.

@jloleysens
Copy link
Contributor Author

@yuliacech another issue sounds reasonable!

Weird that this was surfaced now! We will need to make another PR to backport this for 7.10.2 (I think it deserves to be fixed in the patch release).

@jloleysens jloleysens merged commit 9d6d783 into elastic:master Dec 4, 2020
@jloleysens jloleysens deleted the fix/ilm-delete-phase-serialization branch December 4, 2020 15:02
@yuliacech
Copy link
Contributor

The issue to track JSON preview #85030

jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 4, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/snapshot-…
d03bfdb 
…fields

* 'master' of github.com:elastic/kibana:
  [ILM] Fix delete phase serialization bug (elastic#84870)
  chore(NA): removes auto install of pre-commit hook (elastic#83566)
  chore(NA): upgrade node-sass into last v4.14.1 to stop shipping old node-gyp (elastic#84935)
  [Alerting] Enables AlertTypes to define the custom recovery action groups (elastic#84408)
  [ML] Functional tests - add missing test data cleanup (elastic#84998)
  Migrate privilege/role/user-related operations to a new Elasticsearch client. (elastic#84641)

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts
@jloleysens jloleysens mentioned this pull request Dec 7, 2020
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 7, 2020
@jloleysens @kibanamachine
[ILM] Fix delete phase serialization bug (elastic#84870)
78c7722 
* added test for correctly deserializing delete phase, and added fix

* clarify test comment

* fix serialization of hot phase to include delete action

Co-authored-by: Kibana Machine <[email protected]>
jloleysens added a commit that referenced this pull request Dec 7, 2020
@jloleysens @kibanamachine
[ILM] Fix delete phase serialization bug (#84870) (#85113)
06dc1fd 
* added test for correctly deserializing delete phase, and added fix

* clarify test comment

* fix serialization of hot phase to include delete action

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@yuliacech yuliacech mentioned this pull request Dec 9, 2020
Merged
1 task
@jloleysens jloleysens mentioned this pull request Jan 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuliacech yuliacech yuliacech approved these changes

Assignees
No one assigned
Labels
Feature:ILM release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
4 participants
@jloleysens @elasticmachine @kibanamachine @yuliacech