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

Fix vega specification parsing #67963

Merged
merged 3 commits into from
Jun 11, 2020
Merged

Fix vega specification parsing #67963

merged 3 commits into from
Jun 11, 2020

Conversation

thatguy7
Copy link
Contributor

@thatguy7 thatguy7 commented Jun 2, 2020

Summary

After #42095 the VEGA parser was not adapted. This resulted in the issue as described in #54330. This pull request fixes the issue by adding a FILTER clause.

This also really should have appeared in the breaking changes documentation of any version since 7.4, since it breaks existing VEGA specifications using the MUST clause without any error messages. The resulting graphs just ignore dashboard filters.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@thatguy7 thatguy7 requested a review from a team June 2, 2020 14:52
@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 2, 2020 •
edited
Loading

💚 CLA has been signed

@timroes timroes added Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:fix labels Jun 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

Jenkins, test this

@thatguy7
Copy link
Contributor Author

thatguy7 commented Jun 9, 2020

I filled out the CLA. Nur sure why the check fails.

I noticed this was labeled for the 7.9 release. I really wish this was part of the next patch release, as this issue has been blocking me from updating for quite a while now.

@timroes
Copy link
Contributor

timroes commented Jun 9, 2020

@thatguy7 It seems that you signed the CLA with your gmail address, while all the commits in this PR were signed from a different address. Could you please make sure to either sign also with the commit mail address (easier), or change the mail that has been used in the commit in git and push those changes.

stratoula
stratoula approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula 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, tested on chrome and firefox

@timroes
Copy link
Contributor

timroes commented Jun 9, 2020

@thatguy7 We're trying to keep patch releases for really critical patches, so they stay as stable as possible. Given that Vega is still experimental and this bug doesn't fundamentally break it (though I totally agree it's very annoying), we won't backport that fix into a patch version, but only for the next minor version. Sorry that this will potentially mean you need to wait for another update :-(

@gjelenc
Copy link

gjelenc commented Jun 10, 2020

Can you release this sooner like 7.7.2 ?

@stratoula
Copy link
Contributor

@thatguy7 thanx for signing the CLA! Can you also merge the master and resolve the conflicts in order to be able to merge it ? 🚀

@thatguy7 thatguy7 force-pushed the fix-vega-specification-parsing branch from 132752a to 9533f87 Compare June 10, 2020 12:45
@stratoula
Copy link
Contributor

Jenkins, test this

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #52654 succeeded 132752a53b137fbb52dc6eae850f095cd948b8bf

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

@stratoula stratoula merged commit 091cece into elastic:master Jun 11, 2020
stratoula pushed a commit to stratoula/kibana that referenced this pull request Jun 11, 2020
@thatguy7 @stratoula
Fix vega specification parsing (elastic#67963)
e3bdee9 
* add filter clause to es_query_parser.js

* update tests to consider the filter clause

* update docs to show the new filter clause

Co-authored-by: Oleg Ivasenko <oleg.ivasenko@devolo.de>
@stratoula stratoula mentioned this pull request Jun 11, 2020
Merged
stratoula added a commit that referenced this pull request Jun 11, 2020
@stratoula @thatguy7
Fix vega specification parsing (#67963) (#68867)
b2db19b 
* add filter clause to es_query_parser.js

* update tests to consider the filter clause

* update docs to show the new filter clause

Co-authored-by: Oleg Ivasenko <oleg.ivasenko@devolo.de>

Co-authored-by: Oleg Ivasenko <ivasenko.oleg@gmail.com>
Co-authored-by: Oleg Ivasenko <oleg.ivasenko@devolo.de>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 11, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
34b7cf9 
* master: (38 commits)
  Support migrating from reserved feature privileges (elastic#68504)
  add `preference` field to SavedObjectsFindOptions (elastic#68620)
  [ILM] Add "wait for snapshot" policy field to Delete phase (elastic#68505)
  Cleanup old license overwrites (elastic#68744)
  Bump TypeScript to v3.9 (elastic#67666)
  [APM] Service maps - adds new storybook stories to test out various data sets (elastic#68727)
  Fix vega specification parsing (elastic#67963)
  docs: add more api information (elastic#68717)
  [APM] Don't show annotations on charts with no data (elastic#68829)
  [Metrics UI] Fix Inventory View sorting by handling null values (elastic#67889)
  skip flaky suite (elastic#68836)
  [SIEM][Detections Engine] - Fix reference rule url overflow (elastic#68640)
  Index pattern public api => common (elastic#68289)
  [APM] Lazy-load alert triggers (elastic#68806)
  [DOCS] Fix table formatting in ingest manager settings (elastic#68824)
  [Endpoint] Functional Tests cleanup (elastic#68756)
  revert previous commit which was unintentional
  Use Github token instead for project assignments
  [SIEM][Exceptions] - ExceptionsViewer cleanup (elastic#68739)
  move @kbn/storybook to devDeps (elastic#68791)
  ...
@stratoula stratoula mentioned this pull request Jul 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

Assignees
No one assigned
Labels
💝community Feature:Vega Vega visualizations release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thatguy7 @kibanamachine @elasticmachine @stratoula @timroes @gjelenc