Skip to content

Remove ts typecheck from build#37190

Merged
mistic merged 4 commits intoelastic:masterfrom
mistic:fix-ts-typecheck-on-build
Jun 27, 2019
Merged

Remove ts typecheck from build#37190
mistic merged 4 commits intoelastic:masterfrom
mistic:fix-ts-typecheck-on-build

Conversation

@mistic
Copy link
Contributor

@mistic mistic commented May 27, 2019

Currently we are not typechecking x-pack typescript code when building, and that PR fixes it.
However one thing that occurred to me was that during our CI we will run a typecheck anyway. While I think it could be a good idea keep checking it in both places (CI + build) I'm also okay with removing it from our build. What are your ideas here @spalger ? Let's keep that PR to fix the typecheck and run it in both places or update the PR to remove the typecheck step from the build?

@mistic mistic requested a review from spalger May 27, 2019 18:00
@mistic mistic requested a review from a team as a code owner May 27, 2019 18:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger
Copy link
Contributor

spalger commented May 27, 2019

I personally don’t think there’s any benefit to type checking in the build and as an independent task in CI, and I personally like that they’re separate tasks.

@mistic
Copy link
Contributor Author

mistic commented May 28, 2019

So are you just in favor on removing it from the build distributable tasks? By my side I support that decision!

@mistic mistic changed the title Fix ts typecheck on build Remove ts typecheck from build May 28, 2019
@mistic
Copy link
Contributor Author

mistic commented May 28, 2019

@spalger as discussed I've removed the typecheck from the build process itself

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mistic
Copy link
Contributor Author

mistic commented May 28, 2019

the build would fail until #36769 is merged

@spalger
Copy link
Contributor

spalger commented May 28, 2019

@elastic/kibana-platform do any of you think this is a bad idea? I didn't realize that we were even doing it, and I'm personally a fan of avoiding duplicate work if we can avoid it. (we run the type_check script in the intake job, no reason to run it on every build)

@mistic
Copy link
Contributor Author

mistic commented May 28, 2019

Just to give more context, in the first place we were building typescript into this task. When moving from tsc to babel7 instead of deleting that task I've refactoring it to apply a typecheck. But revisiting it right now it makes more sense to me to just remove it.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@mistic mistic added release_note:skip Skip the PR/issue when compiling release notes and removed review labels Jun 17, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mistic mistic merged commit ec6c278 into elastic:master Jun 27, 2019
@mistic
Copy link
Contributor Author

mistic commented Jun 27, 2019

7.x: e481029

mistic added a commit to mistic/kibana that referenced this pull request Jun 27, 2019
* chore(NA): add typecheck for x-pack

* chore(NA): remove typescript typecheck task from build
mistic added a commit that referenced this pull request Jun 27, 2019
* chore(NA): add typecheck for x-pack

* chore(NA): remove typescript typecheck task from build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build release_note:skip Skip the PR/issue when compiling release notes Team:Operations Kibana-Operations Team v7.3.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants