-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 4 commits
8b330dc
d831c7f
51d2f9a
88cdad7
4089344
c150f0a
fa920cd
4412f9c
7589efa
3859247
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = { | ||
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`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of replacing this with |
||
}; | ||
|
||
export const getPoliciesListPath = () => { | ||
return '/policies'; | ||
}; |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably would have used a factory pattern here to capture 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)); | ||
}; |
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.
Calling this const
REACT_ROUTER_ROUTES
could make this even clearer. Since the strings are coupled to how react router works.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'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.