From 1a96467fbaf028283450c4ca313bef4a496a4695 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Thu, 28 Jul 2022 12:23:44 -0400 Subject: [PATCH] fix: Windows updater log fix (#595) * Added os specific log path Signed-off-by: Corbin Phelps * make tests run on windows * make fmt, fix function redefinitions * reduce diff * add license Co-authored-by: Corbin Phelps --- .../logging/{logging.go => logging_others.go} | 2 + updater/internal/logging/logging_test.go | 31 ++++----- updater/internal/logging/logging_windows.go | 68 +++++++++++++++++++ 3 files changed, 84 insertions(+), 17 deletions(-) rename updater/internal/logging/{logging.go => logging_others.go} (98%) create mode 100644 updater/internal/logging/logging_windows.go diff --git a/updater/internal/logging/logging.go b/updater/internal/logging/logging_others.go similarity index 98% rename from updater/internal/logging/logging.go rename to updater/internal/logging/logging_others.go index 1dbc74a9b..82140b95f 100644 --- a/updater/internal/logging/logging.go +++ b/updater/internal/logging/logging_others.go @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +//go:build !windows + package logging import ( diff --git a/updater/internal/logging/logging_test.go b/updater/internal/logging/logging_test.go index 8911da880..119488f8e 100644 --- a/updater/internal/logging/logging_test.go +++ b/updater/internal/logging/logging_test.go @@ -18,7 +18,6 @@ import ( "bytes" "os" "path/filepath" - "runtime" "testing" "github.com/stretchr/testify/require" @@ -26,16 +25,16 @@ import ( func TestNewLogger(t *testing.T) { t.Run("Existing file is removed", func(t *testing.T) { - if runtime.GOOS == "windows" { - // Skip on windows, because the log file will still be open - // when the test attempts to remove the temp dir, which ends up making - // the test fail. - t.SkipNow() - } - tmpDir := t.TempDir() + // We don't use t.TempDir here, because we can't clean up the out directory on windows. + // We also don't clean up the out directory; It's in the temporary directory and may be cleaned up manually at any time. + tmpDir, err := os.MkdirTemp("", "test-logger-existing-file") + require.NoError(t, err) + // Remove previous log directory if it exists + require.NoError(t, os.RemoveAll(filepath.Join(tmpDir, "log"))) require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "log"), 0775)) - updaterLogPath := filepath.Join(tmpDir, "log", "updater.log") + updaterLogPath, err := filepath.Abs(filepath.Join(tmpDir, "log", "updater.log")) + require.NoError(t, err) initialBytes := []byte("Some existing bytes") require.NoError(t, os.WriteFile(updaterLogPath, initialBytes, 0660)) @@ -57,16 +56,14 @@ func TestNewLogger(t *testing.T) { }) t.Run("Logger creates file if existing file does not exist", func(t *testing.T) { - if runtime.GOOS == "windows" { - // Skip on windows, because the log file will still be open - // when the test attempts to remove the temp dir, which ends up making - // the test fail. - t.SkipNow() - } - tmpDir := t.TempDir() + tmpDir, err := os.MkdirTemp("", "test-logger-no-existing-file") + require.NoError(t, err) + // Remove previous log directory if it exists + require.NoError(t, os.RemoveAll(filepath.Join(tmpDir, "log"))) require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "log"), 0775)) - updaterLogPath := filepath.Join(tmpDir, "log", "updater.log") + updaterLogPath, err := filepath.Abs(filepath.Join(tmpDir, "log", "updater.log")) + require.NoError(t, err) logger, err := NewLogger(tmpDir) require.NoError(t, err) diff --git a/updater/internal/logging/logging_windows.go b/updater/internal/logging/logging_windows.go new file mode 100644 index 000000000..a1f3b106b --- /dev/null +++ b/updater/internal/logging/logging_windows.go @@ -0,0 +1,68 @@ +// Copyright observIQ, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package logging + +import ( + "fmt" + "net/url" + "os" + "sync" + + "github.com/observiq/observiq-otel-collector/updater/internal/path" + "go.uber.org/zap" +) + +var registerSinkOnce = &sync.Once{} + +// NewLogger returns a new logger, that logs to the log directory relative to installDir. +// It deletes the previous log file, as well. +// NewLogger must only be called once, at the start of the program. +func NewLogger(installDir string) (*zap.Logger, error) { + // On windows, absolute paths do not work for zap's default sink, so we must register it. + // see: https://github.com/uber-go/zap/issues/621 + var err error + registerSinkOnce.Do(func() { + err = zap.RegisterSink("winfile", newWinFileSink) + }) + if err != nil { + return nil, fmt.Errorf("failed to registed windows file sink: %w", err) + } + + logFile := path.LogFile(installDir) + + err = os.RemoveAll(logFile) + if err != nil { + return nil, fmt.Errorf("failed to remove previous log file: %w", err) + } + + conf := zap.NewProductionConfig() + conf.OutputPaths = []string{ + "winfile:///" + logFile, + } + + prodLogger, err := conf.Build() + if err != nil { + return nil, fmt.Errorf("failed to build logger: %w", err) + } + + return prodLogger, nil +} + +// Windows requires a special sink, so that we may properly parse the file path +// See: https://github.com/uber-go/zap/issues/621 +func newWinFileSink(u *url.URL) (zap.Sink, error) { + // Remove leading slash left by url.Parse() + return os.OpenFile(u.Path[1:], os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0600) +}