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

[index patterns] Don't attempt to wrap Boom errors #14253

Merged

Conversation

kimjoar
Copy link
Contributor

@kimjoar kimjoar commented Oct 2, 2017

Looks like Boom.wrap throws if it's already a Boom error and either status or message is specified (https://github.com/hapijs/boom/blob/master/lib/index.js#L96). Looks like there's a couple of people that have hit this problem in #14021.

Boom 2.8.0 in https://github.com/elastic/kibana/blob/v5.5.0/package.json#L109 and Boom 5.2.0 in
https://github.com/elastic/kibana/blob/v5.6.2/package.json#L108, so maybe some breaking change there that might cause this to throw now(?)

@kimjoar kimjoar added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v5.6.3 v6.0.0 v6.1.0 v7.0.0 labels Oct 2, 2017
@rhoboat
Copy link

rhoboat commented Oct 2, 2017

According to this comment, last version where this didn't throw was Boom 4.2.0. But now, check it out, there's a commit in Boom 5.2.0 recommending using boomify over wrap.

Copy link

@rhoboat rhoboat left a comment

Choose a reason for hiding this comment

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

This will definitely help. Thanks for doing this.

@kimjoar kimjoar force-pushed the index-patterns/do-not-wrap-boom-errors branch from 6dfc3fb to a45e43c Compare October 3, 2017 09:41
@kimjoar kimjoar merged commit 5009435 into elastic:master Oct 3, 2017
kimjoar added a commit to kimjoar/kibana that referenced this pull request Oct 3, 2017
kimjoar added a commit to kimjoar/kibana that referenced this pull request Oct 3, 2017
@kimjoar
Copy link
Contributor Author

kimjoar commented Oct 3, 2017

6.x: 3e7457d
6.0: 60dc602
5.6: 6819311

@epixa
Copy link
Contributor

epixa commented Oct 5, 2017

@kjbekkelund Does this actually fix #14021?

@kimjoar
Copy link
Contributor Author

kimjoar commented Oct 5, 2017

@epixa I haven't been able to reproduce it, but I expect not. It will help debugging this though. If you have other ideas for debugging while waiting for this to get in, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.6.3 v6.0.0 v6.1.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants