refac: `artifact list` command with `errors` package by vg006 · Pull Request #780 · goharbor/harbor-cli

vg006 · 2026-03-31T13:59:37Z

Description

This PR demonstrates the implementation of the project's pkg/errors package in the #636, by replacing the existing non-uniform error return statements with the pkg/errors package APIs.

Fixes Proposal: Fix improper logging and error handling #635

Type of Change

Changes

Replace the return statements with fmt.Errorf, errors.New with the built-in pkg/errors

Overview

codecov · 2026-03-31T14:02:57Z

Codecov Report

❌ Patch coverage is 65.26316% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.61%. Comparing base (60ad0bd) to head (27bfaa7).
⚠️ Report is 123 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/utils/config.go	10.52%	17 Missing ⚠️
pkg/utils/encryption.go	5.88%	16 Missing ⚠️
pkg/errors/utils.go	48.27%	12 Missing and 3 partials ⚠️
pkg/errors/errors.go	93.63%	5 Missing and 2 partials ⚠️
cmd/harbor/root/artifact/list.go	0.00%	6 Missing ⚠️
pkg/utils/client.go	40.00%	3 Missing ⚠️
pkg/prompt/prompt.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main    #780      +/-   ##
=========================================
- Coverage   10.99%   8.61%   -2.38%     
=========================================
  Files         173     271      +98     
  Lines        8671   13294    +4623     
=========================================
+ Hits          953    1145     +192     
- Misses       7612   12031    +4419     
- Partials      106     118      +12

☔ 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.

NucleoFusion · 2026-03-31T14:16:28Z

It would be nice to also have a reference for the API handlers for that to Hint etc like how chaining would work

NucleoFusion · 2026-04-02T04:16:40Z

cmd/harbor/root/artifact/list.go

 				projectName, repoName, err = utils.ParseProjectRepo(args[0])
 				if err != nil {
-					return fmt.Errorf("failed to parse project/repo: %v", err)
+					return errors.New("Invalid project/repository format", "expected format: <project>/<repository>")


This and other instances of this are wrong no? since we would use the WithHint or WithMessage here instead of a new error?

Actually, the APIs are changed a bit. WithHint is removed and merged with WithMessage which accepts the hints optionally.

In the previous style, at different layers, we could add both messages and hints optionally. So this caused issues in rendering the tree view.

Thus, I updated the WithMessage method to accept the hints optionally, but the messages aren't optional. So hints are passed only if a message is conveyed at that level.

You can check the signature of WithMessage here

yeah but shouldn't it still chain here, instead of returning a new one?

That's my bad. Thanks for noting it down.

Actually for initial testing, I refactored simple errors in the artifact/list.go and this is one of such. After I refactored all returns functions that are invoked at each layer. So I forgot to replace the initial one with the actually refactored one at the primitive layer. I have updated it now.

You can view the related changes here [1][2]

vg006 · 2026-04-07T07:17:44Z

@NucleoFusion The exact errors that will be returned by the API calls are quite hard to reproduce. So, if the sample tree needed to be visualized, it can be seen the in the preview of this PR #636.

But the least possible demonstration is shown below, which is produced when the CLI makes API calls to Harbor instance that is down.

Here the error is raised while attempting to fetch project name.

For simplicity, use could remove the message add here, so render the following.

Signed-off-by: vg006 <devvg006@gmail.com>

qcserestipy

Thanks for the contribution! Looks very cool. I just left a few comments here and there.

qcserestipy · 2026-04-07T16:49:26Z

pkg/errors/errors.go

+	Hints   []string
+}
+
+type Error struct {


I really like this package and think it is great. I wanted to note that it might be worth to document that we are shadowing the std errors package and explicitely telling people that they have to be careful about it (Like making a dedicated docs md for this).
A good way to let people do this difference could be this:

import ( stderrors "errors" "github.com/goharbor/harbor-cli/pkg/errors" )

Thank you for supporting my proposal. Yeah, maintaining proper documentation for this package will help maintainers and contributors to refactor either the project or the package itself. I think adding that doc to the CONTRIBUTING.md will inform the new contributors at the first place, if they work on this package or try to handle errors.

Also yeah, this package wraps the std errors, enforces to use it rather for uniformity across the codebase. Thus we could instruct the contributors to use this package instead of the std. So there will be only,

import ( "github.com/goharbor/harbor-cli/pkg/errors" )

qcserestipy · 2026-04-07T16:57:17Z

pkg/views/styles.go

+)
+
+const (
+	GreenANSI = "\033[32m"


To me it looks like that those colors and styles are already defined further up. Also, there are not used anywhere.

Yeah the styles that are declared using the lipgloss library are the ones that I introduced newly. But the hardcoded ANSI sequences were defined earlier in the project itself and removed by @NucleoFusion last month (ref). I don't know why, but I suspect an issue while rebasing. I will check it and clean it up.

qcserestipy · 2026-04-08T19:26:03Z

pkg/errors/errors.go

+		parts = append(parts, hintsTree.String())
+	}
+
+	if len(e.frames) > 1 {


It would be great if you could test whether the tree structure output breaks in pipeline environments. I do not know whether all terms or envs would support this or expect single line error messages. Maybe @bupd and @NucleoFusion have some ideas, too.

This tree output is planned to only be returned on non-verbose mode, where it's used by a real user. But for the CI and any other environment, the verbose mode of logging will be used I guess. So this tree won't be renderded, rather the error will be detailed by the logger as in the PR #722.

Even if CI or other envs use non-verbose mode, it will work I believe, because the ANSI sequences are valid UTF-8 encoded, won't disrupt tools like grep, rg parsing them, when piped the harbor output in their scripts.

For example, I tested the following in the local env to check compatibility with grep

package main import ( "fmt" "github.com/charmbracelet/lipgloss" ) func main() { style := lipgloss.NewStyle(). Bold(true). Foreground(lipgloss.Color("205")). Padding(1) output := style.Render("Hello, Pipeline!") fmt.Println(output) }

Preview

vg006 · 2026-04-09T13:11:28Z

Thank you @qcserestipy for supporting and reviewing the package. However, the above comments are very specific to the errors package that we are developing in the PR #636, as this PR is a sample, demonstrating the errors package by refactoring the artifact list command.

As you really interested in the betterment of this package, I really appreciate you to look into it PR #636 and will be happy to discuss further in there, will be helpful to all track relevant reviews and changes properly, that's why.

Thank you.

NucleoFusion · 2026-04-09T13:33:44Z

Thank you @qcserestipy for supporting and reviewing the package. However, the above comments are very specific to the errors package that we are developing in the PR #636, as this PR is a sample, demonstrating the errors package by refactoring the artifact list command.

As you really interested in the betterment of this package, I really appreciate you to look into it PR #636 and will be happy to discuss further in there, will be helpful to all track relevant reviews and changes properly, that's why.

Thank you.

Isnt this PR much suited for merging? since we also get a reference material which the umbrella issue can mention and even other contributors can pick up on?

I think the changes (aside from the errors package) are simple enough that we wont need to spend a lot of extra time reviewing it.
@qcserestipy what do you think?

vg006 · 2026-04-09T13:44:22Z

Yeah @NucleoFusion,

This PR is partially ready to merge, as it needs some more subtle changes.
This could be used as a referrence and could create an umbrella issue, tracking all the migrations.
Also the migration is quite simple and easier to review and manage, if we move command-by-command.

qcserestipy self-requested a review April 1, 2026 17:46

NucleoFusion reviewed Apr 2, 2026

View reviewed changes

vg006 requested a review from NucleoFusion April 7, 2026 07:17

vg006 added 11 commits April 7, 2026 17:16

feat(pkg): Initialize package errors

ea6f8ae

Signed-off-by: vg006 <devvg006@gmail.com>

feat(pkg): Add error type and methods

a8165cd

Signed-off-by: vg006 <devvg006@gmail.com>

refac(pkg): Update error parse with utils

53c1577

Signed-off-by: vg006 <devvg006@gmail.com>

fix(pkg): Add utilities to fix cycle import

4fe4418

Signed-off-by: vg006 <devvg006@gmail.com>

feat(pkg): Add functions and methods to errors

74fb55b

Signed-off-by: vg006 <devvg006@gmail.com>

fix(pkg): Update error package and Lint

a6151e9

Signed-off-by: vg006 <devvg006@gmail.com>

refac(pkg): Update the Error structure

d8978b1

Signed-off-by: vg006 <devvg006@gmail.com>

feat(pkg): Add tests to the errors package

f03af0e

Signed-off-by: vg006 <devvg006@gmail.com>

refac(pkg): update the methods, tests and styles

7c9788e

Signed-off-by: vg006 <devvg006@gmail.com>

refac: update return statements with pkg/errors

56380a9

Signed-off-by: vg006 <devvg006@gmail.com>

fix: avoid new error creation

27bfaa7

Signed-off-by: vg006 <devvg006@gmail.com>

vg006 force-pushed the refac/alist branch from 14b2db7 to 27bfaa7 Compare April 7, 2026 11:47

qcserestipy reviewed Apr 8, 2026

View reviewed changes

Conversation

vg006 commented Mar 31, 2026

Description

Type of Change

Changes

Overview

Uh oh!

codecov bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NucleoFusion commented Mar 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vg006 commented Apr 7, 2026

Uh oh!

qcserestipy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vg006 Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Preview

Uh oh!

vg006 commented Apr 9, 2026

Uh oh!

NucleoFusion commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vg006 commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 31, 2026 •

edited

Loading

vg006 Apr 9, 2026 •

edited

Loading

NucleoFusion commented Apr 9, 2026 •

edited

Loading