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 Pipelines] Pipeline Editor MVP #63617

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Apr 15, 2020

Summary

First iteration of pipeline editor.

Notes to reviewer

  • Main focus has been establishing patterns for us to flesh out all the other processor forms.

  • All styling optimisations have been deferred until after this review and once final designs have been agreed upon.

  • Translations have been done or marked as TODO throughout (please flag any ones that I may have missed).

  • The data has not been flattened to a byId map which keeps the reordering and rendering logic a bit simpler for now. Optimisation other than flattening to a byId map is still possible, which I would like to explore a bit later on but for now, worst case O(n) on an array that should never grow past 50-60 (guessing?) items is acceptable IMO.

  • Have not built rendering of on failure pipelines yet. There are some things that are still in flux here, however I can definitely work on just rending the nested list next.

Screenshots

Main View

Screenshot 2020-04-16 at 11 59 00

React DnD 🎉

Screenshot 2020-04-16 at 11 59 21

Processor form

Screenshot 2020-04-16 at 11 59 29

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

…bana into pipeline-editor-part-mvp-1

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana:
  [Ingest pipelines] Edit pipeline page (elastic#63522)

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/sections/pipelines_create/pipelines_create.tsx
#	x-pack/plugins/ingest_pipelines/public/shared_imports.ts
@jloleysens jloleysens marked this pull request as ready for review April 16, 2020 10:13
@jloleysens jloleysens requested a review from a team as a code owner April 16, 2020 10:13
@jloleysens jloleysens 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 labels Apr 16, 2020
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

History

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

<UseField config={typeConfig} path={'type'} defaultValue={initialType}>
{typeField => {
return (
<EuiComboBox
Copy link
Contributor

@sebelga sebelga Apr 20, 2020

Choose a reason for hiding this comment

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

There is an <EuiComboBoxField /> that already takes care of serialization, deserialization, validation at the array level, validation at the item level. I would recommend using it.

This is the config (from the schema) where you can see the 2 types of validations: at the Array level (the first one), and at the item level (the second one, declaring a type: VALIDATION_TYPES.ARRAY_ITEM).

https://github.com/elastic/kibana/blob/master/x-pack/plugins/index_management/public/application/components/template_form/template_form_schemas.tsx#L100

And this is how it is simply declared

https://github.com/elastic/kibana/blob/master/x-pack/plugins/index_management/public/application/components/template_form/steps/step_logistics.tsx#L127

No need to re-write the logic of onChange, options, selectedOptions.

The important part is to not forget to declare the defaultValue as [] (empty array).

} from '../../../../../shared_imports';

const { emptyField } = fieldValidators;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you prefer to declare all the configs above the component instead of inside a FormSchema? I would find it easier to scan to have all the fields declaration in a single place, but that's my preference 😊

label: i18n.translate('xpack.ingestPipelines.pipelineEditor.gsubForm.patternFieldLabel', {
defaultMessage: 'Pattern',
}),
validations: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this field is for index pattern or not, but in case it is, know that there is an indexPatternField validator for them.

https://github.com/elastic/kibana/blob/master/src/plugins/es_ui_shared/static/forms/helpers/field_validators/index_pattern_field.ts#L26

});

useEffect(() => {
const subscription = form.subscribe(({ data }) => {
Copy link
Contributor

@sebelga sebelga Apr 20, 2020

Choose a reason for hiding this comment

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

Instead of this + having to declare a state and update it, it is better to use the FormDataProvider

return (
  <Form form={form}>
    <ProcessorTypeField initialType={processor?.type} />
    <FormDataProvider pathsToWatch="type">
      {({ type }) => {
        const FormFields = getProcessorFormDescriptor(type as any);
        // TODO: Handle this error in a different way
        if (!FormFields) {
          throw new Error(`Could not find form for type ${type}`);
        }
        return (
          <>
            <FormFields />
            <CommonProcessorFields />
            <EuiButton onClick={form.submit}>Submit</EuiButton>
          </>
        );
      }}

    </FormDataProvider>
  </Form>
);

@jloleysens
Copy link
Contributor Author

Closing this PR in favor of #63694.

@sebelga I will implement your feedback on the other PR, thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants