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

[packages] Move @kbn/interpreter to Bazel #101089

Merged
merged 26 commits into from
Jun 22, 2021
Merged

[packages] Move @kbn/interpreter to Bazel #101089

merged 26 commits into from
Jun 22, 2021

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Jun 1, 2021

Part of #69706

That PR moves the @kbn/interpreter into Bazel by pushing a BUILD file for that package.
After merging this the package will be consumed from within bazel-bin folder.

@jbudz jbudz added Team:Operations Team label for Operations Team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 1, 2021
@jbudz jbudz requested a review from a team as a code owner June 1, 2021 20:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@jbudz jbudz requested a review from mistic June 1, 2021 20:01
mistic
mistic reviewed Jun 1, 2021
View reviewed changes
package.json Outdated Show resolved Hide resolved
ppisljar
ppisljar reviewed Jun 2, 2021
View reviewed changes
packages/kbn-interpreter/package.json Outdated
"build": "node scripts/build",
"kbn:bootstrap": "node scripts/build --dev",
"kbn:watch": "node scripts/build --dev --watch"
"interpreter:peg": "../../node_modules/.bin/pegjs src/common/lib/grammar.peg"
Copy link
Member

Choose a reason for hiding this comment

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

can we run this as part of bootstrap ?

Copy link
Member

Choose a reason for hiding this comment

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

@jbudz probably in order to accomplish this we need to do something like https://github.com/elastic/kibana/blob/master/packages/kbn-tinymath/BUILD.bazel#L33

Copy link
Member

@ppisljar ppisljar Jun 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

except that interpreter is not using peggy yet but still the old pegjs.
i am ok with moving to peggy in the same pr as long as it doesn't break

@mistic mistic mentioned this pull request Jun 3, 2021
Closed
23 tasks
@ppisljar
Copy link
Member

ppisljar commented Jun 8, 2021

whats the status on this ?

@mistic
Copy link
Member

mistic commented Jun 9, 2021

@elasticmachine merge upstream

@jbudz
Copy link
Member Author

jbudz commented Jun 9, 2021

jenkins, test it

@ppisljar
Copy link
Member

seems issue still persists, grammar file can't be found .... and a bunch of types failling (maybe due to the same reason?) i guess all the type failiures can be mitigating by using ExpressionAst type from expressions plugin rather that the Ast type from the package

@jbudz jbudz requested a review from a team as a code owner June 17, 2021 15:28
@mistic
Copy link
Member

mistic commented Jun 17, 2021

@ppisljar everything is now working

ppisljar
ppisljar approved these changes Jun 18, 2021
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM!

@ppisljar
Copy link
Member

@elasticmachine merge upstream

flash1293
flash1293 approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Kibana app changes LGTM, code review only

ThomThomson
ThomThomson approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Code review only, Presentation team changes LGTM!

@mistic
Copy link
Member

mistic commented Jun 22, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.5MB 1.5MB +551.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
canvas 531.2KB 531.8KB +551.0B
expressions 209.6KB 210.2KB +551.0B
total +1.1KB

History

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

mistic
mistic approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

LGTM

@jbudz jbudz merged commit 11e68fd into elastic:master Jun 22, 2021
@jbudz jbudz deleted the bazel/kbn-interpreter branch June 22, 2021 14:59
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
❌ 7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 101089

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 24, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 101089 or prevent reminders by adding the backport:skip label.

jbudz added a commit that referenced this pull request Jun 24, 2021
@jbudz @mistic @kibanamachine
[7.x] [packages] Move @kbn/interpreter to Bazel (#101089) (#103308)
e33c29d 
Co-authored-by: Tiago Costa <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Tiago Costa <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 101089 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 101089 or prevent reminders by adding the backport:skip label.

@mistic
Copy link
Member

mistic commented Jun 28, 2021

@jbudz I think this is correctly backported right?

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 28, 2021
@tylersmalley tylersmalley mentioned this pull request Jul 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@ppisljar ppisljar ppisljar approved these changes

@mistic mistic mistic approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backported release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jbudz @elasticmachine @ppisljar @mistic @kibanamachine @flash1293 @ThomThomson