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

chore(NA): moving @kbn/tinymath into bazel #97022

Merged
merged 7 commits into from
Apr 14, 2021
Merged

chore(NA): moving @kbn/tinymath into bazel #97022

merged 7 commits into from
Apr 14, 2021

Conversation

mistic
Copy link
Member

@mistic mistic commented Apr 13, 2021

One step forward on #69706

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

@mistic mistic added chore release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.13.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 13, 2021
@mistic

This comment has been minimized.

Sign in to view
@mistic mistic marked this pull request as ready for review April 13, 2021 21:28
@mistic mistic requested a review from a team April 13, 2021 21:28
@elasticmachine
Copy link
Contributor

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

@mistic
Copy link
Member Author

mistic commented Apr 13, 2021

@elastic/kibana-app Can I have a code owners review here ? I've migrated that package into the Bazel build system

@mistic mistic mentioned this pull request Apr 13, 2021
Closed
23 tasks
wylieconlon
wylieconlon reviewed Apr 13, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Since I'm the most recent person who has maintained tinymath, and I need to make a few more changes in the near future, I am trying to understand how this affects my development flow.

Specifically, it looks like you have taken out the scripts that I would use to update the grammar definition. Normally I would run yarn build from the same directory, but you've deleted this.

Second, it looks like you are now using the build products in the test code. My workflow is normally:

  1. Change the grammar.peg file
  2. yarn build
  3. Jest will automatically pick up the change to grammar.js and rerun the tests

Will this workflow change?

@mistic
Copy link
Member Author

mistic commented Apr 13, 2021 •
edited
Loading

Thanks for the quick feedback @wylieconlon

Your workflow within our new build system for packages would be:

1- Change the grammar.peg file
2- yarn kbn bazel-build or if you need to continuously watch it you can use yarn kbn bazel-watch
3 - When re-running the tests the new changes will be picked up.

I've also summarised some info at https://github.com/elastic/kibana/blob/master/docs/developer/getting-started/monorepo-packages.asciidoc

wylieconlon
wylieconlon approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Got it. I haven't actually checked out the code and followed that workflow, but I think the changes to tinymath itself are fine.

@mistic mistic requested a review from jbudz April 13, 2021 22:29
jbudz
jbudz approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

I tested build, watch, and server start on Windows. LGTM

@mistic mistic enabled auto-merge (squash) April 14, 2021 16:42
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@mistic mistic merged commit 0bfa5aa into elastic:master Apr 14, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 14, 2021
@mistic @kibanamachine
chore(NA): moving @kbn/tinymath into bazel (elastic#97022)
8f2e987 
* chore(NA): moving @kbn/tinymath into bazel

* chore(NA): fixed jest tests

* chore(NA): simplified tsconfig file

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Apr 14, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 14, 2021
@kibanamachine @mistic
chore(NA): moving @kbn/tinymath into bazel (#97022) (#97173)
9b3499a 
* chore(NA): moving @kbn/tinymath into bazel

* chore(NA): fixed jest tests

* chore(NA): simplified tsconfig file

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
@wylieconlon wylieconlon mentioned this pull request Apr 28, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wylieconlon wylieconlon wylieconlon approved these changes

@jbudz jbudz jbudz approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@mistic @elasticmachine @kibanamachine @wylieconlon @jbudz