ROSAENG-62084: refactor: platform api error enumeration - #331
Merged
openshift-merge-bot[bot] merged 7 commits intoAug 13, 2026
Conversation
…lers Introduce APIError struct with stable code, HTTP status, and message — defined once in errorcodes.go and referenced by both handlers. Replaces scattered inline string literals, fixes duplicate codes (CREATE-002 used for three different conditions, nodepool CREATE-003 shared between conflict and failure), and adds cluster name length validation (CLUSTERS-MGMT-CREATE-006) derived from the HyperShift namespace formula.
Introduces pkg/apierror as a shared leaf package providing APIError with WithErrors/WithReason builder methods and a Write function that derives reason from structured or plain errors automatically. All handlers and middleware now use typed error vars defined in errorcodes.go — no inline JSON, no hardcoded strings, no local message overrides. Integration test assertions updated to reference .Code fields instead of stale literals.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds shared JSON response helpers and structured API errors. It migrates platform handlers and middleware to centralized responses, adds cluster-name validation, injects loggers into health and info handlers, and updates validation formatting and tests. ChangesAPI response and error contracts
Endpoint migration
Validation and verification
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The refactor centralizes platform API error responses and adds redaction behavior; one path does not handle serialization failure and another can split multibyte UTF-8 input, potentially producing malformed client responses. The PR is otherwise mergeable with explicit owner awareness of these bounded correctness risks. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/pkg/api/error.go`:
- Around line 45-59: Update the error response flow around the APIError envelope
and its Encode call to serialize the complete response before invoking
WriteHeader. Handle marshal/serialization failures, including unencodable
structured Errors values that bypass the error branch, and return both
serialization and post-commit write errors to the caller instead of discarding
them; only commit the HTTP status after serialization succeeds.
In `@platform-api/pkg/api/response.go`:
- Around line 18-25: Update the response-writing function around json.Marshal so
serialization failures write the centralized 500 error response before
returning, ensuring handlers do not leave the client with an empty default 200
response. Keep post-commit failures from w.Write as returned errors for logging,
and preserve the existing successful JSON response flow.
In `@platform-api/pkg/handlers/accounts.go`:
- Line 89: Remove the customer identifier fields from the success logs: in
platform-api/pkg/handlers/accounts.go:89-89, update the account-enabled log to
omit req.AccountID; in platform-api/pkg/handlers/management_cluster.go:94-94,
update the corresponding success log to omit accountID and mc.Name. Retain only
non-sensitive fields or use an approved correlation identifier.
In `@platform-api/pkg/handlers/health.go`:
- Around line 32-42: Handle and log every error returned by api.Write in
HealthHandler.Readiness and the corresponding health handler code at
platform-api/pkg/handlers/health.go lines 32-42, adding or reusing the handler
logger; also add the logger and log the api.Write error at
platform-api/pkg/handlers/info.go line 36. Do not discard these write errors,
including both readiness response paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e360b1f-1ba8-4763-b93e-c30a8e778fdd
📒 Files selected for processing (28)
hyperfleet-operator/internal/render/nodepool_test.goplatform-api/pkg/api/error.goplatform-api/pkg/api/error_test.goplatform-api/pkg/api/response.goplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/handlers/accounts.goplatform-api/pkg/handlers/authz.goplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/cluster_test.goplatform-api/pkg/handlers/errorcodes.goplatform-api/pkg/handlers/health.goplatform-api/pkg/handlers/info.goplatform-api/pkg/handlers/info_test.goplatform-api/pkg/handlers/management_cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/handlers/zoa.goplatform-api/pkg/handlers/zoa_test.goplatform-api/pkg/middleware/account_check.goplatform-api/pkg/middleware/admin_check.goplatform-api/pkg/middleware/admin_check_test.goplatform-api/pkg/middleware/authorization.goplatform-api/pkg/middleware/authorization_test.goplatform-api/pkg/middleware/authz.goplatform-api/pkg/middleware/errorcodes.goplatform-api/pkg/middleware/privileged.goplatform-api/pkg/ratelimit/middleware.goplatform-api/pkg/ratelimit/middleware_test.goplatform-api/pkg/validation/field_validator.go
gdbranco
force-pushed
the
fix/rosaeng-62084-platform-api-error-enumeration
branch
from
August 12, 2026 21:15
e0d4450 to
66552e2
Compare
Moves APIError, WriteError, and builders (WithReason, WithErrors) from pkg/apierror into pkg/api alongside the new api.Write for success responses. All handlers now route every response — success, no-content, and error — through these two functions instead of open-coded w.Header/w.WriteHeader/json.Encode triples. api.Write marshals to a buffer before committing headers so a marshal failure can still return a 500; write errors after headers are committed are returned to the caller for logging rather than silently discarded.
gdbranco
force-pushed
the
fix/rosaeng-62084-platform-api-error-enumeration
branch
from
August 12, 2026 21:17
66552e2 to
0da7ade
Compare
…leware WriteError now uses marshal-before-commit and returns error. All writeAPIError and writeError wrappers accept a *slog.Logger and log failures. InfoHandler and HealthHandler gain logger fields, removing the last TODO stubs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/pkg/api/errorcodes.go`:
- Around line 19-23: Handle the error returned by json.Marshal when initializing
fallbackBody in the error-code initialization flow. Ensure a marshal failure
cannot leave fallbackBody nil: either propagate the initialization error or fail
fast, while preserving the existing fallback payload for successful marshaling.
In `@platform-api/pkg/handlers/redact.go`:
- Around line 14-15: Update redact to calculate the midpoint and slice boundary
using runes rather than byte length, ensuring the returned string remains valid
UTF-8 for multibyte characters. Add a regression case in redact tests covering a
value such as “aé” and verify the expected redacted output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be9eed81-a175-4f10-80bc-6590e506fce8
📒 Files selected for processing (7)
platform-api/pkg/api/error.goplatform-api/pkg/api/errorcodes.goplatform-api/pkg/api/response.goplatform-api/pkg/handlers/accounts.goplatform-api/pkg/handlers/management_cluster.goplatform-api/pkg/handlers/redact.goplatform-api/pkg/handlers/redact_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- platform-api/pkg/api/response.go
- platform-api/pkg/handlers/management_cluster.go
- platform-api/pkg/handlers/accounts.go
- platform-api/pkg/api/error.go
…action - WriteError returns error and writes fallback 500 on marshal failure - Write sends centralized 500 via WriteError when json.Marshal fails - fallbackBody pre-computed in errorcodes.go init() to avoid init ordering bug - Redact first half of customer identifiers in success logs
gdbranco
force-pushed
the
fix/rosaeng-62084-platform-api-error-enumeration
branch
from
August 12, 2026 21:56
6e1553f to
a4c76f4
Compare
cdoan1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@gdbranco: This pull request references ROSAENG-62084 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Collaborator
|
/approve |
openshift-ci
Bot
added
the
approved
Collaborator
|
/lgtm |
Contributor
Author
|
/retest |
Assert status code explicitly and update code field from "429" to "RATE-LIMIT-001" to match the typed error definition.
Contributor
Author
|
/test on-demand-e2e |
…r codes Replace stale legacy strings with the actual typed codes: - "missing-target-cluster" → ZOA-CREATE-003 - "missing-jira" → ZOA-CREATE-004 - "invalid-jira" → ZOA-CREATE-005 - "invalid-params" → ZOA-CREATE-006 - "write-cooldown" → ZOA-CREATE-007 - "account-exists" → ACCOUNTS-MGMT-CREATE-004
Contributor
Author
|
/test on-demand-e2e |
Collaborator
|
@gdbranco I verified the e2e locally |
Collaborator
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, gdbranco 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 |
openshift-merge-bot
Bot
merged commit Aug 13, 2026
9b4e391
into
openshift-online:main
14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests