Skip to content

fix(retention): prevent failures in tag retention rule deletion flow#996

Open
gcharpe1604 wants to merge 5 commits into
goharbor:mainfrom
gcharpe1604:fix/tag-retention-deletion-safety
Open

fix(retention): prevent failures in tag retention rule deletion flow#996
gcharpe1604 wants to merge 5 commits into
goharbor:mainfrom
gcharpe1604:fix/tag-retention-deletion-safety

Conversation

@gcharpe1604

Copy link
Copy Markdown
Contributor

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Documentation update
  • Chore / maintenance

Description / Quick Summary

This PR safeguards the tag retention rule deletion flow against unhandled errors. By propagating errors from API calls and prompt handlers up to Cobra's RunE boundary, the CLI now exits gracefully when rule lists are empty, when API calls fail, or when the user cancels the interactive prompt.


Problem

  • What issue exists?
    • prompt.GetRetentionTagRule() would block indefinitely on connection failures and did not return errors to its caller.
    • The deletion command assumed retention rules were always found, resulting in potential errors when indexing empty payloads.
    • api.DeleteRetention() executed redundant network lookups to resolve project names to retention IDs when the ID was already available.
  • Who is affected? Users attempting to view or delete tag retention rules when network/connection problems occur, or when no retention rules are configured on the project.
  • Current Behavior: The CLI could exit unexpectedly, block, or fail to surface actionable errors to the user.
  • Desired Behavior: All errors should propagate gracefully up to Cobra, printing user-friendly error messages and exiting with code 1.

Root Cause

  • Relevant Files:
    • cmd/harbor/root/tag/retention/delete.go
    • pkg/api/retention_handler.go
    • pkg/prompt/prompt.go
    • pkg/views/retention/select/view.go
  • Technical Explanation:
    The command and API wrapper layers did not check return errors from api.ListRetention() and did not propagate the error up to the Cobra RunE boundary.

Solution

  • Overview:
    1. Updated prompt.GetRetentionTagRule signature to return (int, error) and check for empty rules.
    2. Simplified DeleteRetention to use the already-available retention ID and removed a redundant lookup request.
    3. Propagated selection aborts and API errors up to cmd/harbor/root/tag/retention/delete.go.

Scope

Included

  • Returning errors from GetRetentionTagRule and propagating them in the delete command.
  • Optimizing DeleteRetention client helper to use direct ID.

Explicitly Not Included

  • Core selection model modification (no changes are made to pkg/views/base/selection/model.go to keep this PR strictly isolated).

Changes

  • cmd/harbor/root/tag/retention/delete.go: Added error check for prompt.GetRetentionTagRule and updated api.DeleteRetention invocation to use the direct retention ID.
  • pkg/api/retention_handler.go: Simplified DeleteRetention signature to accept retentionID directly and removed the redundant call to GetRetentionId.
  • pkg/prompt/prompt.go: Refactored GetRetentionTagRule to return (int, error) and added checks for API errors and empty rule lists.
  • pkg/views/retention/select/view.go: Refactored RetentionList to return (int, error) and propagate selection errors.

Testing

Unit Tests

  • All existing package tests pass (go test ./...)

Manual Verification

  1. Configured local dummy server.
  2. Ran .\harbor_retention.exe tag retention delete --project <project_name>.
  3. Observed the graceful exit and correct error reporting on connection refusal.

Risks & Compatibility

  • Backward Compatibility: Non-breaking. Enhances error propagation without changing successful prompt behavior.
  • Potential Risks: None identified.

Signed-off-by: Govind Charpe <govind.charpe16@gmail.com>
@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 9.29%. Comparing base (60ad0bd) to head (e9c1dcb).
⚠️ Report is 188 commits behind head on main.

Files with missing lines Patch % Lines
pkg/views/retention/select/view.go 0.00% 11 Missing ⚠️
pkg/api/retention_handler.go 0.00% 8 Missing ⚠️
pkg/prompt/prompt.go 0.00% 8 Missing ⚠️
cmd/harbor/root/tag/retention/delete.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             main    #996      +/-   ##
=========================================
- Coverage   10.99%   9.29%   -1.71%     
=========================================
  Files         173     321     +148     
  Lines        8671   16092    +7421     
=========================================
+ Hits          953    1495     +542     
- Misses       7612   14462    +6850     
- Partials      106     135      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines -130 to -135
retentionIDStr, err := GetRetentionId(projectName, true)
if err != nil {
return err
}

retentionResp, err := ListRetention(retentionIDStr)

@NucleoFusion NucleoFusion Jun 20, 2026

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.

wait, why are we removing this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The caller in delete.go already resolves the retention ID via GetRetentionId() before invoking DeleteRetention. The removed lines were performing the same lookup a second time. By accepting retentionID directly, we eliminate a redundant network call.

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.

okay, nice

Comment thread pkg/prompt/prompt.go
Comment thread pkg/views/retention/select/view.go Outdated
…ling

Signed-off-by: Govind Charpe <govind.charpe16@gmail.com>
Copilot AI review requested due to automatic review settings June 21, 2026 07:00
@gcharpe1604 gcharpe1604 requested a review from NucleoFusion June 21, 2026 07:01

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

This PR hardens the harbor tag retention delete flow by propagating prompt/API failures up to Cobra’s RunE boundary and simplifying the retention deletion API helper to avoid redundant lookups.

Changes:

  • Refactors the retention-rule selection UI to return (int64, error) instead of exiting the process.
  • Updates the prompt helper to surface API failures (and handle empty rule lists) rather than blocking/silent behavior.
  • Simplifies api.DeleteRetention to accept a resolved retention ID directly and updates the command to use it.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
cmd/harbor/root/tag/retention/delete.go Updates delete command to handle selection errors and pass retention ID directly to deletion helper.
pkg/api/retention_handler.go Changes DeleteRetention to accept retentionID directly and adjusts rule removal logic to use an int64 index.
pkg/prompt/prompt.go Refactors GetRetentionTagRule to return (int64, error) and adds handling for API errors/empty rule lists.
pkg/views/retention/select/view.go Refactors retention selection view to return errors instead of calling os.Exit, and introduces abort/empty-selection handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/views/retention/select/view.go
Comment thread pkg/views/retention/select/view.go
Comment thread pkg/prompt/prompt.go
Comment thread pkg/api/retention_handler.go Outdated
Signed-off-by: Govind Charpe <govind.charpe16@gmail.com>
@gcharpe1604 gcharpe1604 force-pushed the fix/tag-retention-deletion-safety branch from 5b51cf4 to 96546b8 Compare June 21, 2026 07:31
Signed-off-by: Govind Charpe <govind.charpe16@gmail.com>
Comment thread pkg/api/retention_handler.go Outdated
Comment thread pkg/prompt/prompt.go Outdated
@gcharpe1604 gcharpe1604 requested a review from NucleoFusion June 21, 2026 10:55
…command layer

Signed-off-by: Govind Charpe <govind.charpe16@gmail.com>
@gcharpe1604 gcharpe1604 force-pushed the fix/tag-retention-deletion-safety branch from 790ec71 to e9c1dcb Compare June 21, 2026 11:17
@qcserestipy qcserestipy added status/in-progress Work on this issue has started or a linked pull request is actively being developed. status/needs-feedback Waiting for input from the issue author or reporter. May be closed after 7 days. labels Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/in-progress Work on this issue has started or a linked pull request is actively being developed. status/needs-feedback Waiting for input from the issue author or reporter. May be closed after 7 days.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: Errors not propagated in tag retention rule deletion flow

4 participants