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.

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

Upgrade to hapi version 18 #80468

Merged
merged 22 commits into from
Nov 2, 2020
Merged

Upgrade to hapi version 18 #80468

merged 22 commits into from
Nov 2, 2020

Conversation

watson
Copy link
Contributor

@watson watson commented Oct 14, 2020

Depends on #80713
Closes #54168

This PR upgrades from hapi v17 to v18 (and related packages) and replaces PR #61959, which had become stale and drifted too far away from master.

See the hapi v18 release notes for details of the breaking changes (see also the 18.4.1 docs).

The reason why we do not upgrade directly to hapi v20 is because that version doesn't work with Node.js 10. Once we've upgraded to a newer Node.js version we can go ahead and upgrade hapi to version 20.

List of upgraded modules:

Old module New module Old version New version Breaking Changes Risk
hapi @hapi/hapi 17.6.0 18.4.1 hapijs/hapi#3871 ❤️
boom @hapi/boom 7.2.2 7.4.11 hapijs/boom#215 💛
hapi-auth-cookie @hapi/cookie 9.0.0 10.1.2 hapijs/cookie#209 ❤️
h2o2 @hapi/h2o2 8.1.2 8.3.2 💚
hoek @hapi/hoek 5.0.4 8.5.1 hapijs/hoek#268
hapijs/hoek#299
hapijs/hoek#306
hapijs/hoek#315
❤️
inert @hapi/inert 5.1.0 5.2.2 💚
podium @hapi/podium 3.1.2 3.4.3 💚
statehood @hapi/statehood 6.0.6 6.1.2 💚
vision @hapi/vision 5.4.0 5.4.4 💚

For now, our fork of good (@elastic/good), hasn't been upgraded to @hapi/good. Likewise, joi hasn't been upgraded to @hapi/joi.

Note to reviewers

This PR might look very daunting since it's touching 244 files. However, 224 out of the 244 file-changes are just renaming the imports to include the @hapi/ prefix.

@watson watson self-assigned this Oct 14, 2020
@watson watson added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Oct 14, 2020
@watson watson force-pushed the bump-hapi2 branch 6 times, most recently from 9eb832b to 39ee6dd Compare October 15, 2020 17:41
@watson watson force-pushed the bump-hapi2 branch 4 times, most recently from c71308d to 13edca3 Compare October 28, 2020 15:17
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting team related code LGTM

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

apm changes lgtm

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM.

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra changes LGTM, thank you!

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Platform changes look good to me but will defer to @restrry for final approval.

Can we create upstream issues for the hapi type bugs? It would be nice to fix them even though we don't have to block this PR before we do that.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM for monitoring!

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Doesn't appear to be any Presentation changes, but fired it up and it looked good to me 👍

@watson
Copy link
Contributor Author

watson commented Oct 30, 2020

Can we create upstream issues for the hapi type bugs? It would be nice to fix them even though we don't have to block this PR before we do that.

Good idea. I'll make sure to do that once this is merged

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML and transforms changes LGTM

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work on this one 😄

@watson watson requested a review from a team as a code owner October 31, 2020 09:49
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

The platform code looks good, thank you ❤️
It demonstrates we need even more integration tests.
joi update in @kbn/config-schema is on us @elastic/kibana-platform

@@ -77,6 +77,10 @@
"url": "https://github.com/elastic/kibana.git"
},
"resolutions": {
"**/@hapi/iron": "^5.1.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to enforce this resolution? Are there any versions conflicting with each other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the hapi modules depend on @hapi/iron version *, which currently resolves to 6.0.0, which isn't compatible with Node.js 10. So to ensure it works with our version of Node.js, I had to force this resolution.

Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean we can remove this after #61587 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I would think so - unless there's some other incompatibility that I don't know of. If nothing else, we should be able to remove it once we upgrade to hapi v20, which can be done after the upgrade to Node.js 12.

src/core/server/http/http_server.mocks.ts Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.test.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.ts Outdated Show resolved Hide resolved
src/core/server/http/router/request.ts Show resolved Hide resolved
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, just @hapi/ prefixes for boom imports. Code review only

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
enterpriseSearch 638.2KB 638.5KB +238.0B

distributable file count

id before after diff
default 48125 48211 +86
oss 28587 28673 +86

page load bundle size

id before after diff
upgradeAssistant 64.2KB 64.4KB +238.0B

History

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

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

i18n changes lgtm. The only relevant change I found is in src/legacy/server/i18n/i18n_mixin.ts

@watson watson merged commit 7002250 into elastic:master Nov 2, 2020
@watson watson deleted the bump-hapi2 branch November 2, 2020 12:18
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 2, 2020
…e-details-overlay

* 'master' of github.com:elastic/kibana: (72 commits)
  [CCR] Update README.md on how to start 2 clusters for testing (elastic#81487)
  [APM] Scale transaction rate correctly (elastic#82155)
  Upgrade to hapi version 18 (elastic#80468)
  [Uptime] Remove custom handling of license enabling (elastic#82019)
  [Telemetry] Remove `from` and `to` timestamps from usage stats APIs (elastic#81579)
  Enable send to background in Vega (elastic#82229)
  Enable send to background in Timelion (elastic#82232)
  [Actions & Connectors] removes Connector flyouts after usage (elastic#82126)
  Add derivative function (elastic#81178)
  [Discover] Deangularize context_app.html, part 3 (elastic#81838)
  [Visualize] Vis listing page breaks on unknown vis type (elastic#82018)
  Rename `batchSize` parameter to `batch_size` to be consisten with the API namings guidelines. (elastic#82123)
  Minor edits in Single Metric Viewer (elastic#82159)
  [Actions] Fix type contract (elastic#82168)
  Upgrade EUI to v30.1.1 (elastic#81499)
  Skip failing ES snapshot test (elastic#82207)
  Skip ES snapshot failing suite (elastic#82206)
  [Alerting UI] Grouped list of alert types using producers in Types filter of Alerts tab (elastic#81876)
  [Maps] convert vector style component to typescript round 1 (elastic#81961)
  Fix link to upgrade assistant (elastic#82138)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update hapi related packages