Skip to content

SDK errors#1401

Merged
wtrocki merged 3 commits intomainfrom
sdk-errors
Feb 1, 2022
Merged

SDK errors#1401
wtrocki merged 3 commits intomainfrom
sdk-errors

Conversation

@wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented Jan 27, 2022

Verification

rhoas service-registry create # reach limits 
rhoas service registry describe --name=whatever
rhoas service registry describe --id=whatever

Notes (outdated)

Current problems I see:

  1. Lack way to handle errors per SDK (API exposing hasError etc.) so we need to define errors in multiple places/defefine them or have helper methods for creating proper message per type.

  2. Errors type should be probably moved to the core and not defined for kafka specifically

  3. SDK on its own defines all errors on root package. Maybe we should have kafkaerrors.ERROR_40 instead?

Screenshot 2022-01-27 at 12 57 00

@wtrocki wtrocki requested a review from a team as a code owner January 27, 2022 13:03
@craicoverflow
Copy link
Contributor

Current problems I see:
SDK on its own defines all errors on root package. Maybe we should have kafkaerrors.ERROR_40 instead?

I just have a question - can you explain further what the problem you see is?

@wtrocki
Copy link
Collaborator Author

wtrocki commented Jan 27, 2022

There are 80 fields in the root of the SDK (from which 95% are errors constants)
image

@craicoverflow
Copy link
Contributor

Thanks that is clear!

@wtrocki wtrocki force-pushed the sdk-errors branch 3 times, most recently from 7a2384c to acb0c61 Compare January 31, 2022 14:45
)

func NotFoundByIDError(id string) error {
NotFoundByIDErr := fmt.Errorf(`Registry instance with ID "%v" not found`, id)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wasn't possible to add localization to those (same as kafka) due to them being used in different context that do not have localiser injected. I think it is good to have those 2 localized and will add that after this PR (it will require a looot of changes that will obfuscate this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

You could create a ServiceRegistryErrors struct to wrap these errors?

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

LGTM


func NotFoundByIDError(id string) error {
NotFoundByIDErr = fmt.Errorf(`Kafka instance with ID "%v" not found`, id)
NotFoundByIDErr := fmt.Errorf(`Kafka instance with ID "%v" not found`, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could return the error inline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh.. yes. That was copy paste from kafka errors.

)

func NotFoundByIDError(id string) error {
NotFoundByIDErr := fmt.Errorf(`Registry instance with ID "%v" not found`, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could create a ServiceRegistryErrors struct to wrap these errors?

@wtrocki wtrocki merged commit f8f81e9 into main Feb 1, 2022
@wtrocki wtrocki deleted the sdk-errors branch February 1, 2022 10:46
@wtrocki
Copy link
Collaborator Author

wtrocki commented Feb 1, 2022

It feels like this was overkill to handle only 3 errors from srs (as all kafka errors were handled already).
We should try to refactor error handling in the CLI but I see this as step process where we improve new and existing commands rather than doing this as whole.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants