Add ILM url generator and use it in Index Management #82165

yuliacech · 2020-10-30T17:06:20Z

Summary

Fixes #79843.
This PR cleans up ILM to only use 1 service for navigation instead of hard coding paths, registers an url generator in ILM plugin setup and uses it in Index Management to link to edit page of an ILM policy.

Release Note

In the Index Management app, you can now click a data stream's index lifecycle policy to view it in the Index Lifecycle Policies app.

…page

elasticmachine · 2020-11-02T15:17:58Z

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

yuliacech · 2020-11-02T15:18:27Z

@elasticmachine merge upstream

cjcenizal · 2020-11-02T17:28:49Z

@yuliacech Would you mind phrasing the release note from the user's/reader's point of view? For example, "In the Index Management app, you can now click a data stream's index lifecycle policy to view it in the Index Lifecycle Policies app."

yuliacech · 2020-11-03T15:38:04Z

Sure @cjcenizal , I'll add your wording to Release notes, thanks a lot for this suggestion!

yuliacech · 2020-11-04T11:27:43Z

@elasticmachine merge upstream

jloleysens

Great work @yuliacech !

I tested locally and the code worked as intended.

I'd like to block this PR on 2 comments

My comment about simplifying types in url_generator
My comment about refactoring useUrlGenerator

The rest are all non-blocking!

Another general question I have is, do we also want to deep link from index template details to ILM since we can attach an ILM policy to those?

jloleysens · 2020-11-04T12:06:13Z

x-pack/plugins/index_lifecycle_management/public/application/services/navigation.ts

  return encodeURI(`/policies/edit/${encodeURIComponent(policyName)}`);
 };
+
+export const getPolicyCreatePath = () => {
+  return `/policies/edit`;


What do you think of replacing this with ROUTES.edit. Otherwise I think we can remove the use of string literals to conform to other string usages.

jloleysens · 2020-11-04T12:07:41Z

x-pack/plugins/index_lifecycle_management/public/application/services/navigation.ts

@@ -4,6 +4,20 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-export const getPolicyPath = (policyName: string): string => {
+export const ROUTES = {


Calling this const REACT_ROUTER_ROUTES could make this even clearer. Since the strings are coupled to how react router works.

I'd prefer keeping ROUTES variable name here for shortness sake and it seemed to me as a good convention to use for a react-router config paths.

jloleysens · 2020-11-04T12:22:59Z

x-pack/plugins/index_lifecycle_management/public/url_generator.ts

+export enum ILM_PAGES {
+  LIST = 'policies_list',
+  EDIT = 'policy_edit',
+  CREATE = 'policy_create',
+}
+
+export interface IlmPoliciesListUrlGeneratorState {
+  page: ILM_PAGES.LIST;
+  absolute?: boolean;
+}
+
+export interface IlmPolicyEditUrlGeneratorState {
+  page: ILM_PAGES.EDIT;
+  policyName: string;
+  absolute?: boolean;
+}
+
+export interface IlmPolicyCreateUrlGeneratorState {
+  page: ILM_PAGES.CREATE;
+  absolute?: boolean;
+}
+
+export type IlmUrlGeneratorState =
+  | IlmPoliciesListUrlGeneratorState
+  | IlmPolicyEditUrlGeneratorState
+  | IlmPolicyCreateUrlGeneratorState;


I think we can compress these types in the following way:

export interface IlmUrlGeneratorState { page: 'policies_list' | 'policy_edit' | 'policy_create'; policyName?: string; absolute?: boolean; } // then the switch statement further down becomes - with auto complete for string literals switch (state.page) { case 'policy_create': { return `${await getAppBasePath(!!state.absolute)}${getPolicyCreatePath()}`; } case 'policy_edit': { return `${await getAppBasePath(!!state.absolute)}${getPolicyEditPath( state.policyName! )}`; } case 'policies_list': { return `${await getAppBasePath(!!state.absolute)}${getPoliciesListPath()}`; } }

I think this makes the code a bit easier to understand - just because there is less of it :). What do you think?

jloleysens · 2020-11-04T12:27:59Z

x-pack/plugins/index_lifecycle_management/public/url_generator.ts

+  | IlmPolicyEditUrlGeneratorState
+  | IlmPolicyCreateUrlGeneratorState;
+
+export class IlmUrlGenerator implements UrlGeneratorsDefinition<typeof ILM_URL_GENERATOR_ID> {


I probably would have used a factory pattern here to capture getAppBasePath.

const createIlmUrlGenerator = (getApppBasePath: getAppBasePath: (absolute?: boolean) => Promise<string>): UrlGeneratorsDefinition<typeof ILM_URL_GENERATOR_ID> => { return { id: ILM_URL_GENERATOR_ID, /* etc */ } }

I think it would result in slightly less code. But I believe this is mostly stylistic preference!

jloleysens · 2020-11-04T12:31:43Z

x-pack/plugins/index_management/__jest__/client_integration/home/data_streams_tab.test.ts

@@ -287,4 +290,82 @@ describe('Data Streams tab', () => {
      });
    });
  });
