Proposal: Fix improper logging and error handling

[vg006]

Overview

This is a proposal to update logger and fix error handling in general.

Origin

I have been using harbor CLI to manage my local registry for a while. So when I was trying to visit the audit logs of my registry using harbor logs, the process exited with a status code 1 without any proper logging or reason.

Then after some time I found that the registry hasn't been started using harbor health (That's my bad), but though there isn't any proper response from the tool. So I just started to test other commands and their sub-commands, where I get varied response and logging.

Surprisingly upon executing harbor user elevate, The program just panicked.

Also even the harbor user create just gets all the inputs and exits without proper logging, irrespective of the status of the registry.

More Context

When I was working on this #634, the logger didn't log anything, as the logrus logger didn't set the output writer properly.
This may be a quick fix, but there are lot of lines of code performing logging, scattered across files.

Additionally, the logrus logger library which is being used in the project, has been in maintenance mode for nearly 6 years. (ref)

Due to these, the errors neither handled properly nor being logged. And at some places, errors are simply ignored, which isn't a good practice. (example).

Proposal

With all the above considerations in mind, I suggest to,

1. Use the Charm's log library.

I suggest using the above lib, as the TUI is developed using Charm's components, styles and will be easy to set the theme across the CLI entirely

2. Build a logger package using Cobra CLI's builtin stdout & stderr writter.
3. Or use of any other logger such as Zerolog, Zap.

And for errors, I would highly suggest to define an internal errors package listing and mapping all the possible errors, that the tool may encounter. Like how it is implemented properly in the Harbor and its go-client SDK itself. (example)

So request the maintainers and the community to go through this proposal and share your thoughts and recommendations in this issue thread.

cc: @bupd @Vad1mo @OrlinVasilev @qcserestipy @NucleoFusion

[NucleoFusion]

I think the reason we miss logs or dont show it is because :-

• Older commands dont use RunE and instead rely on Run + custom logging.
• Error cases are not handled properly, such as those shown in which a nil ptr deref

And on the seldom cases that you would need a logger, we have logrus as a dependency already.

What do you think @bupd ?
In contrast, an errors package doenst sound bad to me

[vg006]

• I agree. Using RunE, instead of Run and returning wrapped errors would give us a single place to log and handle failures.
• Yeah we do have logrus, but I prefer to replace with log, as it is highly customizable and could configure it to align with the theme of the CLI. So which will enhance consistency of the TUI across the CLI.
• Also as the CLI is built using multiple Charm's libs like bubbletea, bubbles, lipgloss, huh, I think it is good to add log to the list, replacing logrus, as it is heavy and being unmaintained for a long time. So I feel it is good to migrate to something better.

[NucleoFusion]

logrus, as it is heavy and being unmaintained for a long time

Ohh, wasnt aware its unmaintained mb

[bupd]

I would say its unmaintained - currently its in maintenance mode meaning it will not get any new features - https://github.com/sirupsen/logrus#:~:text=Logrus%20is%20in%20maintenance%2Dmode

@vg006
yeah it would be good to use - https://go.dev/blog/slog - though its a new logger officially from go. this would be sufficient.

[vg006]

Yeah I support it, the log/slog std package is sufficient enough. So we could even prettify the logs using the lipgloss lib, which is already being used in the project.
Thanks for the suggestion @bupd

[samruddhi-Rahegaonkar]

Hi @bupd 👋, I submitted PR #661 addressing error handling in the user elevate command (fixing issue #655).
That change ensures the elevate command returns an error if the user does not exist, preventing execution in absence of users.
Please let me know if there are additional scenarios under this issue that should be handled. Thanks!

[qcserestipy]

@samruddhi-Rahegaonkar Thanks for the PR. Please note that this issue focuses on the overall logging and error-handling strategy rather than individual command fixes. PR #661 relates to issue #655 and would be better discussed there to keep this thread focused.