Uploaded image for project: 'OpenShift GitOps'
  1. OpenShift GitOps
  2. GITOPS-1911

Provide better logging methods

XMLWordPrintable

    • Icon: Task Task
    • Resolution: Unresolved
    • Icon: Major Major
    • None
    • None
    • Operator
    • 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
      •  

            Unassigned Unassigned
            jfischer@redhat.com Jann Fischer
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: