-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Onboarding] Add delete index action to search index detail page #192428
[Onboarding] Add delete index action to search index detail page #192428
Conversation
478a21a
to
fe41521
Compare
@@ -18,7 +18,7 @@ export const useIndex = (indexName: string) => { | |||
refetchInterval: POLLING_INTERVAL, | |||
refetchIntervalInBackground: true, | |||
refetchOnWindowFocus: 'always', | |||
retry: true, | |||
retry: false, |
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.
When retry is true
, the api try to refetch even if there is a fetch error. So there is no way to confirm if fetchIndex had a failure since isLoading
& isSuccess
will be false.
Setting retry to false
, would stop refetch when last fetch failed and will save the error which can help to show error message in front end.
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.
Would suggest retry to be capped to a number like 3 rather than true so that errors are bubbled to the UI when it consistently throws an error
https://tanstack.com/query/latest/docs/framework/react/reference/useQuery#usequery
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.
updated in 13ac078
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.
svl_search_navigation
test service changes LGTM
@@ -51,6 +51,7 @@ export const getDocLinks = ({ kibanaBranch, buildFlavor }: GetDocLinkOptions): D | |||
elasticStackGetStarted: isServerless | |||
? `${SERVERLESS_DOCS}` | |||
: `${ELASTIC_WEBSITE_URL}guide/en/index.html`, | |||
apiReference: `${ELASTIC_WEBSITE_URL}guide/en/starting-with-the-elasticsearch-platform-and-its-solutions/${DOC_LINK_VERSION}/api-reference.html`, |
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.
would link this to current, like the other URLS
apiReference: `${ELASTIC_WEBSITE_URL}guide/en/starting-with-the-elasticsearch-platform-and-its-solutions/${DOC_LINK_VERSION}/api-reference.html`, | |
apiReference: `${ELASTIC_WEBSITE_URL}guide/en/starting-with-the-elasticsearch-platform-and-its-solutions/current/api-reference.html`, |
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.
updated in 13ac078
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#6907[✅] x-pack/test_serverless/functional/test_suites/search/config.feature_flags.ts: 25/25 tests passed. |
indices, | ||
}); | ||
const result = useMutation({ | ||
mutationFn: async () => { |
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.
also invalidate the index from querycache, onSuccess. see this doc https://tanstack.com/query/v4/docs/framework/react/guides/invalidations-from-mutations
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.
updated in 13ac078
title={i18n.translate( | ||
'xpack.searchIndices.indexActionsMenu.deleteIndex.confirmModal.modalTitle', | ||
{ | ||
defaultMessage: 'Delete {index}', |
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.
I think the modal title should be more general and simply read 'Delete index'—especially since we are also including the index name in the body of the modal.
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.
updated in 5dde7ca
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.
Overall looks good, some minor comments and questions.
constructor() {} | ||
|
||
setDocLinks(newDocLinks: DocLinks) { | ||
this.apiReference = newDocLinks.apiReference; |
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.
@joemcelroy Will we need to worry about showing different links here based on serverless and stack?
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.
We will do eventually. ATM we rely on stack URLs, waiting on @leemthompo guidance on how that will work out :)
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.
@lcawl knows more about this topic than me, and I know the autogenerated API refs are coming along nicely :)
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.
We do have the isServerless option too IIUC
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.
Yes, the doc link service can have two "flavours" of links so that's definitely something that already exists.
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.
@lcawl would you recommend linking to the autogenerated API refs today, or circling back in a while on that one?
x-pack/plugins/search_indices/public/components/indices/details_page.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/search_indices/public/components/indices/details_page.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/search_indices/public/components/indices/delete_index_modal.tsx
Outdated
Show resolved
Hide resolved
indices, | ||
}); | ||
const queryClient = useQueryClient(); | ||
const { queryKey } = useIndex(indexName); |
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.
We should probably have a constants for the query keys and likely a function for building the queryKey based on the indexName.
I think using this hook here will do a fetch of of the index, with the polling, by including this hook here.
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.
For an example I experimented with using enum
for query keys in search homepage That PR ended up not getting merged for other reasons though.
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.
Also moved for other UseQuery implementation APIs as well. d7ca011
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#6912[✅] x-pack/test_serverless/functional/test_suites/search/config.feature_flags.ts: 25/25 tests passed. |
@joemcelroy @TattdCodeMonkey, just a note. I have also updated FTR for page reload logic in f4866b7 |
…ana into onboarding/delete-index
@elasticmachine merge upstream |
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.
Looks good!
f803c82
to
7e966a6
Compare
@@ -51,6 +51,7 @@ export const getDocLinks = ({ kibanaBranch, buildFlavor }: GetDocLinkOptions): D | |||
elasticStackGetStarted: isServerless | |||
? `${SERVERLESS_DOCS}` | |||
: `${ELASTIC_WEBSITE_URL}guide/en/index.html`, | |||
apiReference: `${ELASTIC_WEBSITE_URL}guide/en/starting-with-the-elasticsearch-platform-and-its-solutions/current/api-reference.html`, |
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.
apiReference:
${ELASTIC_WEBSITE_URL}guide/en/starting-with-the-elasticsearch-platform-and-its-solutions/current/api-reference.html
,
If you do want this link to take you to a list of all APIs for all products and deployment types, this is a good target for now (the long term target will be https://www.elastic.co/docs/api/). If you're only interested in a specific subset of APIs and you want to differentiate between serverless and non-serverless contexts, you could use the "isServerless" condition here. Just lmk if there a specific subset you want to be able to link to and I can help figure out the URL.
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.
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.
IMO, we could go with stack url and revisit in future if need to differentiate between serverless & stack.
@joemcelroy please lmk if you think otherwise
pinging @elastic/kibana-core |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Approving doclink to unblock :)
Apologies, ignore the linked skip. That was meant for #192688 |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…stic#192428) ## Summary In this PR for Onboarding Search detail page added, * Delete index action * Page loading section while fetching index * Page error section when index not found * Api reference doc link * Added FTR tests for the new implementation ## Screenshots <img width="1462" alt="Screenshot 2024-09-09 at 11 17 05 PM" src="https://github.com/user-attachments/assets/b407d8ff-36c6-4bd6-a6af-d63d9ee6ac40"> <img width="1484" alt="Screenshot 2024-09-09 at 11 17 33 PM" src="https://github.com/user-attachments/assets/5ff8a84d-b8fd-4b43-8057-2691a3741353"> **How to test:** 1. Enable searchIndices plugin in `kibana.dev.yml` as this plugin is behind Feature flag ``` xpack.searchIndices.enabled: true ``` 2. Create a new index 3. Navigate to `/app/elasticsearch/indices/index_details/${indexName}` 4. Delete index and visit to `/app/elasticsearch/indices/index_details/${indexName}` again ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit dcbba95) # Conflicts: # x-pack/plugins/search_indices/tsconfig.json
#192428) (#193075) # Backport This will backport the following commits from `main` to `8.x`: - [[Onboarding] Add delete index action to search index detail page (#192428)](#192428) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Saarika Bhasi","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-13T14:31:23Z","message":"[Onboarding] Add delete index action to search index detail page (#192428)\n\n## Summary\r\n\r\nIn this PR for Onboarding Search detail page added, \r\n* Delete index action \r\n* Page loading section while fetching index \r\n* Page error section when index not found \r\n* Api reference doc link\r\n* Added FTR tests for the new implementation\r\n\r\n## Screenshots\r\n<img width=\"1462\" alt=\"Screenshot 2024-09-09 at 11 17 05 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/b407d8ff-36c6-4bd6-a6af-d63d9ee6ac40\">\r\n\r\n<img width=\"1484\" alt=\"Screenshot 2024-09-09 at 11 17 33 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/5ff8a84d-b8fd-4b43-8057-2691a3741353\">\r\n\r\n**How to test:** \r\n1. Enable searchIndices plugin in `kibana.dev.yml` as this plugin is\r\nbehind Feature flag\r\n```\r\nxpack.searchIndices.enabled: true\r\n\r\n```\r\n2. Create a new index \r\n3. Navigate to `/app/elasticsearch/indices/index_details/${indexName}` \r\n4. Delete index and visit to\r\n`/app/elasticsearch/indices/index_details/${indexName}` again\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"dcbba9561f6911e3b579cd3c0eed387bfa08ff66","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","v9.0.0","v8.16.0"],"number":192428,"url":"https://github.com/elastic/kibana/pull/192428","mergeCommit":{"message":"[Onboarding] Add delete index action to search index detail page (#192428)\n\n## Summary\r\n\r\nIn this PR for Onboarding Search detail page added, \r\n* Delete index action \r\n* Page loading section while fetching index \r\n* Page error section when index not found \r\n* Api reference doc link\r\n* Added FTR tests for the new implementation\r\n\r\n## Screenshots\r\n<img width=\"1462\" alt=\"Screenshot 2024-09-09 at 11 17 05 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/b407d8ff-36c6-4bd6-a6af-d63d9ee6ac40\">\r\n\r\n<img width=\"1484\" alt=\"Screenshot 2024-09-09 at 11 17 33 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/5ff8a84d-b8fd-4b43-8057-2691a3741353\">\r\n\r\n**How to test:** \r\n1. Enable searchIndices plugin in `kibana.dev.yml` as this plugin is\r\nbehind Feature flag\r\n```\r\nxpack.searchIndices.enabled: true\r\n\r\n```\r\n2. Create a new index \r\n3. Navigate to `/app/elasticsearch/indices/index_details/${indexName}` \r\n4. Delete index and visit to\r\n`/app/elasticsearch/indices/index_details/${indexName}` again\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"dcbba9561f6911e3b579cd3c0eed387bfa08ff66"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192428","number":192428,"mergeCommit":{"message":"[Onboarding] Add delete index action to search index detail page (#192428)\n\n## Summary\r\n\r\nIn this PR for Onboarding Search detail page added, \r\n* Delete index action \r\n* Page loading section while fetching index \r\n* Page error section when index not found \r\n* Api reference doc link\r\n* Added FTR tests for the new implementation\r\n\r\n## Screenshots\r\n<img width=\"1462\" alt=\"Screenshot 2024-09-09 at 11 17 05 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/b407d8ff-36c6-4bd6-a6af-d63d9ee6ac40\">\r\n\r\n<img width=\"1484\" alt=\"Screenshot 2024-09-09 at 11 17 33 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/5ff8a84d-b8fd-4b43-8057-2691a3741353\">\r\n\r\n**How to test:** \r\n1. Enable searchIndices plugin in `kibana.dev.yml` as this plugin is\r\nbehind Feature flag\r\n```\r\nxpack.searchIndices.enabled: true\r\n\r\n```\r\n2. Create a new index \r\n3. Navigate to `/app/elasticsearch/indices/index_details/${indexName}` \r\n4. Delete index and visit to\r\n`/app/elasticsearch/indices/index_details/${indexName}` again\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"dcbba9561f6911e3b579cd3c0eed387bfa08ff66"}},{"branch":"8.x","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: kibanamachine <[email protected]>
Summary
In this PR for Onboarding Search detail page added,
Screenshots
How to test:
kibana.dev.yml
as this plugin is behind Feature flag/app/elasticsearch/indices/index_details/${indexName}
/app/elasticsearch/indices/index_details/${indexName}
againChecklist
Delete any items that are not applicable to this PR.