Skip to content

fix(retention): return 404 for missing policies - #23592

Open
rkthtrifork wants to merge 8 commits into
goharbor:mainfrom
rkthtrifork:fix/retention-policy-not-found
Open

rkthtrifork wants to merge 8 commits into
goharbor:mainfrom
rkthtrifork:fix/retention-policy-not-found

Conversation

@rkthtrifork

@rkthtrifork rkthtrifork commented Jul 21, 2026 •

Copy link
Copy Markdown

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Return 404 Not Found when 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 returned 500 Internal Server Error.

This change:

  • Wraps missing retention policies with Harbor's standard NOT_FOUND error using orm.WrapNotFoundError.
  • Propagates the typed error through the retention manager.
  • Documents the 404 response in the API specification.
  • Adds DAO and manager assertions covering the error classification and message.

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:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Copilot AI review requested due to automatic review settings July 21, 2026 10:20
@rkthtrifork
rkthtrifork requested a review from a team as a code owner July 21, 2026 10:20
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
@rkthtrifork
rkthtrifork force-pushed the fix/retention-policy-not-found branch from 5bed78a to 3ede672 Compare July 21, 2026 10:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 404 response.

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.

Comment thread src/pkg/retention/manager_test.go
Signed-off-by: Rasmus Kock Thygesen <rasmus.thygesen.privat@gmail.com>
Copilot AI review requested due to automatic review settings July 21, 2026 10:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 22, 2026 07:47
@wy65701436 wy65701436 added the release-note/update Update or Fix label Jul 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 27, 2026 08:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@codecov

codecov Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.39%. Comparing base (89dbf8f) to head (2d90c2e).
⚠️ Report is 106 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
unittests 66.39% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/pkg/retention/dao/retention.go 60.00% <100.00%> (ø)
src/pkg/retention/manager.go 75.47% <ø> (+0.92%) ⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread api/v2.0/swagger.yaml
$ref: '#/responses/401'
'403':
$ref: '#/responses/403'
'404':

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 404 response is added only to getRetention, but this change affects more endpoints than that.

  • updateRetention — controller UpdateRetention calls manager.GetPolicy
  • deleteRetention — DeleteRetention calls manager.GetPolicy

I am not go through them all, but please. thanks.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took a while for me to get back. I believe i have fixed what you asked about.

@wy65701436 wy65701436 added release-note/breaking-change Breaking changes in the release and removed release-note/update Update or Fix labels Jul 27, 2026
@github-actions

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the Stale label Sep 26, 2026
winrarr and others added 4 commits September 28, 2026 14:27
@rkthtrifork

Copy link
Copy Markdown
Author

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.

This PR is not stale

@github-actions github-actions Bot removed the Stale label Sep 29, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants