From 0f4ba3431199714a4c53805f05ae9fe84d3a4498 Mon Sep 17 00:00:00 2001 From: Corbin Phelps Date: Thu, 28 Jul 2022 10:23:47 -0400 Subject: [PATCH 1/5] Added os specific log path Signed-off-by: Corbin Phelps --- updater/internal/path/path.go | 5 ---- updater/internal/path/path_darwin.go | 11 ++++++++- updater/internal/path/path_linux.go | 11 ++++++++- updater/internal/path/path_others_test.go | 28 ++++++++++++++++++++++ updater/internal/path/path_test.go | 4 ---- updater/internal/path/path_windows.go | 6 +++++ updater/internal/path/path_windows_test.go | 4 ++++ 7 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 updater/internal/path/path_others_test.go diff --git a/updater/internal/path/path.go b/updater/internal/path/path.go index 509a76040..e5ceb6f52 100644 --- a/updater/internal/path/path.go +++ b/updater/internal/path/path.go @@ -40,8 +40,3 @@ func ServiceFileDir(installDir string) string { func BackupServiceFile(installDir string) string { return filepath.Join(BackupDir(installDir), "backup.service") } - -// LogFile returns the full path to the log file for the updater -func LogFile(installDir string) string { - return filepath.Join(installDir, "log", "updater.log") -} diff --git a/updater/internal/path/path_darwin.go b/updater/internal/path/path_darwin.go index 75d5631e6..8fda79d94 100644 --- a/updater/internal/path/path_darwin.go +++ b/updater/internal/path/path_darwin.go @@ -14,7 +14,11 @@ package path -import "go.uber.org/zap" +import ( + "path/filepath" + + "go.uber.org/zap" +) // DarwinInstallDir is the path to the install directory on Darwin. const DarwinInstallDir = "/opt/observiq-otel-collector" @@ -23,3 +27,8 @@ const DarwinInstallDir = "/opt/observiq-otel-collector" func InstallDir(_ *zap.Logger) (string, error) { return DarwinInstallDir, nil } + +// LogFile returns the full path to the log file for the updater +func LogFile(installDir string) string { + return filepath.Join(installDir, "log", "updater.log") +} diff --git a/updater/internal/path/path_linux.go b/updater/internal/path/path_linux.go index 41377c621..6fdb42fdb 100644 --- a/updater/internal/path/path_linux.go +++ b/updater/internal/path/path_linux.go @@ -14,7 +14,11 @@ package path -import "go.uber.org/zap" +import ( + "path/filepath" + + "go.uber.org/zap" +) // LinuxInstallDir is the install directory of the collector on linux. const LinuxInstallDir = "/opt/observiq-otel-collector" @@ -23,3 +27,8 @@ const LinuxInstallDir = "/opt/observiq-otel-collector" func InstallDir(_ *zap.Logger) (string, error) { return LinuxInstallDir, nil } + +// LogFile returns the full path to the log file for the updater +func LogFile(installDir string) string { + return filepath.Join(installDir, "log", "updater.log") +} diff --git a/updater/internal/path/path_others_test.go b/updater/internal/path/path_others_test.go new file mode 100644 index 000000000..ebd191840 --- /dev/null +++ b/updater/internal/path/path_others_test.go @@ -0,0 +1,28 @@ +// 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. + +//go:build !windows + +package path + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLogFile(t *testing.T) { + require.Equal(t, filepath.Join("install", "log", "updater.log"), LogFile("install")) +} diff --git a/updater/internal/path/path_test.go b/updater/internal/path/path_test.go index 538c11e97..31e80ed89 100644 --- a/updater/internal/path/path_test.go +++ b/updater/internal/path/path_test.go @@ -40,7 +40,3 @@ func TestServiceFileDir(t *testing.T) { func TestBackupServiceFile(t *testing.T) { require.Equal(t, filepath.Join("install", "tmp", "rollback", "backup.service"), BackupServiceFile("install")) } - -func TestLogFile(t *testing.T) { - require.Equal(t, filepath.Join("install", "log", "updater.log"), LogFile("install")) -} diff --git a/updater/internal/path/path_windows.go b/updater/internal/path/path_windows.go index f0787def1..8d6590b76 100644 --- a/updater/internal/path/path_windows.go +++ b/updater/internal/path/path_windows.go @@ -16,6 +16,7 @@ package path import ( "fmt" + "path/filepath" "go.uber.org/zap" "golang.org/x/sys/windows/registry" @@ -51,3 +52,8 @@ func installDirFromRegistry(logger *zap.Logger, productName string) (string, err func InstallDir(logger *zap.Logger) (string, error) { return installDirFromRegistry(logger, defaultProductName) } + +// LogFile returns the full path to the log file for the updater +func LogFile(installDir string) string { + return filepath.Join("winfile://", installDir, "log", "updater.log") +} diff --git a/updater/internal/path/path_windows_test.go b/updater/internal/path/path_windows_test.go index 330fab3dd..a7789b8e9 100644 --- a/updater/internal/path/path_windows_test.go +++ b/updater/internal/path/path_windows_test.go @@ -78,3 +78,7 @@ func createInstallDirRegistryKey(t *testing.T, productName, installDir string) { err = key.SetStringValue("InstallLocation", installDir) require.NoError(t, err) } + +func TestLogFile(t *testing.T) { + require.Equal(t, filepath.Join("winfile:///", "install", "log", "updater.log"), LogFile("install")) +} From 425a5ab5a2e46a02f918fa3240b41e4ebfa5b703 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Thu, 28 Jul 2022 11:53:32 -0400 Subject: [PATCH 2/5] make tests run on windows --- .../logging/{logging.go => logging_others.go} | 2 + updater/internal/logging/logging_test.go | 31 +++++------ updater/internal/logging/logging_windows.go | 54 +++++++++++++++++++ updater/internal/path/path.go | 5 ++ updater/internal/path/path_others_test.go | 28 ---------- updater/internal/path/path_test.go | 4 ++ updater/internal/path/path_windows.go | 6 --- updater/internal/path/path_windows_test.go | 4 -- 8 files changed, 79 insertions(+), 55 deletions(-) rename updater/internal/logging/{logging.go => logging_others.go} (98%) create mode 100644 updater/internal/logging/logging_windows.go delete mode 100644 updater/internal/path/path_others_test.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..841ada6b7 --- /dev/null +++ b/updater/internal/logging/logging_windows.go @@ -0,0 +1,54 @@ +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) +} diff --git a/updater/internal/path/path.go b/updater/internal/path/path.go index e5ceb6f52..509a76040 100644 --- a/updater/internal/path/path.go +++ b/updater/internal/path/path.go @@ -40,3 +40,8 @@ func ServiceFileDir(installDir string) string { func BackupServiceFile(installDir string) string { return filepath.Join(BackupDir(installDir), "backup.service") } + +// LogFile returns the full path to the log file for the updater +func LogFile(installDir string) string { + return filepath.Join(installDir, "log", "updater.log") +} diff --git a/updater/internal/path/path_others_test.go b/updater/internal/path/path_others_test.go deleted file mode 100644 index ebd191840..000000000 --- a/updater/internal/path/path_others_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// 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. - -//go:build !windows - -package path - -import ( - "path/filepath" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestLogFile(t *testing.T) { - require.Equal(t, filepath.Join("install", "log", "updater.log"), LogFile("install")) -} diff --git a/updater/internal/path/path_test.go b/updater/internal/path/path_test.go index 31e80ed89..538c11e97 100644 --- a/updater/internal/path/path_test.go +++ b/updater/internal/path/path_test.go @@ -40,3 +40,7 @@ func TestServiceFileDir(t *testing.T) { func TestBackupServiceFile(t *testing.T) { require.Equal(t, filepath.Join("install", "tmp", "rollback", "backup.service"), BackupServiceFile("install")) } + +func TestLogFile(t *testing.T) { + require.Equal(t, filepath.Join("install", "log", "updater.log"), LogFile("install")) +} diff --git a/updater/internal/path/path_windows.go b/updater/internal/path/path_windows.go index 8d6590b76..f0787def1 100644 --- a/updater/internal/path/path_windows.go +++ b/updater/internal/path/path_windows.go @@ -16,7 +16,6 @@ package path import ( "fmt" - "path/filepath" "go.uber.org/zap" "golang.org/x/sys/windows/registry" @@ -52,8 +51,3 @@ func installDirFromRegistry(logger *zap.Logger, productName string) (string, err func InstallDir(logger *zap.Logger) (string, error) { return installDirFromRegistry(logger, defaultProductName) } - -// LogFile returns the full path to the log file for the updater -func LogFile(installDir string) string { - return filepath.Join("winfile://", installDir, "log", "updater.log") -} diff --git a/updater/internal/path/path_windows_test.go b/updater/internal/path/path_windows_test.go index a7789b8e9..330fab3dd 100644 --- a/updater/internal/path/path_windows_test.go +++ b/updater/internal/path/path_windows_test.go @@ -78,7 +78,3 @@ func createInstallDirRegistryKey(t *testing.T, productName, installDir string) { err = key.SetStringValue("InstallLocation", installDir) require.NoError(t, err) } - -func TestLogFile(t *testing.T) { - require.Equal(t, filepath.Join("winfile:///", "install", "log", "updater.log"), LogFile("install")) -} From 631ca99413b7a08cba037807d0ded2429071fe93 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Thu, 28 Jul 2022 11:59:31 -0400 Subject: [PATCH 3/5] make fmt, fix function redefinitions --- updater/internal/logging/logging_windows.go | 108 ++++++++++---------- updater/internal/path/path_darwin.go | 7 -- updater/internal/path/path_linux.go | 11 +- 3 files changed, 55 insertions(+), 71 deletions(-) diff --git a/updater/internal/logging/logging_windows.go b/updater/internal/logging/logging_windows.go index 841ada6b7..cbaef9d3f 100644 --- a/updater/internal/logging/logging_windows.go +++ b/updater/internal/logging/logging_windows.go @@ -1,54 +1,54 @@ -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) -} +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) +} diff --git a/updater/internal/path/path_darwin.go b/updater/internal/path/path_darwin.go index 8fda79d94..2f7aa5b6f 100644 --- a/updater/internal/path/path_darwin.go +++ b/updater/internal/path/path_darwin.go @@ -15,8 +15,6 @@ package path import ( - "path/filepath" - "go.uber.org/zap" ) @@ -27,8 +25,3 @@ const DarwinInstallDir = "/opt/observiq-otel-collector" func InstallDir(_ *zap.Logger) (string, error) { return DarwinInstallDir, nil } - -// LogFile returns the full path to the log file for the updater -func LogFile(installDir string) string { - return filepath.Join(installDir, "log", "updater.log") -} diff --git a/updater/internal/path/path_linux.go b/updater/internal/path/path_linux.go index 6fdb42fdb..41377c621 100644 --- a/updater/internal/path/path_linux.go +++ b/updater/internal/path/path_linux.go @@ -14,11 +14,7 @@ package path -import ( - "path/filepath" - - "go.uber.org/zap" -) +import "go.uber.org/zap" // LinuxInstallDir is the install directory of the collector on linux. const LinuxInstallDir = "/opt/observiq-otel-collector" @@ -27,8 +23,3 @@ const LinuxInstallDir = "/opt/observiq-otel-collector" func InstallDir(_ *zap.Logger) (string, error) { return LinuxInstallDir, nil } - -// LogFile returns the full path to the log file for the updater -func LogFile(installDir string) string { - return filepath.Join(installDir, "log", "updater.log") -} From cb1af452d6264dde5307f9b9f55332d5eb3e64eb Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Thu, 28 Jul 2022 12:00:29 -0400 Subject: [PATCH 4/5] reduce diff --- updater/internal/path/path_darwin.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/updater/internal/path/path_darwin.go b/updater/internal/path/path_darwin.go index 2f7aa5b6f..75d5631e6 100644 --- a/updater/internal/path/path_darwin.go +++ b/updater/internal/path/path_darwin.go @@ -14,9 +14,7 @@ package path -import ( - "go.uber.org/zap" -) +import "go.uber.org/zap" // DarwinInstallDir is the path to the install directory on Darwin. const DarwinInstallDir = "/opt/observiq-otel-collector" From 1cbfe2a93b3527ec52299b4d0c9e24091a5b5da9 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Thu, 28 Jul 2022 12:06:56 -0400 Subject: [PATCH 5/5] add license --- updater/internal/logging/logging_windows.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/updater/internal/logging/logging_windows.go b/updater/internal/logging/logging_windows.go index cbaef9d3f..a1f3b106b 100644 --- a/updater/internal/logging/logging_windows.go +++ b/updater/internal/logging/logging_windows.go @@ -1,3 +1,17 @@ +// 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 (