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

feat(api): add interceptor to bubble up underlying error code #1266

Merged
merged 9 commits into from
Jan 10, 2024

Conversation

devholic
Copy link
Contributor

Resolves #1215

Copy link

netlify bot commented Dec 11, 2023

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 7c0294f
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/659d0481d5d2a70008bf7b04
😎 Deploy Preview https://deploy-preview-1266.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 77 lines in your changes are missing coverage. Please review.

Comparison is base (c343c41) 45.56% compared to head (7c0294f) 45.31%.
Report is 12 commits behind head on main.

Files Patch % Lines
internal/api/option/error.go 39.65% 34 Missing and 1 partial ⚠️
internal/api/delete_stage_v1alpha1.go 0.00% 4 Missing ⚠️
internal/api/approve_freight_v1alpha1.go 0.00% 3 Missing ⚠️
...ernal/api/set_auto_promotion_for_stage_v1alpha1.go 0.00% 3 Missing ⚠️
internal/api/create_project_v1alpha1.go 0.00% 2 Missing ⚠️
internal/api/delete_project_v1alpha1.go 0.00% 2 Missing ⚠️
internal/api/delete_promotion_policy_v1alpha1.go 0.00% 2 Missing ⚠️
internal/api/delete_warehouse_v1alpha1.go 0.00% 2 Missing ⚠️
internal/api/option/option.go 0.00% 2 Missing ⚠️
internal/api/update_promotion_policy_v1alpha1.go 0.00% 2 Missing ⚠️
... and 18 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1266      +/-   ##
==========================================
- Coverage   45.56%   45.31%   -0.25%     
==========================================
  Files         135      137       +2     
  Lines       11657    11791     +134     
==========================================
+ Hits         5311     5343      +32     
- Misses       6155     6256     +101     
- Partials      191      192       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devholic
Copy link
Contributor Author

@krancour Changed interceptor to wrap error if the given error is unknown connect error or plain go error

@devholic devholic requested a review from krancour December 14, 2023 01:30
@krancour krancour changed the title feat: add interceptor to bubble up underlying error code feat(api): add interceptor to bubble up underlying error code Dec 19, 2023
@krancour krancour added this to the v0.4.0 milestone Dec 22, 2023
@krancour
Copy link
Member

@devholic it also looks like there might be several remaining cases where a handler does a apierrors.IsNotFound(err) test to explicitly return a connect error with connect.CodeNotFound.

That explicit handling can be removed now, right?

@devholic
Copy link
Contributor Author

devholic commented Jan 2, 2024

@devholic it also looks like there might be several remaining cases where a handler does a apierrors.IsNotFound(err) test to explicitly return a connect error with connect.CodeNotFound.

That explicit handling can be removed now, right?

@krancour I'm still not sure whether it's okay to remove explicit error handling in handlers.

  • Each handler tests will be affected by interceptors.
    • If there are changes in interceptors, all handler test cases can be affected (where checking status code)
  • There can be an inconsistency when writing handler test codes - to check returned error types

@krancour
Copy link
Member

krancour commented Jan 2, 2024

Each handler tests will be affected by interceptors

@devholic handlers should be tested independently of any interceptors.

Sunghoon Kang added 6 commits January 4, 2024 17:34
Resolves #1215

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Sunghoon Kang added 2 commits January 5, 2024 14:28
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
@krancour
Copy link
Member

krancour commented Jan 7, 2024

Code looks good @devholic. I'm going to test drive e2e Monday morning.

@krancour
Copy link
Member

krancour commented Jan 8, 2024

I tried this with a variety of error cases just now. Overall, it's working great, but I did notice that 422 (unprocessable content/entity) is something k8s may sometimes return when, for instance, a webhook or other admission control has found something invalid in a resource, but that's a status code the interceptor isn't mapping.

@devholic I think maybe we should handle that case the same as "bad request." I couldn't find a connect error code that was a better mapping than that.

Easy way to see this in action is through kargo create project FOO, which will fail because Kubernetes doesn't like caps in resource names. Right now it comes through as an internal error due to lack of mapping mentioned above.

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
@krancour krancour self-requested a review January 9, 2024 15:48
Copy link
Member

@krancour krancour left a comment

Choose a reason for hiding this comment

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

This now shows the anticipated error type for every error I've been able to force. 🔥

Thank you @devholic!

@devholic devholic added this pull request to the merge queue Jan 10, 2024
Merged via the queue into main with commit a096d5c Jan 10, 2024
14 of 16 checks passed
@devholic devholic deleted the hoon/#1215 branch January 10, 2024 03:08
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.

error 500 where 403 would be more clear
2 participants