-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Enhancing errors for the metadata api #7515
base: master
Are you sure you want to change the base?
Enhancing errors for the metadata api #7515
Conversation
Signed-off-by: Shivam <[email protected]>
|
||
import ( | ||
"fmt" | ||
"net/http" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you need to run: make lint
locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... thanks
pkg/api/errors/metdata.go
Outdated
kiterrors "github.com/dapr/kit/errors" | ||
) | ||
|
||
func IncomingContextMetadataNotFound() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a generic not found error in the errors.go file in this same directory you can use instead of creating one specific to the Metadata API
pkg/api/errors/metdata.go
Outdated
"ERR_METADATA_RETRIEVAL_FAILED", | ||
). | ||
WithErrorInfo("METADATA_RETRIEVAL_FAILED", nil). | ||
WithResourceInfo("", "", "", message). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we are passing in more resource info here, otherwise its really not worth adding since you already added the message
above
@@ -15,7 +15,7 @@ package metadata | |||
|
|||
import ( | |||
"context" | |||
|
|||
"erros" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, why was this added? I don't see this import being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
Oh not saw this
@@ -31,7 +31,7 @@ type ctxKey struct{} | |||
func FromIncomingContext(ctx context.Context) (MD, bool) { | |||
md, ok := ctx.Value(ctxKey{}).(MD) | |||
if !ok { | |||
return nil, false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check where this func is being called? I don't think we should be changing a return of type bool
to being an error
. I don't know if the caller will expect that and it might cause other issues.
The idea behind the error standardization for the Metadata API was to update err
variables that are already in our code base to be our standardized error with more relevant error details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check where this func is being called? I don't think we should be changing a return of type
bool
to being anerror
. I don't know if the caller will expect that and it might cause other issues.The idea behind the error standardization for the Metadata API was to update
err
variables that are already in our code base to be our standardized error with more relevant error details.
Thanks for suggesting I will look in these
Thanks for the start on this, however I think there is a long ways to go still. Specifically, there may be some confusion on the Metadata API, and what that is. Here is the Dapr docs for the Metadata API. From there, try to force some Metadata API errors, and update those existing returned |
I looked into the docs.. I understood that these are the things whichever metadata api will return It would be great if I get a small example... Although I saw the pr for state and pubsub.. A small example would really help me.. a small and clear one @cicoyle |
Just an example like how one error handler is created for metadata and how it is used @cicoyle |
There are examples of using the API in the docs link I provided. I sent a very clear example or 2 with links to the line in our codebase in our discord conversation as well. My suggestion is to read the docs link I provided, and try to force an error, then trace that in the codebase. I would also suggest making this a draft PR, since the tests need to be written as well and polishing up the changes. |
Thanks.. Ya let me make it draft pr |
Signed-off-by: shivam <[email protected]>
Hello @cicoyle I referred the PR for PubSub API and State API both and the Metadata api docs also api docs I have a few questions:-
My question is that from where we are getting it? When i searched in the Repo I saw we are using it from here
My question is that is there any good way to find what our function will return? Thanks. |
I am currently not knowing that what functions will take as Parameters. So do we need to take Parameters based on these? |
The I recommend reading the code there to understand better how to use the pkg when constructing the error with the builder pattern. If you are using an IDE, you should be able to The function always returns the error type. If you are asking about the parameters the function accepts, that depends on the API and what makes sense for that specific error you are standardizing. |
This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 67 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description
I am working on modifying the error handling for metadata API. Currently i have added 2 of them and
working on writing more rich error handlers
Need some suggestions till here is it going correct?Issue reference
refs #7489
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: