fix: remove duplicated error message for cloud console#915
fix: remove duplicated error message for cloud console#915feichashao wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughSimplified non-200 error handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #915 +/- ##
==========================================
- Coverage 53.93% 53.91% -0.02%
==========================================
Files 88 88
Lines 6656 6651 -5
==========================================
- Hits 3590 3586 -4
+ Misses 2596 2595 -1
Partials 470 470
🚀 New features to boost your workflow:
|
| if readErr == nil { | ||
| response.Body = io.NopCloser(strings.NewReader(bodyStr)) | ||
| } | ||
| apiErr := utils.TryPrintAPIError(response, false) |
There was a problem hiding this comment.
looks removing TryPrintAPIError here makes this path inconsistent with the rest of the CLI as I can see most other cli commands rely on TryPrintAPIError as the standard way to parse and present bp-api errors.
IIUC, the original intent (from PR #825) was to improve visibility for non-JSON responses (e.g. infra errors like 502), which is a valid. However, removing TryPrintAPIError means we lose the structured/clean formatting for the common case (JSON errors from bp-api).
Would it make sense to keep TryPrintAPIError for JSON responses, and only fall back to raw response body when parsing fails (non-JSON)? WDYT?
There was a problem hiding this comment.
This makes sense. I will make a card for this instead as it requires some efforts to carefully change and test.
What type of PR is this?
What this PR does / Why we need it?
The backplane cloud console error message is duplicated, removing the formatted one and keep the raw response.
Before:
After:
Which Jira/Github issue(s) does this PR fix?
Special notes for your reviewer
Unit Test Coverage
Guidelines
Test coverage checks
Pre-checks (if applicable)
/label tide/merge-method-squash
Summary by CodeRabbit
Documentation
Bug Fixes