fix: remove duplicated error message for cloud console#915
fix: remove duplicated error message for cloud console#915feichashao wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSimplified error handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
|
@feichashao: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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