-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Added CITs for Circle processor. #101871
Added CITs for Circle processor. #101871
Conversation
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
|
||
expect(processors[0].circle).toEqual({ | ||
field: 'field_1', | ||
error_distance: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that error_distance
has a default value of 1 in the UI, but it's not mentioned in the docs. I think if error_distance
doesn't have a default, then its input field should rather stay empty and force the user to set a value. We might need a bug issue for that, but I'd like to confirm with @alisonelizabeth first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @yuliacech! I didn't notice your review until after mine (I left a similar comment) 😄.
@danhermann can you help us understand how the error_distance
option should behave for the circle
processor? The docs don't mention any default value, but we provide a default value of 1
in the UI. Does this make sense, or should we always force the user to provide their own value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know enough about how the underlying circle shape works to know whether a default value (perhaps scaled by the radius of the circle) for error_distance
would make sense there or not. Perhaps someone from @elastic/es-analytics-geo make a suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A default value would only make sense if it is scaled to the radius of a circle. Unfortunately, we are setting up processor at this point and have no information about what type of circles the user is going to ingest. So, I think it would be the best to leave it blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..._pipelines/public/application/components/pipeline_editor/__jest__/processors/circle.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @cuff-links! LGTM. I left one nit as well as a comment on how the error_distance
option should behave.
..._pipelines/public/application/components/pipeline_editor/__jest__/processors/circle.test.tsx
Outdated
Show resolved
Hide resolved
..._pipelines/public/application/components/pipeline_editor/__jest__/processors/circle.test.tsx
Show resolved
Hide resolved
* [Discover] Render empty embeddable * First version of grid embeddable * More search embeddable * Almost stable version * Fixing typescript errors * Fixing filtering and sorting * Add data-shared-item to DiscoverGridEmbeddable * Trigger rerender when title changes * Fixing incorrectly touched files * Remove search_embeddable * Remove lodash * Fixing imports * Minor fixes * Removing unnecessary files * Minor fix * Remove unused import * Applying PR comments * Applying PR comments * Removing search embeddable * Fix missing import * Addressing PR comments * Do not memoize saved search component * Applying Matthias's suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* Fixes filter is applied even if is disabled on a dashboard * Fix 18n problem Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…rtifacts (elastic#101379) * Removes zlib compression when creating artifacts. Also fixes related unit tests and removes old code * Replaces artifact in new manifest using the ones from fleet client with zlip compression * Fixes create_policy_artifact_manifest pushArtifacts missing new manifest. Also fixes unit tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…elastic#101963) Co-authored-by: Shahzad <shahzad31comp@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [ML] Switching to new datafeed preview * fixing wizard test button * adding schema validator * fixing tests * adding check for empty detectors list Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* feat: 🎸 create management app locator * refactor: 💡 simplify management locator * feat: 🎸 export management app locator from plugin contract * feat: 🎸 improve share plugin exports * test: 💍 fix test mock * test: 💍 adjust test mocks * Update src/plugins/management/public/plugin.ts Co-authored-by: Tim Roes <mail@timroes.de> * Update src/plugins/management/public/types.ts Co-authored-by: Tim Roes <mail@timroes.de> * Update src/plugins/management/public/types.ts Co-authored-by: Tim Roes <mail@timroes.de> * Update src/plugins/management/server/plugin.ts Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tim Roes <mail@timroes.de>
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
Something went wrong here and will be reissuing this PR. |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
Closes #100106
Summary
This PR is to add test coverage for the Ingest Pipelines Circle Processor.
Test cases being covered