-
Task
-
Resolution: Won't Do
-
Major
-
None
-
None
-
8
-
False
-
None
-
False
-
GITOPS Sprint 223, GITOPS Sprint 224, GITOPS Sprint 225, GITOPS Sprint 226
As a developer of argocd-operator, I will need to log out messages regularly. However, the current logging mechanism is somewhat aweful to use, as it does not even support usage of format strings as log messages, i.e. the following pattern is regularly observed:
log.Info(fmt.Sprintf("foo %s", bar))
This is neither good to read, nor good to write.
Also, log in this case is a global variable, private to the (huge) package github.com/argoproj-labs/argocd-operator/controller/argocd. This is not good practice at all, and the symbol name log is risking shadowing a future package import.
Proposed solutions
We should either have a wrapper package around the sigs.k8s.io/controller-runtime/pkg/log that supports format strings, i.e. something like the following mocked methods that we could use throughout our code base:
package log func Infof(format string, args ...interface{}) { // ... } func Errorf(format string, args ...interface{}) { // }
Ideally, it will also support structured fields.
As inspiration for gaining implementation ideas, you can take a look at the logging wrapper of Argo CD Image Updater at https://github.com/argoproj-labs/argocd-image-updater/blob/master/pkg/log/log.go
We should not chose github.com/sirupsen/logrus as underlying logging framework tho, because it is not actively maintained anymore.
Acceptance criteria
- The global variable log is removed from the argocd package
- A new package log is created, which contains all required wrapper methods to properly log messages
- The package provides at least the following methods:
- Info(msg string) - produce informational log message without format string
- Infof(format string, args ...interface{}) - produce informational log message with format string
- Error(err error, msg string) - produce error log message without format string, taking err into account when logging the message (similar to current behavior of controller-runtime logging package
- Errorf(format string, args ...interface{}) - produce error log message with format string
- All existing log statements that make use of fmt.Sprintf() are replaced by using the Infof() method as described above
Notes
- It's fine to use a global variable for log initialization and general parameterization that is private to the new log package, as long as it does not pollute the argocd package