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

yuliacech · 2020-09-18T11:14:31Z

Summary

Fixes #75558.
This PR adds a url generator for Ingest Pipelines UI plugin that is registered in SharePlugin and can be used by other apps to deep-link to pages in Ingest Pipelines. For more information see README.md.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-09-18T11:26:47Z

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

cjcenizal

Great work, Yulia! I took a quick look and had some questions/suggestions, mostly regarding the interface.

cjcenizal · 2020-09-18T19:34:24Z

...k/plugins/ingest_pipelines/public/application/sections/pipelines_create/pipelines_create.tsx

@@ -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));


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, }));

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.

cjcenizal · 2020-09-18T19:39:49Z

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

+  return `${BASE_PATH}${CREATE_PATH}`;
+};
+
+const getClonePath = (name: string, encode = true): string => {


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.

cjcenizal · 2020-09-18T19:43:21Z

x-pack/plugins/ingest_pipelines/public/url_generator.test.ts

+  const urlGenerator = new IngestPipelinesUrlGenerator(getAppBasePath);
+
+  describe('Pipelines List', () => {
+    it('should generate correct relative url for list without pipelineId', async () => {


This is just a pet peeve of mine so feel free to disregard it. 😄 I noticed that every description contains the phrase should generate correct <result> and I think the words "should" and "correct' become a bit redundant for test descriptions. If you wanted to make them terser I think you could rephrase them as generates <result>.

cjcenizal · 2020-09-18T19:47:16Z

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

+
+  public readonly createUrl = async (state: IngestPipelinesUrlGeneratorState): Promise<string> => {
+    return `${await this.getAppBasePath(!!state.absolute)}${URL_GENERATOR[state.page](
+      state.pipelineId!


We're referring to this as pipelineId here but in navigation.ts the term name is used instead. Can we consistently use one or the other to make the connection clearer?

Hey @cjcenizal , I'm glad you noticed that! After a discussion with @darnautov and reading the docs, I figured out that the "official" term is pipeline_id. In Ingest Pipelines plugin code we refer to this property as pipelineName, so I decided to use name in internal functions and id for external use.

cjcenizal · 2020-09-18T19:53:07Z

x-pack/plugins/ingest_pipelines/public/url_generator.test.ts

+
+    it('should generate correct absolute url for pipeline clone', async () => {
+      const url = await urlGenerator.createUrl({
+        page: INGEST_PIPELINES_PAGES.CREATE,


Do consumers of the generator have access to INGEST_PIPELINES_PAGES? Is it defined on the public interface somewhere? I wonder why it's not exposed as part of the URL generator's interface, like this:

urlGenerator.createUrl({ page: urlGenerator.CREATE, pipelineId: 'pipeline_name', });

I guess I'm just confused about how consumers in another plugin will know which strings to pass in for the page property.

We've faced the same issue, it's not possible ATM to extend urlGenerator class, check

kibana/src/plugins/share/public/url_generators/url_generator_internal.ts

Line 70 in 543481b

getPublicContract(): UrlGeneratorContract<Id> {

I am currently exporting generator id, generator class, generator state and pages enum for external use in public.ts

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

I think, it should be enough for ML plugin to use this url generator, WDYT @darnautov ?

I don't think we need any of these TBH, because you register the URL generator during your Inget Pipeline plugin setup and we simply get it using the share. Typescript will validate the provided state as well.

yuliacech · 2020-09-21T13:40:31Z

@elasticmachine merge upstream

…vice

yuliacech · 2020-09-21T14:42:47Z

Hi @cjcenizal,
thanks a lot for your feedback and suggestions! I re-organized how public/url_generator.ts and public/application/services/navigation.ts files export object/functions. url_generator is intended for external users and now has INGEST_PIPELINES_PAGES enum (everything needed for external users is exported in public/index.ts). Would you mind having another look please?

cjcenizal

Code LGTM! Nice work! I had a couple of suggestions but they're not blockers.

cjcenizal · 2020-09-21T21:43:33Z

x-pack/plugins/ingest_pipelines/common/constants.ts

@@ -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';


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

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

That would be the Kibana App Arch team.

cjcenizal · 2020-09-21T21:47:01Z

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

+  return `${BASE_PATH}${name ? `?pipeline=${encodeURIComponent(name)}` : ''}`;
+};
+
+export const ROUTES_CONFIG = {


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.

cjcenizal · 2020-09-21T21:49:23Z

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

+  clone: _getClonePath(':sourceName', false),
+};
+
+export const getListPath = ({


Nit: what do we gain by exporting adapter functions instead of the original functions? For example:

// Setting a default for `encode` supports the simpler consumption pattern of not // having to specify it, and I don't think we lose anything by allowing the consumer // to specify `false`. export const getEditPath = ({ pipelineName, encode = true }: { pipelineName: string, encode?: boolean }): string => { return `${BASE_PATH}${EDIT_PATH}/${encode ? encodeURIComponent(name) : name}`; }; /* snip */ export const ROUTES_CONFIG = { edit: getEditPath({ pipelineName: ':name', encode: false }), /* snip */ };

I think that encoding the parameter is an implementation detail and should not be needed for clients using the navigation service, hence a separate ROUTES constant.

kibanamachine · 2020-09-22T20:02:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9b3e223

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
ingestPipelines	449	+2	447
management	46	+1	45
total		+3

async chunks size

id	value	diff	baseline
ingestPipelines	765.7KB	+116.0B	765.6KB

page load bundle size

id	value	diff	baseline
ingestPipelines	40.9KB	+8.2KB	32.7KB
management	30.4KB	+142.0B	30.2KB
total		+8.3KB

distributable file count

id	value	diff	baseline
default	45928	+1	45927
oss	26652	+1	26651
total		+2

History

💚 Build #75882 succeeded 56fa521
💔 Build #75870 failed 573c9f2
💔 Build #75852 failed 8173db2
💚 Build #75530 succeeded 833ecd0
💔 Build #75523 failed 14e92c1

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

ppisljar

app arch owned changes LGTM

…c#77872) * [Ingest Pipelines] Add url generator for ingest pipelines app * [Ingest Pipelines] Fix type check error * [Ingest Pipelines] Fix import errors * [Ingest Pipelines] Fix type check errors * [Ingest Pipelines] Fix type check errors * [ILM] Update UrlGenerator interface, clean up internal navigation service * [ILM] Fix function export * [ILM] Update functions signatures * [ILM] Fix errors * [ILM] Fix errors * [ILM] Rename ROUTES_CONFIG and export MANAGEMENT_APP_ID Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (31 commits) skip tests for old pacakge (elastic#78194) [Ingest Pipelines] Add url generator for ingest pipelines app (elastic#77872) [Lens] Rename "telemetry" to "stats" (elastic#78125) [CSM] Url search (elastic#77516) [Drilldowns] Config to disable URL Drilldown (elastic#77887) [Lens] Combined histogram/range aggregation for numbers (elastic#76121) Remove legacy plugins support (elastic#77599) 'Auto' interval must be correctly calculated for natural numbers (elastic#77995) [CSM] fix ingest data retry order messed up (elastic#78163) Add response status helpers (elastic#78006) Bump react-beautiful-dnd (elastic#78028) [Security Solution][Detection Engine] Bubbles up more error messages from ES queries to the UI (elastic#78004) Index pattern - refactor constructor (elastic#77791) Add `xpack.security.sameSiteCookies` to docker allow list (elastic#78192) Remove [key: string]: any; from IIndexPattern (elastic#77968) Remove requirement for manage_index_templates privilege for Index Management (elastic#77377) [Ingest Manager] Agent bulk actions UI (elastic#77690) [Metrics UI] Add inventory view timeline (elastic#77804) Reporting/Docs: Updates for setting to enable CSV Download (elastic#78101) Update to latest rum-react (elastic#78193) ...

#78278) * [Ingest Pipelines] Add url generator for ingest pipelines app * [Ingest Pipelines] Fix type check error * [Ingest Pipelines] Fix import errors * [Ingest Pipelines] Fix type check errors * [Ingest Pipelines] Fix type check errors * [ILM] Update UrlGenerator interface, clean up internal navigation service * [ILM] Fix function export * [ILM] Update functions signatures * [ILM] Fix errors * [ILM] Fix errors * [ILM] Rename ROUTES_CONFIG and export MANAGEMENT_APP_ID Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

[Ingest Pipelines] Add url generator for ingest pipelines app

858da91

yuliacech added Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0 labels Sep 18, 2020

yuliacech marked this pull request as ready for review September 18, 2020 11:26

yuliacech requested a review from a team as a code owner September 18, 2020 11:26

yuliacech added 4 commits September 18, 2020 13:47

[Ingest Pipelines] Fix type check error

2d065d0

[Ingest Pipelines] Fix import errors

65c5f58

[Ingest Pipelines] Fix type check errors

14e92c1

[Ingest Pipelines] Fix type check errors

833ecd0

yuliacech requested review from alisonelizabeth and darnautov September 18, 2020 14:58

cjcenizal reviewed Sep 18, 2020

View reviewed changes

elasticmachine and others added 5 commits September 21, 2020 09:40

Merge branch 'master' into ingest_url_gen

2e94ed2

[ILM] Update UrlGenerator interface, clean up internal navigation ser…

8173db2

…vice

[ILM] Fix function export

7c63503

[ILM] Update functions signatures

52f9396

[ILM] Fix errors

573c9f2

[ILM] Fix errors

56fa521

cjcenizal approved these changes Sep 21, 2020

View reviewed changes

[ILM] Rename ROUTES_CONFIG and export MANAGEMENT_APP_ID

9b3e223

yuliacech requested a review from a team as a code owner September 22, 2020 18:23

ppisljar approved these changes Sep 23, 2020

View reviewed changes

yuliacech merged commit 2c6cae7 into elastic:master Sep 23, 2020

yuliacech mentioned this pull request Sep 23, 2020

[7.x] [Ingest Pipelines] Add url generator for ingest pipelines app (#77872) #78278

Merged

yuliacech deleted the ingest_url_gen branch October 7, 2020 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yuliacech commented Sep 18, 2020 •

edited

Loading

elasticmachine commented Sep 18, 2020

cjcenizal left a comment

cjcenizal Sep 18, 2020

yuliacech Sep 21, 2020

cjcenizal Sep 18, 2020

cjcenizal Sep 18, 2020

cjcenizal Sep 18, 2020

yuliacech Sep 21, 2020

cjcenizal Sep 18, 2020

darnautov Sep 21, 2020

yuliacech Sep 21, 2020

darnautov Sep 22, 2020

yuliacech commented Sep 21, 2020

yuliacech commented Sep 21, 2020

cjcenizal left a comment

cjcenizal Sep 21, 2020

yuliacech Sep 22, 2020

cjcenizal Sep 22, 2020

cjcenizal Sep 21, 2020

cjcenizal Sep 21, 2020

yuliacech Sep 22, 2020

kibanamachine commented Sep 22, 2020

ppisljar left a comment

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

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

Conversation

yuliacech commented Sep 18, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Sep 18, 2020

cjcenizal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuliacech commented Sep 21, 2020

yuliacech commented Sep 21, 2020

cjcenizal 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

kibanamachine commented Sep 22, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

ppisljar left a comment

Choose a reason for hiding this comment

yuliacech commented Sep 18, 2020 •

edited

Loading