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

Add a warning callout when deleting managed assets #207329

Merged
merged 21 commits into from
Jan 28, 2025
Merged

Add a warning callout when deleting managed assets #207329

merged 21 commits into from
Jan 28, 2025

Conversation

sabarasaba
Copy link
Member

@sabarasaba sabarasaba commented Jan 21, 2025 •
edited by kibanamachine
Loading

Fixes #203154

Summary

This PR improves the confirmation modal for deleting managed index templates and ingest pipelines by using a small warning callout for adding context about Elasticsearch’s behavior of automatically recreating these assets and making sure users understand that deletions are processed but the assets will be recreated automatically.

The list of assets to be deleted in the deletion confirmation modal now also shows which ones are managed and which ones aren't.

Screenshot 2025-01-22 at 11 43 58

@sabarasaba sabarasaba added Feature:Index Management Index and index templates UI 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 v9.0.0 Feature:Ingest Node Pipelines Ingest node pipelines management backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Jan 21, 2025
@sabarasaba sabarasaba self-assigned this Jan 21, 2025
@sabarasaba sabarasaba requested a review from a team January 21, 2025 14:35
florent-leborgne
florent-leborgne reviewed Jan 21, 2025
View reviewed changes
Copy link
Contributor

@florent-leborgne florent-leborgne left a comment

Choose a reason for hiding this comment

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

Hey @sabarasaba! I left a few suggestions, lmk how it sounds to you!

src/platform/packages/shared/kbn-management/delete_managed_assets_callout/README.mdx Outdated Show resolved Hide resolved
...latform/packages/shared/kbn-management/delete_managed_assets_callout/src/callout.stories.tsx Outdated Show resolved Hide resolved
...latform/packages/shared/kbn-management/delete_managed_assets_callout/src/callout.stories.tsx Outdated Show resolved Hide resolved
src/platform/packages/shared/kbn-management/delete_managed_assets_callout/src/callout.tsx Outdated Show resolved Hide resolved
sabarasaba and others added 6 commits January 21, 2025 17:32
@sabarasaba @florent-leborgne
Update src/platform/packages/shared/kbn-management/delete_managed_ass…
5ed0763 
…ets_callout/src/callout.tsx

Co-authored-by: florent-leborgne <[email protected]>
@sabarasaba @florent-leborgne
Update src/platform/packages/shared/kbn-management/delete_managed_ass…
263384f 
…ets_callout/README.mdx

Co-authored-by: florent-leborgne <[email protected]>
@sabarasaba @florent-leborgne
Update src/platform/packages/shared/kbn-management/delete_managed_ass…
070c4bd 
…ets_callout/src/callout.stories.tsx

Co-authored-by: florent-leborgne <[email protected]>
@sabarasaba @florent-leborgne
Update src/platform/packages/shared/kbn-management/delete_managed_ass…
c199126 
…ets_callout/src/callout.stories.tsx

Co-authored-by: florent-leborgne <[email protected]>
@sabarasaba
Fix tests
2ea886b
@sabarasaba
Should be lowercase
20a3eb5
@sabarasaba sabarasaba marked this pull request as ready for review January 21, 2025 20:33
@sabarasaba sabarasaba requested a review from a team as a code owner January 21, 2025 20:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

florent-leborgne
florent-leborgne approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@florent-leborgne florent-leborgne left a comment

Choose a reason for hiding this comment

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

Copy LGTM! 🌆 See my previous comment inline if you prefer to turn it into plural

@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
indexManagement 720 722 +2
ingestPipelines 378 380 +2
total +4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/delete-managed-asset-callout - 6 +6

Async chunks

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

id before after diff
indexManagement 735.6KB 736.7KB +1.1KB
ingestPipelines 419.8KB 420.8KB +974.0B
total +2.0KB
Unknown metric groups

API count

id before after diff
@kbn/delete-managed-asset-callout - 6 +6

History

cc @sabarasaba

@ElenaStoeva ElenaStoeva self-requested a review January 22, 2025 18:31
ElenaStoeva
ElenaStoeva reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for improving this @sabarasaba! Tested locally and the modal looks good for both index templates and ingest pipelines. However, in index templates, I noticed that I'm not able to actually delete the template - clicking the Delete button doesn't do anything:

Screen.Recording.2025-01-24.at.21.28.58.mov

I am able to delete managed templates on main. Would it be possible that this PR causes this behavior?

src/platform/packages/shared/kbn-management/delete_managed_assets_callout/src/callout.tsx Outdated Show resolved Hide resolved
@sabarasaba
Copy link
Member Author

Thanks for having a look @ElenaStoeva, seems I forgot to add a schema param 7b5e01d

ElenaStoeva
ElenaStoeva approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback @sabarasaba! Latest changes lgtm and the deletion of index templates works now.

@sabarasaba sabarasaba merged commit c8bd387 into elastic:main Jan 28, 2025
10 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13006692704

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 207329

Questions ?

Please refer to the Backport tool documentation

@sabarasaba sabarasaba mentioned this pull request Jan 28, 2025
Merged
@sabarasaba
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

sabarasaba added a commit to sabarasaba/kibana that referenced this pull request Jan 28, 2025
@sabarasaba
Add a warning callout when deleting managed assets (elastic#207329)
28fdbbe 
(cherry picked from commit c8bd387)

# Conflicts:
#	.github/CODEOWNERS
#	tsconfig.base.json
#	yarn.lock
This was referenced Jan 29, 2025
Merged
Merged
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 30, 2025
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

sabarasaba added a commit that referenced this pull request Jan 30, 2025
@sabarasaba @kibanamachine
[8.x] Add a warning callout when deleting managed assets (#207329) (#…
1cf806f 
…208491)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Add a warning callout when deleting managed assets
(#207329)](#207329)

<!--- Backport version: 9.6.4 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Ignacio
Rivas","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-28T08:46:58Z","message":"Add
a warning callout when deleting managed assets
(#207329)","sha":"c8bd387668ed3e6fe0fd71ec9cfbc5be58bbfc5c","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Index
Management","Team:Kibana
Management","release_note:skip","v9.0.0","Feature:Ingest Node
Pipelines","backport:prev-minor"],"title":"Add a warning callout when
deleting managed
assets","number":207329,"url":"https://github.com/elastic/kibana/pull/207329","mergeCommit":{"message":"Add
a warning callout when deleting managed assets
(#207329)","sha":"c8bd387668ed3e6fe0fd71ec9cfbc5be58bbfc5c"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207329","number":207329,"mergeCommit":{"message":"Add
a warning callout when deleting managed assets
(#207329)","sha":"c8bd387668ed3e6fe0fd71ec9cfbc5be58bbfc5c"}}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <[email protected]>
@kibanamachine kibanamachine added v8.18.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElenaStoeva ElenaStoeva ElenaStoeva approved these changes

@florent-leborgne florent-leborgne florent-leborgne approved these changes

Assignees

@sabarasaba sabarasaba

Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) Feature:Index Management Index and index templates UI Feature:Ingest Node Pipelines Ingest node pipelines management 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 v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deleting managed Elasticsearch assets from Stack Management
5 participants
@sabarasaba @elasticmachine @kibanamachine @florent-leborgne @ElenaStoeva