Skip to content
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

Refactoring of logging #2576

Closed
20 tasks done
phillebaba opened this issue Jun 3, 2024 · 6 comments · Fixed by #3265
Closed
20 tasks done

Refactoring of logging #2576

phillebaba opened this issue Jun 3, 2024 · 6 comments · Fixed by #3265
Assignees
Milestone

Comments

@phillebaba
Copy link
Member

phillebaba commented Jun 3, 2024

Describe what should be investigated or refactored

Test week proved that certain functions are difficult because logging logic is tightly coupled with other logic. For example there are a couple of functions which will print formatted tables to a global logger instead of returning structured data and allowing the caller to deal with the output. Additionally there are a couple of places where errors are logged and exit is called instead of returning errors and allowing the called deal with them. We have also discovered that Pterm is not thread safe as documented in #2558 which causes more issues for unit testing.

For these reasons it has become apparent that some sort of refactoring of logging is required. A lot has happened since the initial implementation, like structured logging in standard lib. We need to define what logging is specific to the CLI and what is logging which is useful for those importing Zarf as a CLI. While it may complicate things we have to decide whether or not spinners and other "UI" elements should be used in any exported packages.

Links to any relevant code

https://github.com/defenseunicorns/zarf/blob/0c02643b9f5631a7bdd98650cc87c74901190771/src/pkg/utils/wait.go#L53
https://github.com/defenseunicorns/zarf/blob/0c02643b9f5631a7bdd98650cc87c74901190771/src/pkg/packager/common.go#L132

Additional context

We should split the work into a couple of steps to avoid creating a mega PR.

Tasks

  • Add logger pkg
  • Introduce --log-format flag and disable message when it's enabled.

Add logger lines alongside existing message lines

(Note, not all of these commands will have messages but they're here to ensure we check them all)

pkgr

  • zarf package create
  • zarf package deploy
  • zarf package inspect
  • zarf package list
  • zarf package publish
  • zarf package pull
  • zarf package mirror-resources
  • zarf package remove

init

  • zarf init

connect

  • zarf connect
  • zarf connect list

destroy

  • zarf destroy

dev

  • zarf dev deploy
  • zarf dev generate
  • zarf dev find-images
  • zarf dev lint
  • zarf dev sha256sum
  • zarf dev patch-git

Not needed

  • zarf version
  • zarf tools
  • zarf completion
  • zarf internal
  • zarf dev generate-config
@phillebaba
Copy link
Member Author

I realize that message fatal prints the debug stack when an error occurs which exits the program. Additionally some logs will differentiate between the message and the error. The error is only written to the debug logger which the message is written to the error writer.

Before removing this logic we need to determine if this is a feature required by end users.

@phillebaba phillebaba mentioned this issue Jun 6, 2024
2 tasks
@schristoff-du schristoff-du moved this to Backlog in Zarf v1.0 Planning Jun 6, 2024
@salaxander salaxander added this to the v1.0.0 milestone Jun 17, 2024
@salaxander salaxander moved this to Backlog in Zarf (old) Jun 24, 2024
@phillebaba phillebaba self-assigned this Jul 11, 2024
@salaxander salaxander added this to Zarf Jul 22, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Zarf Jul 22, 2024
@salaxander salaxander moved this from Backlog to In progress in Zarf Jul 23, 2024
@mkcp
Copy link
Contributor

mkcp commented Oct 28, 2024

Edited the list of tasks at the top to instead list out the commands that have been ported over to slog, vs the ones that haven't.

@mkcp mkcp mentioned this issue Nov 7, 2024
4 tasks
@mkcp
Copy link
Contributor

mkcp commented Nov 12, 2024

zarf connect list will be addressed by #3226

@mkcp
Copy link
Contributor

mkcp commented Nov 21, 2024

Done - follow up issues are gathered in #3210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants