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.

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

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

Merged
merged 10 commits into from
Nov 6, 2020
3 changes: 2 additions & 1 deletion x-pack/plugins/index_lifecycle_management/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"requiredPlugins": [
"licensing",
"management",
"features"
"features",
"share"
],
"optionalPlugins": [
"cloud",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { UIM_APP_LOAD } from './constants/ui_metric';
import { EditPolicy } from './sections/edit_policy';
import { PolicyTable } from './sections/policy_table';
import { trackUiMetric } from './services/ui_metric';
import { ROUTES } from './services/navigation';

export const App = ({
history,
Expand All @@ -28,14 +29,14 @@ export const App = ({
return (
<Router history={history}>
<Switch>
<Redirect exact from="/" to="/policies" />
<Redirect exact from="/" to={ROUTES.list} />
<Route
exact
path={`/policies`}
path={ROUTES.list}
render={(props) => <PolicyTable {...props} navigateToApp={navigateToApp} />}
/>
<Route
path={`/policies/edit/:policyName?`}
path={ROUTES.edit}
render={(props) => <EditPolicy {...props} getUrlForApp={getUrlForApp} />}
/>
</Switch>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { RouteComponentProps } from 'react-router-dom';
import { reactRouterNavigate } from '../../../../../../../../src/plugins/kibana_react/public';
import { getIndexListUri } from '../../../../../../index_management/public';
import { PolicyFromES } from '../../../../../common/types';
import { getPolicyPath } from '../../../services/navigation';
import { getPolicyEditPath } from '../../../services/navigation';
import { sortTable } from '../../../services';
import { trackUiMetric } from '../../../services/ui_metric';

Expand Down Expand Up @@ -229,7 +229,7 @@ export const TableContent: React.FunctionComponent<Props> = ({
return (
<EuiLink
data-test-subj="policyTablePolicyNameLink"
{...reactRouterNavigate(history, getPolicyPath(value as string), () =>
{...reactRouterNavigate(history, getPolicyEditPath(value as string), () =>
trackUiMetric(METRIC_TYPE.CLICK, UIM_EDIT_CLICK)
)}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { reactRouterNavigate } from '../../../../../../../src/plugins/kibana_rea
import { PolicyFromES } from '../../../../common/types';
import { filterItems } from '../../services';
import { TableContent } from './components/table_content';
import { getPolicyCreatePath } from '../../services/navigation';

interface Props {
policies: PolicyFromES[];
Expand All @@ -45,7 +46,7 @@ export const PolicyTable: React.FunctionComponent<Props> = ({

const createPolicyButton = (
<EuiButton
{...reactRouterNavigate(history, '/policies/edit')}
{...reactRouterNavigate(history, getPolicyCreatePath())}
fill
iconType="plusInCircle"
data-test-subj="createPolicyButton"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

list: '/policies',
edit: '/policies/edit/:policyName?',
create: '/policies/edit',
};

export const getPolicyEditPath = (policyName: string): string => {
return encodeURI(`/policies/edit/${encodeURIComponent(policyName)}`);
};

export const getPolicyCreatePath = () => {
return `/policies/edit`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

};

export const getPoliciesListPath = () => {
return '/policies';
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from '@elastic/eui';

import { ApplicationStart } from 'kibana/public';
import { getPolicyPath } from '../../application/services/navigation';
import { getPolicyEditPath } from '../../application/services/navigation';
import { Index, IndexLifecyclePolicy } from '../../../common/types';

const getHeaders = (): Array<[keyof IndexLifecyclePolicy, string]> => {
Expand Down Expand Up @@ -192,7 +192,7 @@ export class IndexLifecycleSummary extends Component<Props, State> {
content = (
<EuiLink
href={this.props.getUrlForApp('management', {
path: `data/index_lifecycle_management/${getPolicyPath(value)}`,
path: `data/index_lifecycle_management/${getPolicyEditPath(value)}`,
})}
>
{value}
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/index_lifecycle_management/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ import { IndexLifecycleManagementPlugin } from './plugin';
export const plugin = (initializerContext: PluginInitializerContext) => {
return new IndexLifecycleManagementPlugin(initializerContext);
};

export { ILM_URL_GENERATOR_ID, ILM_PAGES, IlmUrlGeneratorState } from './url_generator';
9 changes: 6 additions & 3 deletions x-pack/plugins/index_lifecycle_management/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ import { init as initDocumentation } from './application/services/documentation'
import { init as initUiMetric } from './application/services/ui_metric';
import { init as initNotification } from './application/services/notification';
import { addAllExtensions } from './extend_index_management';
import { PluginsDependencies, ClientConfigType } from './types';
import { ClientConfigType, SetupDependencies } from './types';
import { registerUrlGenerator } from './url_generator';

export class IndexLifecycleManagementPlugin {
constructor(private readonly initializerContext: PluginInitializerContext) {}

public setup(coreSetup: CoreSetup, plugins: PluginsDependencies) {
public setup(coreSetup: CoreSetup, plugins: SetupDependencies) {
const {
ui: { enabled: isIndexLifecycleManagementUiEnabled },
} = this.initializerContext.config.get<ClientConfigType>();
Expand All @@ -31,7 +32,7 @@ export class IndexLifecycleManagementPlugin {
getStartServices,
} = coreSetup;

const { usageCollection, management, indexManagement, home, cloud } = plugins;
const { usageCollection, management, indexManagement, home, cloud, share } = plugins;

// Initialize services even if the app isn't mounted, because they're used by index management extensions.
initHttp(http);
Expand Down Expand Up @@ -97,6 +98,8 @@ export class IndexLifecycleManagementPlugin {
if (indexManagement) {
addAllExtensions(indexManagement.extensionsService);
}

registerUrlGenerator(coreSetup, management, share);
}
}

Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/index_lifecycle_management/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ import { UsageCollectionSetup } from '../../../../src/plugins/usage_collection/p
import { ManagementSetup } from '../../../../src/plugins/management/public';
import { IndexManagementPluginSetup } from '../../index_management/public';
import { CloudSetup } from '../../cloud/public';
import { SharePluginSetup } from '../../../../src/plugins/share/public';

export interface PluginsDependencies {
export interface SetupDependencies {
usageCollection?: UsageCollectionSetup;
management: ManagementSetup;
cloud?: CloudSetup;
indexManagement?: IndexManagementPluginSetup;
home?: HomePublicPluginSetup;
share: SharePluginSetup;
}

export interface ClientConfigType {
Expand Down
83 changes: 83 additions & 0 deletions x-pack/plugins/index_lifecycle_management/public/url_generator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* 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 { CoreSetup } from 'kibana/public';
import { UrlGeneratorsDefinition } from '../../../../src/plugins/share/public/';
import {
getPoliciesListPath,
getPolicyCreatePath,
getPolicyEditPath,
} from './application/services/navigation';
import { MANAGEMENT_APP_ID } from '../../../../src/plugins/management/public';
import { SetupDependencies } from './types';
import { PLUGIN } from '../common/constants';

export const ILM_URL_GENERATOR_ID = 'ILM_URL_GENERATOR_ID';

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


export class IlmUrlGenerator implements UrlGeneratorsDefinition<typeof ILM_URL_GENERATOR_ID> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

constructor(private readonly getAppBasePath: (absolute?: boolean) => Promise<string>) {}

public readonly id = ILM_URL_GENERATOR_ID;

public readonly createUrl = async (state: IlmUrlGeneratorState): Promise<string> => {
switch (state.page) {
case ILM_PAGES.CREATE: {
return `${await this.getAppBasePath(!!state.absolute)}${getPolicyCreatePath()}`;
}
case ILM_PAGES.EDIT: {
return `${await this.getAppBasePath(!!state.absolute)}${getPolicyEditPath(
state.policyName
)}`;
}
case ILM_PAGES.LIST: {
return `${await this.getAppBasePath(!!state.absolute)}${getPoliciesListPath()}`;
}
}
};
}

export const registerUrlGenerator = (
coreSetup: CoreSetup,
management: SetupDependencies['management'],
share: SetupDependencies['share']
) => {
const getAppBasePath = async (absolute = false) => {
const [coreStart] = await coreSetup.getStartServices();
return coreStart.application.getUrlForApp(MANAGEMENT_APP_ID, {
path: management.sections.section.data.getApp(PLUGIN.ID)!.basePath,
absolute,
});
};

share.urlGenerators.registerUrlGenerator(new IlmUrlGenerator(getAppBasePath));
};
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ export type TestSubjects =
| 'createTemplateButton'
| 'dataStreamsEmptyPromptTemplateLink'
| 'dataStreamTable'
| 'dataStreamTable'
| 'deleteSystemTemplateCallOut'
| 'deleteTemplateButton'
| 'deleteTemplatesConfirmation'
| 'documentationLink'
| 'emptyPrompt'
| 'filterList.filterItem'
| 'ilmPolicyLink'
| 'includeStatsSwitch'
| 'indexTable'
| 'indexTableIncludeHiddenIndicesToggle'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { act } from 'react-dom/test-utils';
import { ReactWrapper } from 'enzyme';

import { EuiDescriptionListDescription } from '@elastic/eui';
import {
registerTestBed,
TestBed,
Expand All @@ -26,15 +27,17 @@ export interface DataStreamsTabTestBed extends TestBed<TestSubjects> {
clickReloadButton: () => void;
clickNameAt: (index: number) => void;
clickIndicesAt: (index: number) => void;
clickDeletActionAt: (index: number) => void;
clickDeleteActionAt: (index: number) => void;
clickConfirmDelete: () => void;
clickDeletDataStreamButton: () => void;
clickDeleteDataStreamButton: () => void;
};
findDeleteActionAt: (index: number) => ReactWrapper;
findDeleteConfirmationModal: () => ReactWrapper;
findDetailPanel: () => ReactWrapper;
findDetailPanelTitle: () => string;
findEmptyPromptIndexTemplateLink: () => ReactWrapper;
findDetailPanelIlmPolicyLink: () => ReactWrapper;
findDetailPanelIlmPolicyName: () => ReactWrapper;
}

export const setup = async (overridingDependencies: any = {}): Promise<DataStreamsTabTestBed> => {
Expand Down Expand Up @@ -115,7 +118,7 @@ export const setup = async (overridingDependencies: any = {}): Promise<DataStrea

const findDeleteActionAt = findTestSubjectAt.bind(null, 'deleteDataStream');

const clickDeletActionAt = (index: number) => {
const clickDeleteActionAt = (index: number) => {
findDeleteActionAt(index).simulate('click');
};

Expand All @@ -135,7 +138,7 @@ export const setup = async (overridingDependencies: any = {}): Promise<DataStrea
});
};

const clickDeletDataStreamButton = () => {
const clickDeleteDataStreamButton = () => {
const { find } = testBed;
find('deleteDataStreamButton').simulate('click');
};
Expand All @@ -150,6 +153,17 @@ export const setup = async (overridingDependencies: any = {}): Promise<DataStrea
return find('dataStreamDetailPanelTitle').text();
};

const findDetailPanelIlmPolicyLink = () => {
const { find } = testBed;
return find('ilmPolicyLink');
};

const findDetailPanelIlmPolicyName = () => {
const descriptionList = testBed.component.find(EuiDescriptionListDescription);
// ilm policy is the last in the details list
return descriptionList.last();
};

return {
...testBed,
actions: {
Expand All @@ -159,15 +173,17 @@ export const setup = async (overridingDependencies: any = {}): Promise<DataStrea
clickReloadButton,
clickNameAt,
clickIndicesAt,
clickDeletActionAt,
clickDeleteActionAt,
clickConfirmDelete,
clickDeletDataStreamButton,
clickDeleteDataStreamButton,
},
findDeleteActionAt,
findDeleteConfirmationModal,
findDetailPanel,
findDetailPanelTitle,
findEmptyPromptIndexTemplateLink,
findDetailPanelIlmPolicyLink,
findDetailPanelIlmPolicyName,
};
};

Expand Down
Loading