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

console: ingest node updates for auto-complete #24100

Merged
merged 7 commits into from
Oct 18, 2018
Merged

console: ingest node updates for auto-complete #24100

merged 7 commits into from
Oct 18, 2018

Conversation

jakelandis
Copy link
Contributor

  • add bytes processor
  • add dissect processor
  • add pipeline processor
  • add drop processor
  • add if conditional to each processor
  • add on_failure to each processor

Please note this has NOT has been tested, and the PR is based simply on the existing structure.

@jakelandis
console: ingest node updates for auto-complete
e0f1acb 
* add bytes processor
* add dissect processor
* add pipeline processor
* add drop processor
* add if conditional to each processor
* add on_failure to each processor
@jakelandis jakelandis mentioned this pull request Oct 16, 2018
Closed
11 tasks
@bmcconaghy
Copy link
Contributor

This looks good to me. One suggestion I have is since if and on_failure are common params to all, the code could make that clearer by defining:

const commonPipelineParams = {
    on_failure: [],
    if: ''
}

and then using the common definition in all of the other ones like this:

const bytesProcessorDefinition = {
  bytes: {
    __template: {
      field: ''
    },
    field: '',
    target_field: '',
    ignore_missing: {
      __one_of: [ false, true ]
    },
    ...commonPipelineParams
  }
};

@jakelandis
Copy link
Contributor Author

and then using the common definition in all of the other ones

I am not familiar with how these constants are used, but wouldn't that result in an object in an object ? (Does that still autocomplete correctly ?)

{
  "uppercase":{
    "__template":{
      "field":""
    },
    "field":"",
    "ignore_missing":{
      "__one_of":[
        false,
        true
      ]
    },
    "commonPipelineParams":{
      "on_failure":[],
      "if":""
    }
  }
}

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Contributor

bmcconaghy commented Oct 16, 2018 •
edited
Loading

@jakelandis the ... syntax basically takes all the properties of the object that follows it and copies them into the target object, so you wind up with what you had to begin with but without the duplication of code. https://dmitripavlutin.com/object-rest-spread-properties-javascript/

@bmcconaghy
Copy link
Contributor

You'll need to merge master into this branch to fix the test failure.

@jakelandis
Copy link
Contributor Author

@bmcconaghy

... syntax

doh! from your example, I read the triple dots as ellipsis, not syntax. My ECMAScript is pretty rusty... TIL, thanks !

Changes implemented on d9128c5 and master merged in.

bmcconaghy
bmcconaghy suggested changes Oct 17, 2018
View reviewed changes
Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

As this stands, autocomplete does not work when I load console. Moving the commonPipelineParams definition to the top fixes this issue.

src/core_plugins/console/api_server/es_6_0/ingest.js Outdated
@@ -344,6 +420,10 @@ const pipelineDefinition = {
version: 123,
};

const commonPipelineParams = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be defined above where it is used (just move this to the top). Also, there needs to be a semicolon after the } closing this object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 11c0ac8

bmcconaghy
bmcconaghy approved these changes Oct 17, 2018
View reviewed changes
Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

Code LGTM, and tested in the browser and all the new processors autocomplete as expected. Thanks!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jakelandis jakelandis merged commit 2be837e into elastic:master Oct 18, 2018
@bmcconaghy bmcconaghy mentioned this pull request Oct 18, 2018
Merged
bmcconaghy pushed a commit to bmcconaghy/kibana that referenced this pull request Oct 18, 2018
@jakelandis @bmcconaghy
console: ingest node updates for auto-complete (elastic#24100)
0c88181 
* add bytes processor
* add dissect processor
* add pipeline processor
* add drop processor
* add if conditional to each processor
* add on_failure to each processor
bmcconaghy added a commit that referenced this pull request Oct 19, 2018
@bmcconaghy
console: ingest node updates for auto-complete (#24100) (#24229)
a7ce5de 
* add bytes processor
* add dissect processor
* add pipeline processor
* add drop processor
* add if conditional to each processor
* add on_failure to each processor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmcconaghy bmcconaghy bmcconaghy approved these changes

Assignees
No one assigned
Labels
release_note:enhancement v6.5.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jakelandis @bmcconaghy @elasticmachine @rayafratkina