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

refactor: http status code and error in api(daytonaio#934) #1056

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

the-johnwick
Copy link
Contributor

@the-johnwick the-johnwick commented Sep 5, 2024

Refactor better HTTP status code and error

Description

Returns client-side HTTP response code & adds error messages according to the error.

issue #934
/claim #934

@the-johnwick the-johnwick force-pushed the better-error-with-httpcode branch 6 times, most recently from 3a433e9 to 335239c Compare September 9, 2024 11:28
@the-johnwick the-johnwick marked this pull request as ready for review September 9, 2024 11:34
@the-johnwick the-johnwick requested a review from a team as a code owner September 9, 2024 11:34
@the-johnwick
Copy link
Contributor Author

@Tpuljak Don't know why tests are failing.

@Tpuljak
Copy link
Member

Tpuljak commented Sep 9, 2024

@Tpuljak Don't know why tests are failing.

Screenshot 2024-09-09 at 13 41 57

Because it seems you didn't set up the mock correctly.

Before pushing, please run go test -tags testing ./... to run them locally and validate.

@the-johnwick
Copy link
Contributor Author

@Tpuljak I tried everything but still is not working. I am returning HTTP Status Code as 2nd return value of the functions.
In mock tests we are using dummy data. so it will not return the Success code 200, so I hardcoded 0 int the return values of the mock functions and mock.on("func",0,nil). it should not fail. but still it is falling

@Tpuljak
Copy link
Member

Tpuljak commented Sep 11, 2024

@Tpuljak I tried everything but still is not working. I am returning HTTP Status Code as 2nd return value of the functions. In mock tests we are using dummy data. so it will not return the Success code 200, so I hardcoded 0 int the return values of the mock functions and mock.on("func",0,nil). it should not fail. but still it is falling

@the-johnwick a glanced at the code and I suggest that we go a different route for this.

Returning an int from all the functions is not really intuitive in this case.

I suggest the following:

  • Revert back all functions to return (DATA, error)
  • When an error occurs on an git provider api client, wrap the error with Status code: STATUS_CODE. Err: ERROR_MESSAGE. Then you can unwrap/parse that error message in the controller and set the appropriate status code
  • This will make the code more readable, maintainable and consistent with the rest of our codebase.

I'll put this PR in draft now. You can mark it ready for review once you've implemented the suggestions. Thanks!

@Tpuljak Tpuljak marked this pull request as draft September 11, 2024 07:15
@the-johnwick
Copy link
Contributor Author

@Tpuljak I will update it ASAP. Thanks for the help.

@the-johnwick the-johnwick force-pushed the better-error-with-httpcode branch 2 times, most recently from 8f26e5c to f818101 Compare September 11, 2024 12:20
@the-johnwick the-johnwick marked this pull request as ready for review September 11, 2024 12:22
@the-johnwick
Copy link
Contributor Author

@Tpuljak Done Please check.

@Tpuljak
Copy link
Member

Tpuljak commented Sep 11, 2024

@the-johnwick putting this back in draft. Please make sure to test everything before requesting a review again.

  • There is a println leftover
  • None of the git providers actually format the error appropriately. In my last comment, I said that the provider methods should be the ones formatting the error
  • When requesting a new review, provide a screenshot where it's shown that the code works and accomplishes the target outcome

@Tpuljak Tpuljak marked this pull request as draft September 11, 2024 12:25
@the-johnwick
Copy link
Contributor Author

@Tpuljak do you mean to format git provider client errors only right

@Tpuljak
Copy link
Member

Tpuljak commented Sep 11, 2024

@Tpuljak do you mean to format git provider client errors only right

Yes.

Signed-off-by: the-johnwick <[email protected]>
@the-johnwick the-johnwick marked this pull request as ready for review September 16, 2024 08:43
@the-johnwick
Copy link
Contributor Author

@Tpuljak Done updated the all the changes.

Signed-off-by: the-johnwick <[email protected]>
pkg/api/controllers/gitprovider/user.go Outdated Show resolved Hide resolved
pkg/api/controllers/utils.go Outdated Show resolved Hide resolved
pkg/api/controllers/gitprovider/branches.go Outdated Show resolved Hide resolved
Signed-off-by: the-johnwick <[email protected]>
@the-johnwick
Copy link
Contributor Author

@Tpuljak Updated the changes.

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

2 issues I found currently:

Screenshot 2024-09-18 at 14 54 58

This happens when I add invalid credentials to Bitbucket (err is not set obviously)

Screenshot 2024-09-18 at 14 56 19

This happens when I add invalid credentials to Github Enterprise Server. There is no nil check inside the error format function so the server panics.

Signed-off-by: the-johnwick <[email protected]>
@the-johnwick
Copy link
Contributor Author

@Tpuljak Updated the code. Added regex instead of marshaling/unmarshalling.

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

@the-johnwick did you test all the errors for all providers before requesting the review?

pkg/gitprovider/gitea.go Outdated Show resolved Hide resolved
pkg/gitprovider/gitlab.go Show resolved Hide resolved
@the-johnwick
Copy link
Contributor Author

the-johnwick commented Sep 18, 2024

@Tpuljak Retested everything and fixed the problem

Signed-off-by: the-johnwick <[email protected]>
pkg/gitprovider/gitlab.go Outdated Show resolved Hide resolved
Signed-off-by: the-johnwick <[email protected]>
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Error parsing is missing from some controller methods such as SetGitProvider and RemoveGitProvider. The error parsing needs to be in all methods.

Screenshot 2024-09-19 at 13 51 58

pkg/gitprovider/github.go Outdated Show resolved Hide resolved
Signed-off-by: the-johnwick <[email protected]>
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

Looks good. Nice work!

Copy link
Contributor

@lbrecic lbrecic left a comment

Choose a reason for hiding this comment

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

LGTM!

@the-johnwick
Copy link
Contributor Author

@Tpuljak Can we merge it now?

@Tpuljak Tpuljak merged commit 15af1bc into daytonaio:main Sep 20, 2024
12 checks passed
divanshu-go pushed a commit to divanshu-go/daytona that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants