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

[Ingest Pipeline] Provide url params to use ingestPipeline UI from Fleet #134776

Merged
merged 4 commits into from
Jun 21, 2022

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Jun 20, 2022

Summary

Related to #133740

In the Fleet UI we will link to the ingest pipeline UI to show, edit or create a new pipeline and we want users to be redirected to Fleet after interacting with ingest pipeline app.

That PR allow to do so by introducing a redirect_path query parameter for all the CRUD ingest pipeline pages and a name parameter for the create pipeline page (when the name parameter is provided name is not editable).

Work in progress

How to manually test that PR

Until the workflow is completely implemented in Fleet you can test it like this

  1. Navigate to /app/management/ingest/ingest_pipelines/create?name=logs-system.syslog@custom&redirect_path=/app/fleet the name should be prefilled and not editable

Screen Shot 2022-06-20 at 2 39 48 PM

On save you should be redirected to fleet

  1. then you can test the following urls and you should be redirect in fleet on save or close
    /app/management/ingest/ingest_pipelines/?pipeline=logs-system.syslog@custom&redirect_path=/app/fleet
    /app/management/ingest/ingest_pipelines/edit/logs-system.syslog@custom?redirect_path=/app/fleet

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes v8.4.0 labels Jun 20, 2022
@nchaulet nchaulet self-assigned this Jun 20, 2022
@nchaulet nchaulet force-pushed the feature-ingest-pipeline-redirect-path branch from 50b6d43 to 70c32bc Compare June 20, 2022 18:17
@nchaulet nchaulet force-pushed the feature-ingest-pipeline-redirect-path branch from 70c32bc to 20c2a8f Compare June 20, 2022 18:35
@nchaulet nchaulet marked this pull request as ready for review June 20, 2022 18:42
@nchaulet nchaulet requested a review from a team as a code owner June 20, 2022 18:42
@nchaulet nchaulet added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 20, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Hi @nchaulet! Thanks for taking this on.

I left a couple comments in the code around naming. Let me know what you think.

I also have a few questions after testing:

  • How will the custom pipeline names be generated from Fleet? Will we ever run into a situation where the user is taken to the create pipeline page, and the pipeline already exists? Seems like that could be a poor UX since the name is not editable, but I don't have a clear understanding of the full flow.

Screen Shot 2022-06-21 at 8 48 35 AM

  • Once this is implemented on the Fleet side, will the redirect_path include the base path, i.e., /czy/app/fleet instead of /app/fleet? The current example in your PR description returns a 404.

Screen Shot 2022-06-21 at 8 51 34 AM

  • What is the expected behavior for the following comment? I currently see a 404 and the query param displays in the flyout.

then you can test the following urls and you should be redirect in fleet on save or close
/app/management/ingest/ingest_pipelines/?pipeline=logs-system.syslog@custom?redirect_path=/app/fleet

Screen Shot 2022-06-21 at 8 50 56 AM

@@ -27,6 +27,7 @@ export interface PipelineFormProps {
saveError: any;
defaultValue?: Pipeline;
isEditing?: boolean;
canEditName?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, I can't think of many use cases where we would want to disallow editing the pipeline name on create (generally speaking, what's allowed via the ES API we don't block in the UI). That said, do you think it makes more sense to name this prop specific to fleet, something like isIntegrationPipeline?

Or, if we leave as-is, I think it might be helpful to add a code comment explaining its usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am in favor of adding a comment here, to avoid to much fleet specifity here.

@alisonelizabeth
Copy link
Contributor

One more thing - do you think you could add client integration tests for this flow as well? Let me know if you need any help with this. Thanks!

@nchaulet
Copy link
Member Author

@alisonelizabeth thanks for the review

How will the custom pipeline names be generated from Fleet? Will we ever run into a situation where the user is taken to the create pipeline page, and the pipeline already exists? Seems like that could be a poor UX since the name is not editable, but I don't have a clear understanding of the full flow.

we have a convention to create the name in Fleet, and we will check if the custom pipeline exists before to show the user a create button or an edit one and send it to the correct ingest pipeline page.

Once this is implemented on the Fleet side, will the redirect_path include the base path, i.e., /czy/app/fleet instead of /app/fleet? The current example in your PR description returns a 404.

Yes the redirect_path in Fleet will include the bath path, my bad for the example I am testing without a basepath

What is the expected behavior for the following comment? I currently see a 404 and the query param displays in the flyout.

I made a mistake in the URL query parameter should be fixed

@nchaulet nchaulet requested a review from alisonelizabeth June 21, 2022 14:24
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback! I did not retest, but code changes LGTM.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ingestPipelines 457 459 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ingestPipelines 451.7KB 452.3KB +611.0B

History

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

cc @nchaulet

@nchaulet nchaulet merged commit 04c8fb9 into elastic:main Jun 21, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2022
@nchaulet nchaulet deleted the feature-ingest-pipeline-redirect-path branch June 21, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants