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: Improve error handling #1275

Merged
merged 5 commits into from
Jun 7, 2021
Merged

chore: Improve error handling #1275

merged 5 commits into from
Jun 7, 2021

Conversation

tolusha
Copy link
Collaborator

@tolusha tolusha commented Jun 3, 2021

Signed-off-by: Anatolii Bazko [email protected]

What does this PR do?

  • Improve error handling all over the code
  • Got read of possible cannot read property 'statusCode' of undefined error

Screenshot/screencast of this PR

2021-06-03T14:59:23.714Z Error: Command server:deploy failed. Error log: /home/tolusha/.cache/chectl/error.log.
2021-06-03T14:59:23.715Z     at newError (~/projects/chectl/src/util.ts:201:17)
2021-06-03T14:59:23.715Z     at Object.wrapCommandError (~/projects/chectl/src/util.ts:197:10)
2021-06-03T14:59:23.715Z     at Deploy.<anonymous> (~/projects/chectl/src/commands/server/deploy.ts:447:18)
2021-06-03T14:59:23.715Z     at Generator.throw (<anonymous>)
2021-06-03T14:59:23.715Z     at rejected (~/projects/chectl/src/commands/server/deploy.ts:15:65)
2021-06-03T14:59:23.715Z Cause: Error: 404 page not found
2021-06-03T14:59:23.715Z     at Object.newError (~/projects/chectl/src/util.ts:201:17)
2021-06-03T14:59:23.715Z     at KubeHelper.wrapK8sClientError (~/projects/chectl/src/api/kube.ts:2633:14)
2021-06-03T14:59:23.715Z     at KubeHelper.<anonymous> (~/projects/chectl/src/api/kube.ts:1647:18)
2021-06-03T14:59:23.715Z     at Generator.throw (<anonymous>)
2021-06-03T14:59:23.715Z     at rejected (~/projects/chectl/src/api/kube.ts:15:65)
2021-06-03T14:59:23.715Z Cause: HttpError: HTTP request failed
2021-06-03T14:59:23.715Z     at Request.localVarRequest [as _callback] (~/projects/chectl/node_modules/@kubernetes/client-node/dist/gen/api/customObjectsApi.js:1482:40)
2021-06-03T14:59:23.715Z     at Request.self.callback (~/projects/chectl/node_modules/request/request.js:185:22)
2021-06-03T14:59:23.715Z     at Request.<anonymous> (~/projects/chectl/node_modules/request/request.js:1154:10)
2021-06-03T14:59:23.715Z     at IncomingMessage.<anonymous> (~/projects/chectl/node_modules/request/request.js:1076:12)

What issues does this PR fix or reference?

eclipse-che/che#19916

How to test this PR?

N/A

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@tolusha tolusha mentioned this pull request Jun 3, 2021
Closed
47 tasks
@tolusha
Copy link
Collaborator Author

tolusha commented Jun 4, 2021

/retest

tolusha added 3 commits June 4, 2021 15:03
@tolusha
chore: Improve error handling
66445ac 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha
Fix
d5b01c3 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha
Improvements
3d068b9 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha tolusha force-pushed the improveerrorhandling branch from 1183467 to 3d068b9 Compare June 4, 2021 12:03
@tolusha
Fix tests
f2a5a3a 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha
Copy link
Collaborator Author

tolusha commented Jun 5, 2021

/retest

1 similar comment
@tolusha
Copy link
Collaborator Author

tolusha commented Jun 6, 2021

/retest

mmorhun
mmorhun reviewed Jun 7, 2021
View reviewed changes
src/util.ts

export function newError(message: string, cause: Error): Error {
const error = new Error(message)
error.stack += `\nCause: ${cause.stack}`
Copy link
Contributor

@mmorhun mmorhun Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Probably we should preserve original error message too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original message is included in cause.stack

test/e2e/util.ts
@@ -111,8 +111,11 @@ export class E2eHelper {
// Return id of test workspaces(e2e-tests. Please look devfile-example.yaml file)
async getWorkspaceId(): Promise<any> {
const workspaces = await this.getAllWorkspaces()
const workspaceId = workspaces.filter((wks => wks.name.match(this.devfileName))).map(({ id }) => id)[0]
if (workspaces.length === 0) {
throw Error('Workspace not found')
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure, but since we introducing newError method we should use it. Otherwise it might be a mess of new Error(...) and new Error() calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A new function is introduced to preserver an original stack trace and message.
So, not for this case

@tolusha
Fix
742b648 
Signed-off-by: Anatolii Bazko <[email protected]>
@openshift-ci openshift-ci bot added the lgtm label Jun 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 7, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mmorhun, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tolusha tolusha merged commit bb102dd into main Jun 7, 2021
@tolusha tolusha deleted the improveerrorhandling branch June 7, 2021 13:54
@che-bot che-bot added this to the 7.32 milestone Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmorhun mmorhun mmorhun approved these changes

@AndrienkoAleksandr AndrienkoAleksandr Awaiting requested review from AndrienkoAleksandr

@flacatus flacatus Awaiting requested review from flacatus

@l0rd l0rd Awaiting requested review from l0rd l0rd is a code owner

Assignees

@mmorhun mmorhun

Labels
lgtm
Projects
None yet
Milestone
7.32
Development

Successfully merging this pull request may close these issues.
3 participants
@tolusha @mmorhun @che-bot