-
Notifications
You must be signed in to change notification settings - Fork 71
Refactor: Make Error logging more user friendly for project
commands
#424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Rizul Gupta <[email protected]>
project
commandsproject
commands
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
@bupd, are the logs okay when using the |
There was a problem hiding this comment.
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 refactors the Harbor CLI error handling for project-related commands to make error logging more user friendly and consistent. Key changes include:
- Introducing a centralized error parsing utility (ParseHarborError) and a new CheckProject function.
- Replacing Run with RunE in commands to allow direct error returns and streamlined error handling.
- Cleaning up code structure and logging for project operations such as view, list, delete, and create.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/utils/error.go | Added ParseHarborError to centralize error parsing logic |
pkg/api/project_handler.go | Updated DeleteProject parameter (using useProjectName) |
cmd/harbor/root/project/view.go | Replaced Run with RunE and integrated CheckProject for existence check |
cmd/harbor/root/project/search.go | Updated error handling in project search command |
cmd/harbor/root/project/logs.go | Enhanced existence check and improved debug logging in logs command |
cmd/harbor/root/project/list.go | Improved pagination logging and error handling in project listing |
cmd/harbor/root/project/delete.go | Refined delete command with concurrent deletion and detailed reporting |
cmd/harbor/root/project/create.go | Streamlined project creation with interactive prompt and flag handling |
cmd/harbor/root/cmd.go | Updated logger configuration to respect verbose mode settings |
Comments suppressed due to low confidence (1)
cmd/harbor/root/project/create.go:35
- [nitpick] Consider using a lower-case variable name (projectName) for local variables to follow Go naming conventions.
var ProjectName string
Co-authored-by: Copilot <[email protected]> Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rizul2108 great job on the verbose logging.
But yet we recieve multiple error messages
instead of having multiple error messages can we have a single user frienldy message with a single specific reason.
Instead of
❯ ./bin/harbor-dev project list
ERRO Error fetching project page error="current-credential-name is not set in config file"
Error: failed to get projects list: Error: current-credential-name is not set in config file
~/s/code/OSS/harbor-cli/prss fix-project-error
❯ ./bin/harbor-dev project list -v
DEBU[2025-04-22T05:18:02+05:30] Starting project list command
DEBU[2025-04-22T05:18:02+05:30] Using list all projects function
DEBU[2025-04-22T05:18:02+05:30] Fetching projects...
DEBU[2025-04-22T05:18:02+05:30] Page size is 0, will fetch all pages
DEBU[2025-04-22T05:18:02+05:30] Fetching next page of projects page=1 page_size=100
ERRO[2025-04-22T05:18:02+05:30] Error fetching project page error="current-credential-name is not set in config file"
Error: failed to get projects list: Error: current-credential-name is not set in config file
We should have
Error: credentials not found, please login
error messages should be simple and concise
Co-authored-by: Prasanth Baskar <[email protected]> Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
There was a problem hiding this comment.
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 refactors error handling for Harbor CLI project commands to provide clearer, consistent, and more user‐friendly error messages.
- Centralized error parsing via the new ParseHarborError utility
- Updated command implementations to use RunE and proper error returns
- Enhanced logging and refined project existence checks and deletion concurrency
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/views/project/create/view.go | Refactored to return errors instead of calling log.Fatal |
pkg/utils/error.go | Introduced ParseHarborError for centralized error message parsing |
pkg/utils/client.go | Updated error message with a login hint |
pkg/api/project_handler.go | Modified flag handling for project deletion |
cmd/harbor/root/project/view.go | Refactored project view command with existence check and error returns |
cmd/harbor/root/project/search.go | Improved error handling in search command |
cmd/harbor/root/project/logs.go | Updated logs command with enhanced debug logging and error returns |
cmd/harbor/root/project/list.go | Added pagination debug logs and refined project listing |
cmd/harbor/root/project/delete.go | Enabled batch deletion with concurrency and detailed outcome reporting |
cmd/harbor/root/project/create.go | Supported both flag-based and interactive project creation |
cmd/harbor/root/cmd.go | Streamlined logrus formatter initialization |
Comments suppressed due to low confidence (1)
cmd/harbor/root/project/logs.go:75
- The symbol 'auditLog' is used but not defined or imported; ensure the correct package name or import is provided to resolve this error.
auditLog.LogsProject(resp.Payload)
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
There was a problem hiding this comment.
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 refactors error logging and handling across the Harbor CLI to improve user friendliness and consistency. Key changes include introducing a centralized error parser (ParseHarborError), replacing Run with RunE in multiple commands, and enhancing prompt functions to return errors.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pkg/prompt/prompt.go | Updated GetProjectNameFromUser signature to return an error alongside the value. |
pkg/api/project_handler.go | Adjusted parameters for DeleteProject and added CheckProject for project checks. |
cmd/harbor/root/* (various commands) | Replaced Run with RunE and improved prompt error handling and logging. |
Co-authored-by: Copilot <[email protected]> Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
…nto fix-project-error
Signed-off-by: Rizul Gupta <[email protected]> lint fix Signed-off-by: Rizul Gupta <[email protected]>
74762a6
to
5ac73cc
Compare
@bupd this PR is ready for review |
There was a problem hiding this comment.
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 refactors error handling in the Harbor CLI to improve user output and consistency. Key changes include centralizing error parsing via ParseHarborError, transitioning commands to use RunE for direct error returns, and adding a new CheckProject function to validate project existence before further processing.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pkg/prompt/prompt.go | Updated GetProjectNameFromUser to return errors along with the name. |
pkg/api/project_handler.go | Adjusted project deletion to use a flag inversion for resource naming. |
cmd/harbor/root/* | Refactored various commands to adopt RunE and standardized error logging and handling. |
Co-authored-by: Copilot <[email protected]> Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
Signed-off-by: Rizul Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
@bupd ready for review |
Overview
This PR introduces a cleaner, more reliable, and consistent approach to error handling across the Harbor CLI. It standardizes parsing and reporting API errors, making the CLI more user-friendly and developer-maintainable.
Key Changes
Added
ParseHarborError
utilityPayload
from Harbor API responses.Replaced
Run
withRunE
across commandsParseHarborError
function for consistent logging.Code cleanup and simplification
Some Errors are not documented in some of the API requests by harbor so they gave response like below
So added a new func
CheckProject
which calls APIheadProject
to check if project existsMotivation
project
sub-commands.Error Output Before vs After
Before:
After:
Related Issues
Fix #316
Fix #253
Fix #262
Fix #431
Fix #436
Fix #440