-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: wrap errors in auto CLI service registration #20493
Conversation
The current code in #20412, when attempting to run simd, was failing with the error: panic: proto: not found" Wrapping the error returned by FindDescriptorByName gave a more informative error: panic: failed to find descriptor for "cosmos.accounts.v1.Msg": proto: not found However, there is an earlier root cause. The a.err field was set due to a failure in proto.MergedRegistry but later overridden. Fixing that early return now shows the root cause: panic: failed to build registry cache: proto: could not resolve import "tendermint/abci/types.proto": not found Helps #20492.
WalkthroughThe recent changes in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant AutoCLI
participant RegistryCache
participant Descriptor
User->>AutoCLI: Invoke command
AutoCLI->>RegistryCache: Build registry cache
RegistryCache-->>AutoCLI: Success/Error
alt Cache Error
AutoCLI->>User: Return formatted error message
else Cache Success
AutoCLI->>Descriptor: Find descriptor
Descriptor-->>AutoCLI: Success/Error
alt Descriptor Error
AutoCLI->>User: Return formatted error message
else Descriptor Success
AutoCLI->>User: Return command result
end
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@mark-rushakoff your pull request is missing a changelog! |
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- runtime/v2/services/autocli.go (2 hunks)
Additional context used
Path-based instructions (1)
runtime/v2/services/autocli.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
runtime/v2/services/autocli.go (2)
108-111
: Improve error handling by wrapping the original error.This change enhances the error message by wrapping the original error with a more descriptive message, which aligns with the PR's objective to provide clearer error outputs. This is a good use of Go's error wrapping feature to preserve the error chain.
117-117
: Ensure error messages are informative and actionable.The addition of a formatted error message here is crucial for debugging. It provides a clear indication of what went wrong and where, which is particularly useful given the complexity of service registration and proto handling in this context.
Thank you for the contribution!! Glad to see you here ❤️ |
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.
Thank you @mark-rushakoff for the improvement
* main: (120 commits) chore: update protoc-gen-swagger to protoc-gen-openapiv2 (#20448) ci: Add GitHub Action for go mod tidy and mocks (#20501) chore: Address linter issues (#20486) fix: wrap errors in auto CLI service registration (#20493) chore: fix comment (#20498) chore: fix the note box syntax error (#20499) feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481) refactor(core): remove redundant ExecMode (#20322) feat(store/v2): pruning manager (#20430) chore: force reload sonar cloud (#20480) refactor(x/accounts): reuse calculated sum in `func safeAdd` (#20458) refactor: remove `defer` in loop (#20223) ci: remove livness test (#20474) build(deps): Bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#20477) chore: migrate a few diagrams to mermaid (#20473) ...
The current code in #20412, when attempting to run simd, was failing with the error:
panic: proto: not found"
Wrapping the error returned by FindDescriptorByName gave a more informative error:
panic: failed to find descriptor for "cosmos.accounts.v1.Msg": proto: not found
However, there is an earlier root cause. The a.err field was set due to a failure in proto.MergedRegistry but later overridden. Fixing that early return now shows the root cause:
panic: failed to build registry cache: proto: could not resolve import "tendermint/abci/types.proto": not found
Helps #20492.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit