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.

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

Jump to bottom

[Ingest] Data source configuration validation UI #61180

Merged
merged 6 commits into from
Apr 8, 2020
Merged

[Ingest] Data source configuration validation UI #61180

merged 6 commits into from
Apr 8, 2020

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Mar 24, 2020 •
edited
Loading

Resolves #60446

Summary

This PR adds validation to data source configuration UI. It validates:

  • Data source name not empty
  • Input config fields
  • Stream config fields
  • Each config field is validated based its corresponding registry definition (based on its type, required, multi)
    • It will not be validated if it is disabled (due to parent stream or parent input being disabled)

Unit tests for the validation methods are included in this PR.

UI behavior

  1. Validation state for each field will kick in onBlur (aka field dirty/touched state)

  1. If the user attempts to submit while having invalid fields, a callout is displayed and erroneous sections are highlighted:

  1. Validation is removed if the user disables parent stream/input:

More screenshots

Expand me!

.
Required fields validation:

image

YAML field validation:

image

If user clicks Continue with invalid fields, a callout is displayed above the button, and the panels that need fixing are highlighted:

image

image

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project Feature:Fleet Fleet team's agent central management project v7.7.0 labels Mar 24, 2020
@jen-huang jen-huang self-assigned this Mar 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Feature:Fleet)

@jen-huang jen-huang marked this pull request as ready for review March 25, 2020 22:48
@jen-huang jen-huang requested a review from a team March 25, 2020 22:48
@jen-huang jen-huang mentioned this pull request Mar 25, 2020
Closed
22 tasks
@jen-huang jen-huang added v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team and removed v7.7.0 labels Mar 26, 2020
@hbharding
Copy link
Contributor

hbharding commented Mar 26, 2020 •
edited
Loading

Thanks for thinking through this Jen! I put together a mockup with notes to show validation errors in a variety of states (expanded/collapsed). Please note that the input and stream's advanced options toggle should always expand to show the invalid form field when the user submits the form. Also note, the mockup uses the latest design / layout, but you should just focus on the validation aspects, and don't worry matching the overall design.

You can view the mockup here in Figma: https://www.figma.com/file/0IiPRxG8NKuLYjIJDn9Nuk/Ingest-Manager-Screens?node-id=1828%3A104574

One small thing I noticed. If you collapse an advanced option which has an invalid input, it doesn't appear as invalid when you expand the advanced options again. Would it be possible to validate the form field when advanced options is expanded so that invalid form fields are highlighted? I noticed this behavior only happens when you haven't submitted the form yet.

image

nchaulet
nchaulet reviewed Mar 30, 2020
View reviewed changes
.../ingest_manager/sections/agent_config/create_datasource_page/services/validate_datasource.ts
* Returns validation information for a given datasource configuration and package info
* Note: this method assumes that `datasource` is correctly structured for the given package
*/
export const validateDatasource = (
Copy link
Member

Choose a reason for hiding this comment

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

This function seems a good candidate for a test

Copy link
Contributor Author

@jen-huang jen-huang Apr 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There is a validate_datasource.test.ts file (github doesn't expand that file diff automatically 😄)

nchaulet
nchaulet approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jen-huang jen-huang mentioned this pull request Apr 8, 2020
Merged
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jen-huang
Copy link
Contributor Author

@hbharding FYI I'm merging this PR now as-is to unblock downstream PRs, I'll make the visual changes you mocked up in a subsequent PR. Thanks!

@jen-huang jen-huang merged commit 86a2587 into elastic:master Apr 8, 2020
@jen-huang jen-huang deleted the ingest/ds-validation branch April 8, 2020 20:33
jen-huang added a commit to jen-huang/kibana that referenced this pull request Apr 8, 2020
@jen-huang
[Ingest] Data source configuration validation UI (elastic#61180)
3ecc632 
* Initial pass at datasource configuration validation

* Show error icon and red text at input and stream levels

* Add tests, fix bugs in validation method

* Fix typings
@jen-huang jen-huang mentioned this pull request Apr 8, 2020
Merged
jen-huang added a commit that referenced this pull request Apr 8, 2020
@jen-huang
[Ingest] Data source configuration validation UI (#61180) (#63022)
f5c8904 
* Initial pass at datasource configuration validation

* Show error icon and red text at input and stream levels

* Add tests, fix bugs in validation method

* Fix typings
@jen-huang jen-huang mentioned this pull request Apr 8, 2020
Closed
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 9, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into es_ui_shared/…
43763b0 
…chore/put-all-xjson-together

* 'master' of github.com:elastic/kibana: (35 commits)
  [SIEM] [Detection Engine] Fixes bug when notification doesn't… (elastic#63013)
  [SIEM][Detection Engine] Fix rule notification critical bugs
  Add Error Exception Type Column (elastic#59596)
  [APM] Agent remote configuration: changes in Java property descriptions (elastic#62282)
  [Alerting] Displays warning when a permanent encryption key is missing and hides alerting UI appropriately (elastic#62772)
  FTR: add chromium-based Edge browser support (elastic#61684)
  [Ingest] Data source configuration validation UI (elastic#61180)
  restore empty_kibana after saved objects test (elastic#62951)
  Index pattern management plugin - src/legacy/core_plugins/management => new platform plugin (elastic#62594)
  Add basic StatusService (elastic#60335)
  [kbn/optimizer] link to kibanaReact/kibanaUtils plugins (elastic#62720)
  [APM] Service map - fixes layout issues for maps with no rum services (elastic#62887)
  Exclude disabled datasources and streams from agent config (elastic#62869)
  [Alerting] Fix validation support for nested IErrorObjects (elastic#62833)
  [Metrics UI] Invalidate non-count alerts which have no metrics (elastic#62837)
  Add --filter option to API docs script (elastic#62888)
  [Maps] fix attribution overflow with exit full screen button (elastic#62699)
  [Uptime]Alerting UI text in case filter is selected (elastic#62570)
  [Maps] Show create filter button for top-term tooltip property (elastic#62461)
  skip flaky suite (elastic#59030)
  ...

# Conflicts:
#	src/plugins/es_ui_shared/public/index.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 9, 2020
@gmmorris
Merge branch 'master' into alerting/es-query
e9106a8 
* master: (40 commits)
  [ML] Functional transform tests - stabilize source selection (elastic#63087)
  add embed flag to saved object url as well (elastic#62926)
  [SIEM] [Detection Engine] Fixes bug when notification doesn't… (elastic#63013)
  [SIEM][Detection Engine] Fix rule notification critical bugs
  Add Error Exception Type Column (elastic#59596)
  [APM] Agent remote configuration: changes in Java property descriptions (elastic#62282)
  [Alerting] Displays warning when a permanent encryption key is missing and hides alerting UI appropriately (elastic#62772)
  FTR: add chromium-based Edge browser support (elastic#61684)
  [Ingest] Data source configuration validation UI (elastic#61180)
  restore empty_kibana after saved objects test (elastic#62951)
  Index pattern management plugin - src/legacy/core_plugins/management => new platform plugin (elastic#62594)
  Add basic StatusService (elastic#60335)
  [kbn/optimizer] link to kibanaReact/kibanaUtils plugins (elastic#62720)
  [APM] Service map - fixes layout issues for maps with no rum services (elastic#62887)
  Exclude disabled datasources and streams from agent config (elastic#62869)
  [Alerting] Fix validation support for nested IErrorObjects (elastic#62833)
  [Metrics UI] Invalidate non-count alerts which have no metrics (elastic#62837)
  Add --filter option to API docs script (elastic#62888)
  [Maps] fix attribution overflow with exit full screen button (elastic#62699)
  [Uptime]Alerting UI text in case filter is selected (elastic#62570)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nchaulet nchaulet nchaulet approved these changes

Assignees

@jen-huang jen-huang

Labels
Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project Feature:Fleet Fleet team's agent central management project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Ingest] Add validation on data source configuration fields
5 participants
@jen-huang @elasticmachine @hbharding @kibanamachine @nchaulet