Skip to content

fix: reject invalid page values in selected list commands#779

Merged
Vad1mo merged 4 commits into
goharbor:mainfrom
PrasunaEnumarthy:fix/page-validation
Apr 17, 2026
Merged

fix: reject invalid page values in selected list commands#779
Vad1mo merged 4 commits into
goharbor:mainfrom
PrasunaEnumarthy:fix/page-validation

Conversation

@PrasunaEnumarthy

@PrasunaEnumarthy PrasunaEnumarthy commented Mar 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request adds early validation for invalid --page values in selected list commands.

Previously, some commands accepted invalid values such as --page 0 or negative page numbers and passed them to the API, which could lead to inconsistent behavior.

This change ensures that invalid page values are rejected before making API calls, improving CLI reliability and consistency.

Type of Change

Please select the relevant type.

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

Changes

  • Added validation to reject --page < 1 in:
    • harbor artifact list
    • harbor project list
    • harbor registry list
  • Used the same validation message as harbor project logs for consistency
  • Ensured validation occurs before API calls

@qcserestipy qcserestipy self-requested a review April 1, 2026 17:34
@codecov

codecov Bot commented Apr 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 41.17647% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 9.13%. Comparing base (60ad0bd) to head (c820468).
⚠️ Report is 132 commits behind head on main.

Files with missing lines Patch % Lines
cmd/harbor/root/replication/executions/list.go 0.00% 6 Missing ⚠️
cmd/harbor/root/logs.go 0.00% 2 Missing ⚠️
cmd/harbor/root/project/member/list.go 0.00% 2 Missing ⚠️
cmd/harbor/root/project/robot/list.go 0.00% 2 Missing ⚠️
cmd/harbor/root/replication/policies/list.go 0.00% 2 Missing ⚠️
cmd/harbor/root/repository/list.go 0.00% 2 Missing ⚠️
cmd/harbor/root/robot/list.go 0.00% 2 Missing ⚠️
cmd/harbor/root/user/list.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             main    #779      +/-   ##
=========================================
- Coverage   10.99%   9.13%   -1.86%     
=========================================
  Files         173     270      +97     
  Lines        8671   13195    +4524     
=========================================
+ Hits          953    1206     +253     
- Misses       7612   11876    +4264     
- Partials      106     113       +7

☔ View full report in Codecov by Sentry.
📢 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.

qcserestipy
qcserestipy requested changes Apr 1, 2026
View reviewed changes

@qcserestipy qcserestipy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for your contribution and having a look into this. It looks to me that this list of changed commands might not be complete:

Image

Can you please check all commands that contain opts.Page and make sure that all of them are checked properly.

@qcserestipy qcserestipy added the Changes Requesed feedback that must be addressed before merging. label Apr 1, 2026
@PrasunaEnumarthy

PrasunaEnumarthy commented Apr 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, I’ll expand the validation to all commands that accept opts.Page and add tests for the new checks.

@PrasunaEnumarthy
fix: reject invalid page values in selected list commands
b38f611 
Signed-off-by: PrasunaEnumarthy <eswari.prasuna@gmail.com>
@PrasunaEnumarthy
fix: validate page values across paginated commands
4766d04 
Signed-off-by: PrasunaEnumarthy <eswari.prasuna@gmail.com>
@PrasunaEnumarthy

PrasunaEnumarthy commented Apr 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, I’ve updated the PR to add the '--page >= 1' validation to all commands that define opts.Page, using the same error message as project logs.
I also added tests for the originally changed commands (artifact list, project list, and registry list) and verified them locally with:

go test ./cmd/harbor/root/artifact ./cmd/harbor/root/project ./cmd/harbor/root/registry

@PrasunaEnumarthy

PrasunaEnumarthy commented Apr 7, 2026

Copy link
Copy Markdown
Contributor Author

@qcserestipy can you please review when you have time...

@qcserestipy

qcserestipy commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

@PrasunaEnumarthy Thank you for taking that up, please fix the lint pipeline. If possible also please add more tests for the other list commands.

@PrasunaEnumarthy
test: add headers to page validation list tests
8713d11 
Signed-off-by: PrasunaEnumarthy <eswari.prasuna@gmail.com>
@PrasunaEnumarthy
test: add invalid page tests for more list commands
c820468 
Signed-off-by: PrasunaEnumarthy <eswari.prasuna@gmail.com>
@PrasunaEnumarthy

PrasunaEnumarthy commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@qcserestipy
I’ve fixed the lint issues and those checks are passing now.
I also added invalid-page tests for more paginated list commands (labels, instance, quota, and schedule).
The only failing check left is codecov/project, while codecov/patch is passing,so it seems related to overall coverage rather than this PR specifically.
Could you please take another look when you get a chance?

qcserestipy
qcserestipy approved these changes Apr 15, 2026
View reviewed changes

@qcserestipy qcserestipy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution!

@Vad1mo Vad1mo merged commit 52663b5 into goharbor:main Apr 17, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qcserestipy qcserestipy qcserestipy approved these changes

Assignees

No one assigned

Labels

Changes Requesed feedback that must be addressed before merging.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[bug]: Reject invalid page values in artifact, project, and registry list commands

3 participants

@PrasunaEnumarthy @qcserestipy @Vad1mo