+
+  describe('url generators', () => {


Great job on these test cases!!

jloleysens · 2020-11-04T12:36:18Z

...agement/public/application/sections/home/template_list/template_details/tabs/tab_summary.tsx

-  const {
-    core: { getUrlForApp },
-  } = useAppContext();
+  const [ilmPolicyLink, setIlmPolicyLink] = useState<string | undefined>();


We should move this piece of state into useUrlGenerator and have it return { url?: string }. Then we can also remove setLinkCallback.

I didn't think about using setState inside useEffect, great idea @jloleysens !

…generator

yuliacech · 2020-11-05T14:52:17Z

Thank you for the review, @jloleysens ! Great suggestions, I made url generator types easier in ILM and added state into useGenerator effect. Would you please have another look please?

yuliacech · 2020-11-06T07:48:32Z

@elasticmachine merge upstream

jloleysens

Great work @yuliacech ! Thanks for addressing my feedback, did not test this again locally but the code is looking in great shape 🍻

kibanamachine · 2020-11-06T14:34:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3859247

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`indexLifecycleManagement`	111	112	+1
`indexManagement`	508	511	+3
total			+4

async chunks size

id	before	after	diff
`indexLifecycleManagement`	224.4KB	224.4KB	+18.0B
`indexManagement`	1.5MB	1.5MB	+4.3KB
total			+4.3KB

page load bundle size

id	before	after	diff
`indexLifecycleManagement`	71.8KB	77.0KB	+5.2KB
`indexManagement`	113.6KB	113.8KB	+273.0B
total			+5.5KB

History

💔 Build #86324 failed 7589efa
💔 Build #86131 failed 4412f9c
💔 Build #86116 failed fa920cd
💚 Build #85768 succeeded 88cdad7
💚 Build #85267 succeeded 51d2f9a

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

* Add ILM url generator and use in IM for cross linking to policy edit page * Fix policy name in the link * Add review suggestions * Fix import * Fix eslint error Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Add ILM url generator and use in IM for cross linking to policy edit page * Fix policy name in the link * Add review suggestions * Fix import * Fix eslint error Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

sebelga · 2021-01-05T12:52:59Z

Testing this PR as part of the BC QA testing. I wonder if it is expected to have a full page refresh when navigating from the data stream details panel to the ILM app?

If I click the ILM app link in the left menu I don't have this page refresh. WDYT @yuliacech ?

yuliacech · 2021-01-11T17:32:09Z

Hi Seb @sebelga , thanks a lot for testing this PR! I created an issue #87876 to add an SPA friendly navigation to this link.

sebelga · 2021-01-14T11:40:23Z

Thanks @yuliacech !

Add ILM url generator and use in IM for cross linking to policy edit …

8b330dc

…page

yuliacech marked this pull request as ready for review November 2, 2020 15:17

yuliacech requested a review from a team as a code owner November 2, 2020 15:17

yuliacech added Feature:ILM Feature:Index Management Index and index templates UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0 labels Nov 2, 2020

yuliacech added the release_note:enhancement label Nov 2, 2020

kibanamachine and others added 2 commits November 2, 2020 10:18

Merge branch 'master' into ilm_url_generator

d831c7f

Fix policy name in the link

51d2f9a

yuliacech requested a review from jloleysens November 4, 2020 11:27

Merge branch 'master' into ilm_url_generator

88cdad7

jloleysens requested changes Nov 4, 2020

View reviewed changes

yuliacech and others added 4 commits November 5, 2020 15:08

Add review suggestions

4089344

Merge branch 'master' into ilm_url_generator

c150f0a

Merge remote-tracking branch 'origin/ilm_url_generator' into ilm_url_…

fa920cd

…generator

Fix import

4412f9c

kibanamachine and others added 2 commits November 6, 2020 02:48

Merge branch 'master' into ilm_url_generator

7589efa

Fix eslint error

3859247

jloleysens approved these changes Nov 6, 2020

View reviewed changes

yuliacech merged commit 71ec5bd into elastic:master Nov 6, 2020

yuliacech mentioned this pull request Nov 6, 2020

[7.x] Add ILM url generator and use it in Index Management (#82165) #82837

Merged

yuliacech deleted the ilm_url_generator branch December 3, 2020 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ILM url generator and use it in Index Management #82165

Add ILM url generator and use it in Index Management #82165

yuliacech commented Oct 30, 2020 •

edited

Loading

elasticmachine commented Nov 2, 2020

yuliacech commented Nov 2, 2020

cjcenizal commented Nov 2, 2020

yuliacech commented Nov 3, 2020

yuliacech commented Nov 4, 2020

jloleysens left a comment

jloleysens Nov 4, 2020

jloleysens Nov 4, 2020

yuliacech Nov 5, 2020

jloleysens Nov 4, 2020

jloleysens Nov 4, 2020

jloleysens Nov 4, 2020

jloleysens Nov 4, 2020

yuliacech Nov 5, 2020

yuliacech commented Nov 5, 2020

yuliacech commented Nov 6, 2020

jloleysens left a comment

kibanamachine commented Nov 6, 2020

sebelga commented Jan 5, 2021

yuliacech commented Jan 11, 2021

sebelga commented Jan 14, 2021

Add ILM url generator and use it in Index Management #82165

Add ILM url generator and use it in Index Management #82165

Conversation

yuliacech commented Oct 30, 2020 • edited Loading

Summary

Release Note

elasticmachine commented Nov 2, 2020

yuliacech commented Nov 2, 2020

cjcenizal commented Nov 2, 2020

yuliacech commented Nov 3, 2020

yuliacech commented Nov 4, 2020

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuliacech commented Nov 5, 2020

yuliacech commented Nov 6, 2020

jloleysens left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 6, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

sebelga commented Jan 5, 2021

yuliacech commented Jan 11, 2021

sebelga commented Jan 14, 2021

yuliacech commented Oct 30, 2020 •

edited

Loading