Make stdout logging the default and remove syslog from CI #7443
Conversation
aarongable
requested review from
beautifulentropy
and removed request for
a team
|
@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
| if _Singleton.log != nil { | ||
| err = errors.New("You may not call Set after it has already been implicitly or explicitly set") | ||
| // Even though the use of once.Do below prevents a later call from | ||
| // overriding an already-set logger, we still do this check so that we can | ||
| // return a useful error message. | ||
| err := errors.New("You may not call Set after it has already been implicitly or explicitly set") | ||
| _Singleton.log.Warning(err.Error()) | ||
| } else { | ||
| _Singleton.log = logger | ||
| return err |
There was a problem hiding this comment.
I understand that this part of the code pre-dates your changes, but I wanted to point out that the check if _Singleton.log != nil outside _Singleton.once.Do() make this unnecessarily racy. However, including it inside Do() would render it redundant because sync.Once guarantees that the initialization code block executes only once. So, perhaps we should consider removing this check altogether.
| func Set(logger Logger) (err error) { | ||
| // Set configures the singleton Logger. This method must only be called once, | ||
| // and before calling Get the first time. | ||
| func Set(logger Logger) error { |
There was a problem hiding this comment.
With so much of the functionality of this package being simplified I can't shake the feeling that both Set() and Get() being exported feels unnecessary. Ideally we should have just a couple of constructors that correctly initialize and return the logger we want. Perhaps there's something I'm missing?
|
Given that we no longer need these changes to support running pkilint in CI, I'm going to close this PR. We can tackle this again in the mid-term future, on a timetable driven by SRE instead of vice-versa. |
Make a few changes to remove all by-default dependence on syslog:
Then, uninstall rsyslog from the integration test docker container since it is no longer necessary. This clears the way to update our docker base image to a newer version which makes it harder to start the rsyslog daemon.
DO NOT MERGE before #7442