CLD-694: Catalog client error handling#514
Conversation
🦋 Changeset detectedLatest commit: d362c1c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling in the catalog client by converting ResponseStatus objects to proper gRPC errors and fixes linting issues related to deprecated credentials fields.
- Added a
checkResponseStatushelper function to convert ResponseStatus to gRPC errors with proper error codes - Refactored all catalog store implementations to use standardized gRPC error handling instead of string-based error checking
- Fixed credential test by removing deprecated
ServerNamefield assertion
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated dependency versions for chainlink-protos and grpc libraries |
| engine/cld/internal/credentials/credentials_test.go | Removed deprecated ServerName field assertion from credentials test |
| datastore/catalog/remote/response_status.go | Added helper function to convert ResponseStatus to gRPC errors |
| datastore/catalog/remote/response_status_test.go | Added comprehensive tests for the response status conversion function |
| datastore/catalog/remote/env_metadata_store.go | Refactored error handling to use gRPC status codes instead of string matching |
| datastore/catalog/remote/contract_metadata_store.go | Refactored error handling to use gRPC status codes instead of string matching |
| datastore/catalog/remote/chain_metadata_store.go | Refactored error handling to use gRPC status codes instead of string matching |
| datastore/catalog/remote/address_ref_store.go | Simplified error handling to use the new checkResponseStatus helper |
| datastore/catalog/remote/address_ref_store_test.go | Changed test skip to error for missing gRPC server connection |
| .changeset/eight-schools-glow.md | Added changeset for the catalog error handling improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| // checkResponseStatus converts a ResponseStatus to a standard gRPC error. |
There was a problem hiding this comment.
checkResponseStatus doeesnt seem like a good name as this performs conversion instead of just "checking"
What about parseResponse? Or any better name ?
| if strings.Contains(response.Status.GetError(), "No records found") { | ||
| return datastore.AddressRef{}, datastore.ErrAddressRefNotFound | ||
| if statusErr := checkResponseStatus(response.Status); statusErr != nil { | ||
| st, _ := status.FromError(statusErr) |
There was a problem hiding this comment.
should we handle the error here instead of _ ?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
graham-chainlink
left a comment
graham-chainlink
left a comment
There was a problem hiding this comment.
Loooks like sonarqube is failing
Yeap currently we do not have a way to run test in CI which causes causes sonarqube to report the code as untested. Although we have tests and I run the locally (see description). I think @giogam mentioned that there is another ticket in the epic to work out a solution for |
|
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## chainlink-deployments-framework@0.58.0 ### Minor Changes - [#514](#514) [`406cb82`](406cb82) Thanks [@DimitriosNaikopoulos](https://github.com/DimitriosNaikopoulos)! - Handle Catalog ResponseStatus errors as grpc errors - [#518](#518) [`99ee634`](99ee634) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat(offchain): new JD in memory client - [#522](#522) [`0cbed61`](0cbed61) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat(engine): integrate memory JD to test runtime - [#529](#529) [`4bb40fa`](4bb40fa) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat(catalog): new datastore field in domain.yaml Field `datastore` is introduced to configure in future where should the data be written to, either file(json) - current behaviour or remote on the catalog service. By default, this field will be set to file for backwards compatibility. - [#520](#520) [`2cc6462`](2cc6462) Thanks [@ecPablo](https://github.com/ecPablo)! - improve decoded proposal error to use bullets instead of tables - [#517](#517) [`5220e9a`](5220e9a) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat(engine/test): support memory catalog ### Patch Changes - [#527](#527) [`8041f81`](8041f81) Thanks [@giogam](https://github.com/giogam)! - chore: remove Catalog field from Environment struct - [#524](#524) [`41b8c65`](41b8c65) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - fix: remove dep on chainlink-common --------- Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>
CLD-694: Catalog client error handling
checkResponseStatushelper function to convert ResponseStatus to gprcerrorsServerNamefield is deprecated for grpcTest require running catalog and currently are skipped in CI (functionality will be added in another ticket)
Local test execution