Skip to content

[Ingest Pipelines] Add url generator for ingest pipelines app #77872

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

Merged
merged 12 commits into from
Sep 23, 2020
Merged

[Ingest Pipelines] Add url generator for ingest pipelines app #77872

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
858da91
[Ingest Pipelines] Add url generator for ingest pipelines app
yuliacech Sep 18, 2020
2d065d0
[Ingest Pipelines] Fix type check error
yuliacech Sep 18, 2020
65c5f58
[Ingest Pipelines] Fix import errors
yuliacech Sep 18, 2020
14e92c1
[Ingest Pipelines] Fix type check errors
yuliacech Sep 18, 2020
833ecd0
[Ingest Pipelines] Fix type check errors
yuliacech Sep 18, 2020
2e94ed2
Merge branch 'master' into ingest_url_gen
elasticmachine Sep 21, 2020
8173db2
[ILM] Update UrlGenerator interface, clean up internal navigation ser…
yuliacech Sep 21, 2020
7c63503
[ILM] Fix function export
yuliacech Sep 21, 2020
52f9396
[ILM] Update functions signatures
yuliacech Sep 21, 2020
573c9f2
[ILM] Fix errors
yuliacech Sep 21, 2020
56fa521
[ILM] Fix errors
yuliacech Sep 21, 2020
9b3e223
[ILM] Rename ROUTES_CONFIG and export MANAGEMENT_APP_ID
yuliacech Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...k/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipelines_clone.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@
*/

import { registerTestBed, TestBedConfig, TestBed } from '../../../../../test_utils';
import { BASE_PATH } from '../../../common/constants';
import { PipelinesClone } from '../../../public/application/sections/pipelines_clone';
import { getFormActions, PipelineFormTestSubjects } from './pipeline_form.helpers';
import { WithAppDependencies } from './setup_environment';
import {
INGEST_PIPELINES_PAGES,
ROUTES_CONFIG,
URL_GENERATOR,
} from '../../../public/application/services/navigation';

export type PipelinesCloneTestBed = TestBed<PipelineFormTestSubjects> & {
actions: ReturnType<typeof getFormActions>;
Expand All @@ -29,8 +33,8 @@ export const PIPELINE_TO_CLONE = {

const testBedConfig: TestBedConfig = {
memoryRouter: {
initialEntries: [`${BASE_PATH}create/${PIPELINE_TO_CLONE.name}`],
componentRoutePath: `${BASE_PATH}create/:name`,
initialEntries: [URL_GENERATOR[INGEST_PIPELINES_PAGES.CLONE](PIPELINE_TO_CLONE.name)],
componentRoutePath: ROUTES_CONFIG[INGEST_PIPELINES_PAGES.CLONE],
},
doMountAsync: true,
};
Expand Down
10 changes: 7 additions & 3 deletions .../plugins/ingest_pipelines/__jest__/client_integration/helpers/pipelines_create.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,23 @@
*/

import { registerTestBed, TestBedConfig, TestBed } from '../../../../../test_utils';
import { BASE_PATH } from '../../../common/constants';
import { PipelinesCreate } from '../../../public/application/sections/pipelines_create';
import { getFormActions, PipelineFormTestSubjects } from './pipeline_form.helpers';
import { WithAppDependencies } from './setup_environment';
import {
INGEST_PIPELINES_PAGES,
ROUTES_CONFIG,
URL_GENERATOR,
} from '../../../public/application/services/navigation';

export type PipelinesCreateTestBed = TestBed<PipelineFormTestSubjects> & {
actions: ReturnType<typeof getFormActions>;
};

const testBedConfig: TestBedConfig = {
memoryRouter: {
initialEntries: [`${BASE_PATH}/create`],
componentRoutePath: `${BASE_PATH}/create`,
initialEntries: [URL_GENERATOR[INGEST_PIPELINES_PAGES.CREATE]()],
componentRoutePath: ROUTES_CONFIG[INGEST_PIPELINES_PAGES.CREATE],
},
doMountAsync: true,
};
Expand Down
10 changes: 7 additions & 3 deletions ...ck/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipelines_edit.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@
*/

import { registerTestBed, TestBedConfig, TestBed } from '../../../../../test_utils';
import { BASE_PATH } from '../../../common/constants';
import { PipelinesEdit } from '../../../public/application/sections/pipelines_edit';
import { getFormActions, PipelineFormTestSubjects } from './pipeline_form.helpers';
import { WithAppDependencies } from './setup_environment';
import {
INGEST_PIPELINES_PAGES,
ROUTES_CONFIG,
URL_GENERATOR,
} from '../../../public/application/services/navigation';

export type PipelinesEditTestBed = TestBed<PipelineFormTestSubjects> & {
actions: ReturnType<typeof getFormActions>;
Expand All @@ -29,8 +33,8 @@ export const PIPELINE_TO_EDIT = {

const testBedConfig: TestBedConfig = {
memoryRouter: {
initialEntries: [`${BASE_PATH}edit/${PIPELINE_TO_EDIT.name}`],
componentRoutePath: `${BASE_PATH}edit/:name`,
initialEntries: [URL_GENERATOR[INGEST_PIPELINES_PAGES.EDIT](PIPELINE_TO_EDIT.name)],
componentRoutePath: ROUTES_CONFIG[INGEST_PIPELINES_PAGES.EDIT],
},
doMountAsync: true,
};
Expand Down
10 changes: 7 additions & 3 deletions ...ck/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipelines_list.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import { act } from 'react-dom/test-utils';

import { BASE_PATH } from '../../../common/constants';
import {
registerTestBed,
TestBed,
Expand All @@ -16,11 +15,16 @@ import {
} from '../../../../../test_utils';
import { PipelinesList } from '../../../public/application/sections/pipelines_list';
import { WithAppDependencies } from './setup_environment';
import {
INGEST_PIPELINES_PAGES,
ROUTES_CONFIG,
URL_GENERATOR,
} from '../../../public/application/services/navigation';

const testBedConfig: TestBedConfig = {
memoryRouter: {
initialEntries: [BASE_PATH],
componentRoutePath: BASE_PATH,
initialEntries: [URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST]()],
componentRoutePath: ROUTES_CONFIG[INGEST_PIPELINES_PAGES.LIST],
},
doMountAsync: true,
};
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/ingest_pipelines/common/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ const basicLicense: LicenseType = 'basic';

export const PLUGIN_ID = 'ingest_pipelines';

export const PLUGIN_MIN_LICENSE_TYPE = basicLicense;
export const MANAGEMENT_APP_ID = 'management';
Copy link
Contributor

@cjcenizal cjcenizal Sep 21, 2020

Choose a reason for hiding this comment

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

This seems like something the management plugin should define inside a common/constants.ts file and export in https://github.com/elastic/kibana/blob/master/src/plugins/management/public/index.ts. Since the management plugin is already a dependency of this plugin, it seems like it would be straightforward to make that change and consume the constant wherever we need it like this:

import { MANAGEMENT_APP_ID } from 'src/plugins/management/public';

The only slightly invasive change would be you'd have to update the management plugin to consume this constant on this line: https://github.com/elastic/kibana/blob/master/src/plugins/management/public/plugin.ts#L75

Copy link
Contributor Author

@yuliacech yuliacech Sep 22, 2020

Choose a reason for hiding this comment

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

Yeah, I was looking at the management app for an exported constant but couldn't find any. So I added an export, do you know maybe who would be a code owner for this plugin? @cjcenizal

Copy link
Contributor

@cjcenizal cjcenizal Sep 22, 2020

Choose a reason for hiding this comment

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

That would be the Kibana App Arch team.


export const BASE_PATH = '/';
export const PLUGIN_MIN_LICENSE_TYPE = basicLicense;

export const API_BASE_PATH = '/api/ingest_pipelines';

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/ingest_pipelines/kibana.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "8.0.0",
"server": true,
"ui": true,
"requiredPlugins": ["licensing", "management", "features"],
"requiredPlugins": ["licensing", "management", "features", "share"],
"optionalPlugins": ["security", "usageCollection"],
"configPath": ["xpack", "ingest_pipelines"],
"requiredBundles": ["esUiShared", "kibanaReact"]
Expand Down
9 changes: 5 additions & 4 deletions x-pack/plugins/ingest_pipelines/public/application/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import {
} from '../shared_imports';

import { PipelinesList, PipelinesCreate, PipelinesEdit, PipelinesClone } from './sections';
import { INGEST_PIPELINES_PAGES, ROUTES_CONFIG } from './services/navigation';

export const AppWithoutRouter = () => (
<Switch>
<Route exact path="/" component={PipelinesList} />
<Route exact path={`/create/:sourceName`} component={PipelinesClone} />
<Route exact path={`/create`} component={PipelinesCreate} />
<Route exact path={`/edit/:name`} component={PipelinesEdit} />
<Route exact path={ROUTES_CONFIG[INGEST_PIPELINES_PAGES.LIST]} component={PipelinesList} />
<Route exact path={ROUTES_CONFIG[INGEST_PIPELINES_PAGES.CLONE]} component={PipelinesClone} />
<Route exact path={ROUTES_CONFIG[INGEST_PIPELINES_PAGES.CREATE]} component={PipelinesCreate} />
<Route exact path={ROUTES_CONFIG[INGEST_PIPELINES_PAGES.EDIT]} component={PipelinesEdit} />
{/* Catch all */}
<Route component={PipelinesList} />
</Switch>
Expand Down
6 changes: 3 additions & 3 deletions ...lugins/ingest_pipelines/public/application/sections/pipelines_create/pipelines_create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
EuiSpacer,
} from '@elastic/eui';

import { BASE_PATH } from '../../../../common/constants';
import { INGEST_PIPELINES_PAGES, URL_GENERATOR } from '../../services/navigation';
import { Pipeline } from '../../../../common/types';
import { useKibana } from '../../../shared_imports';
import { PipelineForm } from '../../components';
Expand Down Expand Up @@ -50,11 +50,11 @@ export const PipelinesCreate: React.FunctionComponent<RouteComponentProps & Prop
return;
}

history.push(BASE_PATH + `?pipeline=${encodeURIComponent(pipeline.name)}`);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST](pipeline.name));
Copy link
Contributor

@cjcenizal cjcenizal Sep 18, 2020

Choose a reason for hiding this comment

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

This seems like a bit of an awkward interface because I have to import two things, combine them in the right way, and then infer that they map to a function which I then call. It seems like a more intuitive interface would be to import a function and then call that.

import { getListPath } from '../../services/navigation';

/* ... */

history.push(getListPath(pipeline.name));

WDYT?

EDIT: I just got to the end of the PR and saw the URL generator, which implements an interface similar to what I'm suggesting. Is it possible to use the generator internally?

import { urlGenerator } from '../../services/navigation';

/* ... */

history.push(getListPath(pipeline.name));
history.push(urlGenerator.createUrl({
  page: urlGenerator.LIST,
  pipelineId: pipeline.name,
}));

Copy link
Contributor Author

@yuliacech yuliacech Sep 21, 2020

Choose a reason for hiding this comment

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

I agree, the import is too complicated, I changed it to a function export for internal use. I'm not sure UrlGenerator should be used internally, since we don't need an absolute path and createUrl is async.

};

const onCancel = () => {
history.push(BASE_PATH);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST]());
};

useEffect(() => {
Expand Down
8 changes: 4 additions & 4 deletions ...ck/plugins/ingest_pipelines/public/application/sections/pipelines_edit/pipelines_edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import {
} from '@elastic/eui';

import { EuiCallOut } from '@elastic/eui';
import { BASE_PATH } from '../../../../common/constants';
import { Pipeline } from '../../../../common/types';
import { useKibana, SectionLoading } from '../../../shared_imports';
import { PipelineForm } from '../../components';

import { INGEST_PIPELINES_PAGES, URL_GENERATOR } from '../../services/navigation';
import { PipelineForm } from '../../components';
import { attemptToURIDecode } from '../shared';

interface MatchParams {
Expand Down Expand Up @@ -56,11 +56,11 @@ export const PipelinesEdit: React.FunctionComponent<RouteComponentProps<MatchPar
return;
}

history.push(BASE_PATH + `?pipeline=${encodeURIComponent(updatedPipeline.name)}`);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST](updatedPipeline.name));
};

const onCancel = () => {
history.push(BASE_PATH);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST]());
};

useEffect(() => {
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/ingest_pipelines/public/application/sections/pipelines_list/empty_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { useHistory } from 'react-router-dom';
import { ScopedHistory } from 'kibana/public';
import { reactRouterNavigate } from '../../../../../../../src/plugins/kibana_react/public';
import { useKibana } from '../../../shared_imports';
import { INGEST_PIPELINES_PAGES, URL_GENERATOR } from '../../services/navigation';

export const EmptyList: FunctionComponent = () => {
const { services } = useKibana();
Expand Down Expand Up @@ -44,7 +45,11 @@ export const EmptyList: FunctionComponent = () => {
</p>
}
actions={
<EuiButton {...reactRouterNavigate(history, '/create')} iconType="plusInCircle" fill>
<EuiButton
{...reactRouterNavigate(history, URL_GENERATOR[INGEST_PIPELINES_PAGES.CREATE]())}
iconType="plusInCircle"
fill
>
{i18n.translate('xpack.ingestPipelines.list.table.emptyPrompt.createButtonLabel', {
defaultMessage: 'Create a pipeline',
})}
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/ingest_pipelines/public/application/sections/pipelines_list/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ import {
} from '@elastic/eui';

import { Pipeline } from '../../../../common/types';
import { BASE_PATH } from '../../../../common/constants';
import { useKibana, SectionLoading } from '../../../shared_imports';
import { UIM_PIPELINES_LIST_LOAD } from '../../constants';
import { INGEST_PIPELINES_PAGES, URL_GENERATOR } from '../../services/navigation';

import { EmptyList } from './empty_list';
import { PipelineTable } from './table';
Expand Down Expand Up @@ -68,16 +68,16 @@ export const PipelinesList: React.FunctionComponent<RouteComponentProps> = ({
}, [pipelineNameFromLocation, data]);

const goToEditPipeline = (name: string) => {
history.push(`${BASE_PATH}/edit/${encodeURIComponent(name)}`);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.EDIT](name));
};

const goToClonePipeline = (name: string) => {
history.push(`${BASE_PATH}/create/${encodeURIComponent(name)}`);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.CLONE](name));
};

const goHome = () => {
setShowFlyout(false);
history.push(BASE_PATH);
history.push(URL_GENERATOR[INGEST_PIPELINES_PAGES.LIST]());
};

if (data && data.length === 0) {
Expand Down
48 changes: 48 additions & 0 deletions x-pack/plugins/ingest_pipelines/public/application/services/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* 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.
*/

const BASE_PATH = '/';

const EDIT_PATH = 'edit';

const CREATE_PATH = 'create';

const getEditPath = (name: string, encode = true): string => {
return `${BASE_PATH}${EDIT_PATH}/${encode ? encodeURIComponent(name) : name}`;
};

const getCreatePath = (): string => {
return `${BASE_PATH}${CREATE_PATH}`;
};

const getClonePath = (name: string, encode = true): string => {
Copy link
Contributor

@cjcenizal cjcenizal Sep 18, 2020

Choose a reason for hiding this comment

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

I think we can improve the readability of the code by using config parameters for these path factories. Consider the difference:

// current
getListPath(updatedPipeline.name);
getClonePath(':sourceName', false);

// my suggestion
getListPath({ inspectedPipeline: updatedPipeline.name });
getClonePath({ clonedPipeline: ':sourceName', skipEncode: true });

I think it generally makes sense to avoid config parameters when you're only passing in one or two arguments, but I think in this situation it's very helpful for communicating the intention of the code.

return `${BASE_PATH}${CREATE_PATH}/${encode ? encodeURIComponent(name) : name}`;
};
const getListPath = (name?: string): string => {
return `${BASE_PATH}${name ? `?pipeline=${encodeURIComponent(name)}` : ''}`;
};

export enum INGEST_PIPELINES_PAGES {
LIST = 'pipelines_list',
EDIT = 'pipeline_edit',
CREATE = 'pipeline_create',
CLONE = 'pipeline_clone',
}

export const ROUTES_CONFIG = {
Copy link
Contributor

@cjcenizal cjcenizal Sep 21, 2020

Choose a reason for hiding this comment

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

Nit: I'm not sure the CONFIG suffix adds more information, since personally I'd infer ROUTES to be a constant, and from the name it would contain the various routes of the app.

[INGEST_PIPELINES_PAGES.LIST]: getListPath(),
[INGEST_PIPELINES_PAGES.EDIT]: getEditPath(':name', false),
[INGEST_PIPELINES_PAGES.CREATE]: getCreatePath(),
[INGEST_PIPELINES_PAGES.CLONE]: getClonePath(':sourceName', false),
};

export const URL_GENERATOR = {
[INGEST_PIPELINES_PAGES.LIST]: (selectedPipelineName?: string) =>
getListPath(selectedPipelineName),
[INGEST_PIPELINES_PAGES.EDIT]: (pipelineName: string) => getEditPath(pipelineName, true),
[INGEST_PIPELINES_PAGES.CREATE]: getCreatePath,
[INGEST_PIPELINES_PAGES.CLONE]: (pipelineName: string) => getClonePath(pipelineName, true),
};
7 changes: 7 additions & 0 deletions x-pack/plugins/ingest_pipelines/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,10 @@ import { IngestPipelinesPlugin } from './plugin';
export function plugin() {
return new IngestPipelinesPlugin();
}

export {
INGEST_PIPELINES_APP_ULR_GENERATOR,
IngestPipelinesUrlGeneratorState,
} from './url_generator';

export { INGEST_PIPELINES_PAGES } from './application/services/navigation';
5 changes: 4 additions & 1 deletion x-pack/plugins/ingest_pipelines/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import { CoreSetup, Plugin } from 'src/core/public';
import { PLUGIN_ID } from '../common/constants';
import { uiMetricService, apiService } from './application/services';
import { Dependencies } from './types';
import { registerUrlGenerator } from './url_generator';

export class IngestPipelinesPlugin implements Plugin {
public setup(coreSetup: CoreSetup, plugins: Dependencies): void {
const { management, usageCollection } = plugins;
const { management, usageCollection, share } = plugins;
const { http, getStartServices } = coreSetup;

// Initialize services
Expand Down Expand Up @@ -46,6 +47,8 @@ export class IngestPipelinesPlugin implements Plugin {
};
},
});

registerUrlGenerator(coreSetup, management, share);
}

public start() {}
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/ingest_pipelines/public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@

import { ManagementSetup } from 'src/plugins/management/public';
import { UsageCollectionSetup } from 'src/plugins/usage_collection/public';
import { SharePluginSetup } from '../../../../src/plugins/share/public';

export interface Dependencies {
management: ManagementSetup;
usageCollection: UsageCollectionSetup;
share: SharePluginSetup;
}
Loading