fix(retention): return 404 for missing policies - #23592
rkthtrifork wants to merge 8 commits into
Conversation
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
5bed78a to
3ede672
Compare
There was a problem hiding this comment.
Pull request overview
Updates retention-policy lookup failures to return Harbor’s typed 404 Not Found response.
Changes:
- Wraps missing DAO records as not-found errors.
- Propagates and tests typed errors.
- Documents the API’s
404response.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/pkg/retention/manager.go |
Propagates typed DAO errors. |
src/pkg/retention/manager_test.go |
Verifies not-found classification and message. |
src/pkg/retention/dao/retention.go |
Wraps missing policies as not-found errors. |
src/pkg/retention/dao/retention_test.go |
Tests DAO error behavior. |
api/v2.0/swagger.yaml |
Documents the GET 404 response. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23592 +/- ##
==========================================
+ Coverage 66.38% 66.39% +0.01%
==========================================
Files 1073 1073
Lines 117757 117770 +13
Branches 2965 2965
==========================================
+ Hits 78170 78192 +22
+ Misses 35284 35275 -9
Partials 4303 4303
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| $ref: '#/responses/401' | ||
| '403': | ||
| $ref: '#/responses/403' | ||
| '404': |
There was a problem hiding this comment.
The 404 response is added only to getRetention, but this change affects more endpoints than that.
updateRetention— controllerUpdateRetentioncallsmanager.GetPolicydeleteRetention—DeleteRetentioncallsmanager.GetPolicy
I am not go through them all, but please. thanks.
There was a problem hiding this comment.
Sorry it took a while for me to get back. I believe i have fixed what you asked about.
|
This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days. |
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
This PR is not stale |
Thank you for contributing to Harbor!
Comprehensive Summary of your change
Return
404 Not Foundwhen requesting a retention policy that does not exist.Previously, the retention DAO returned Beego's raw
orm.ErrNoRows, and the retention manager converted it into an untyped error. Harbor's shared HTTP error responder therefore classified the error as unknown and returned500 Internal Server Error.This change:
NOT_FOUNDerror usingorm.WrapNotFoundError.404response in the API specification.This matches the existing behavior and implementation used by other ID-based resources such as robots and quotas.
Issue being fixed
No existing issue.
Please indicate you've done the following: