From ba97e19ef319a37f3d42c7265265a99f48def27b Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 25 Oct 2025 09:54:26 -0500 Subject: [PATCH] begin moving to slog --- cmd/appview/serve.go | 4 +++ cmd/hold/main.go | 62 +++++++++++++++++++++++----------------- pkg/appview/config.go | 6 ++++ pkg/hold/config.go | 4 +++ pkg/logging/logger.go | 58 +++++++++++++++++++++++++++++++++++++ pkg/s3/types.go | 15 ++++++---- pkg/s3/types_test.go | 18 ++++++++++++ scripts/migrate-image.sh | 4 +-- 8 files changed, 138 insertions(+), 33 deletions(-) create mode 100644 pkg/logging/logger.go diff --git a/cmd/appview/serve.go b/cmd/appview/serve.go index 94a590f..bed44f0 100644 --- a/cmd/appview/serve.go +++ b/cmd/appview/serve.go @@ -25,6 +25,7 @@ import ( "atcr.io/pkg/auth" "atcr.io/pkg/auth/oauth" "atcr.io/pkg/auth/token" + "atcr.io/pkg/logging" // UI components "atcr.io/pkg/appview" @@ -58,6 +59,9 @@ func init() { } func serveRegistry(cmd *cobra.Command, args []string) error { + // Initialize structured logging + logging.InitLogger(appview.GetLogLevel()) + // Load configuration from environment variables fmt.Println("Loading configuration from environment variables...") config, err := appview.LoadConfigFromEnv() diff --git a/cmd/hold/main.go b/cmd/hold/main.go index 4feabb2..3fbeff7 100644 --- a/cmd/hold/main.go +++ b/cmd/hold/main.go @@ -3,7 +3,7 @@ package main import ( "context" "fmt" - "log" + "log/slog" "net/http" "os" "os/signal" @@ -13,6 +13,7 @@ import ( "atcr.io/pkg/hold" "atcr.io/pkg/hold/oci" "atcr.io/pkg/hold/pds" + "atcr.io/pkg/logging" "atcr.io/pkg/s3" // Import storage drivers @@ -28,9 +29,13 @@ func main() { // Load configuration from environment variables cfg, err := hold.LoadConfigFromEnv() if err != nil { - log.Fatalf("Failed to load config: %v", err) + slog.Error("Failed to load config", "error", err) + os.Exit(1) } + // Initialize structured logging + logging.InitLogger(cfg.LogLevel) + // Initialize embedded PDS if database path is configured // This must happen before creating HoldService since service needs PDS for authorization var holdPDS *pds.HoldPDS @@ -39,25 +44,27 @@ func main() { if cfg.Database.Path != "" { // Generate did:web from public URL holdDID := pds.GenerateDIDFromURL(cfg.Server.PublicURL) - log.Printf("Initializing embedded PDS with DID: %s", holdDID) + slog.Info("Initializing embedded PDS", "did", holdDID) // Initialize PDS with carstore and keys ctx := context.Background() holdPDS, err = pds.NewHoldPDS(ctx, holdDID, cfg.Server.PublicURL, cfg.Database.Path, cfg.Database.KeyPath, cfg.Registration.EnableBlueskyPosts) if err != nil { - log.Fatalf("Failed to initialize embedded PDS: %v", err) + slog.Error("Failed to initialize embedded PDS", "error", err) + os.Exit(1) } // Create storage driver from config (needed for bootstrap profile avatar) driver, err := factory.Create(ctx, cfg.Storage.Type(), cfg.Storage.Parameters()) if err != nil { - log.Fatalf("failed to create storage driver: %v", err) - return + slog.Error("Failed to create storage driver", "error", err) + os.Exit(1) } // Bootstrap PDS with captain record, hold owner as first crew member, and profile if err := holdPDS.Bootstrap(ctx, driver, cfg.Registration.OwnerDID, cfg.Server.Public, cfg.Registration.AllowAllCrew, cfg.Registration.ProfileAvatarURL); err != nil { - log.Fatalf("Failed to bootstrap PDS: %v", err) + slog.Error("Failed to bootstrap PDS", "error", err) + os.Exit(1) } // Create event broadcaster for subscribeRepos firehose @@ -72,15 +79,16 @@ func main() { // Bootstrap events from existing repo records (one-time migration) if err := broadcaster.BootstrapFromRepo(holdPDS); err != nil { - log.Printf("Warning: Failed to bootstrap events from repo: %v", err) + slog.Warn("Failed to bootstrap events from repo", "error", err) } // Wire up repo event handler to broadcaster holdPDS.RepomgrRef().SetEventHandler(broadcaster.SetRepoEventHandler(), true) - log.Printf("Embedded PDS initialized successfully with firehose enabled") + slog.Info("Embedded PDS initialized successfully with firehose enabled") } else { - log.Fatalf("Database path is required for embedded PDS authorization") + slog.Error("Database path is required for embedded PDS authorization") + os.Exit(1) } // Create blob store adapter and XRPC handlers @@ -90,13 +98,14 @@ func main() { ctx := context.Background() driver, err := factory.Create(ctx, cfg.Storage.Type(), cfg.Storage.Parameters()) if err != nil { - log.Fatalf("failed to create storage driver: %v", err) - return + slog.Error("Failed to create storage driver", "error", err) + os.Exit(1) } s3Service, err := s3.NewS3Service(cfg.Storage.Parameters(), cfg.Server.DisablePresignedURLs, cfg.Storage.Type()) if err != nil { - log.Fatalf("Failed to create s3 service: %v", err) + slog.Error("Failed to create S3 service", "error", err) + os.Exit(1) } // Create PDS XRPC handler (ATProto endpoints) @@ -128,13 +137,13 @@ func main() { // Register XRPC/ATProto PDS endpoints if PDS is initialized if xrpcHandler != nil { - log.Printf("Registering ATProto PDS endpoints") + slog.Info("Registering ATProto PDS endpoints") xrpcHandler.RegisterHandlers(r) } // Register OCI multipart upload endpoints if ociHandler != nil { - log.Printf("Registering OCI multipart upload endpoints") + slog.Info("Registering OCI multipart upload endpoints") ociHandler.RegisterHandlers(r) } @@ -153,7 +162,7 @@ func main() { // Start server in goroutine serverErr := make(chan error, 1) go func() { - log.Printf("Starting hold service on %s", cfg.Server.Addr) + slog.Info("Starting hold service", "addr", cfg.Server.Addr) if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { serverErr <- err } @@ -164,35 +173,36 @@ func main() { ctx := context.Background() if err := holdPDS.SetStatus(ctx, "online"); err != nil { - log.Printf("Warning: Failed to set status post to online: %v", err) + slog.Warn("Failed to set status post to online", "error", err) } else { - log.Printf("Status post set to online") + slog.Info("Status post set to online") } } // Wait for signal or server error select { case err := <-serverErr: - log.Fatalf("Server failed: %v", err) + slog.Error("Server failed", "error", err) + os.Exit(1) case sig := <-sigChan: - log.Printf("Received signal %v, shutting down gracefully...", sig) + slog.Info("Received signal, shutting down gracefully", "signal", sig) // Update status post to "offline" before shutdown if holdPDS != nil { ctx := context.Background() if err := holdPDS.SetStatus(ctx, "offline"); err != nil { - log.Printf("Warning: Failed to set status post to offline: %v", err) + slog.Warn("Failed to set status post to offline", "error", err) } else { - log.Printf("Status post set to offline") + slog.Info("Status post set to offline") } } // Close broadcaster database connection if broadcaster != nil { if err := broadcaster.Close(); err != nil { - log.Printf("Warning: Failed to close broadcaster database: %v", err) + slog.Warn("Failed to close broadcaster database", "error", err) } else { - log.Printf("Broadcaster database closed") + slog.Info("Broadcaster database closed") } } @@ -201,9 +211,9 @@ func main() { defer cancel() if err := server.Shutdown(shutdownCtx); err != nil { - log.Printf("Server shutdown error: %v", err) + slog.Error("Server shutdown error", "error", err) } else { - log.Printf("Server shutdown complete") + slog.Info("Server shutdown complete") } } } diff --git a/pkg/appview/config.go b/pkg/appview/config.go index 7fb4a05..655f97b 100644 --- a/pkg/appview/config.go +++ b/pkg/appview/config.go @@ -240,6 +240,12 @@ func GetEnvOrDefault(key, defaultValue string) string { return defaultValue } +// GetLogLevel returns the configured log level from environment +// Centralizes ATCR_LOG_LEVEL env var reading +func GetLogLevel() string { + return GetEnvOrDefault("ATCR_LOG_LEVEL", "info") +} + // GetStringParam extracts a string parameter from configuration.Parameters func GetStringParam(params configuration.Parameters, key, defaultValue string) string { if v, ok := params[key]; ok { diff --git a/pkg/hold/config.go b/pkg/hold/config.go index 91bdecc..50ee770 100644 --- a/pkg/hold/config.go +++ b/pkg/hold/config.go @@ -17,6 +17,7 @@ import ( // Config represents the hold service configuration type Config struct { Version string `yaml:"version"` + LogLevel string `yaml:"log_level"` Storage StorageConfig `yaml:"storage"` Server ServerConfig `yaml:"server"` Registration RegistrationConfig `yaml:"registration"` @@ -90,6 +91,9 @@ func LoadConfigFromEnv() (*Config, error) { Version: "0.1", } + // Logging configuration + cfg.LogLevel = getEnvOrDefault("ATCR_LOG_LEVEL", "info") + // Server configuration cfg.Server.Addr = getEnvOrDefault("HOLD_SERVER_ADDR", ":8080") cfg.Server.PublicURL = os.Getenv("HOLD_PUBLIC_URL") diff --git a/pkg/logging/logger.go b/pkg/logging/logger.go new file mode 100644 index 0000000..389e5a9 --- /dev/null +++ b/pkg/logging/logger.go @@ -0,0 +1,58 @@ +// Package logging provides centralized structured logging using slog +// with configurable log levels. Call InitLogger() from main() to configure. +package logging + +import ( + "io" + "log/slog" + "os" + "strings" +) + +// InitLogger initializes the global slog default logger with the specified log level. +// Valid levels: debug, info, warn, error (case-insensitive) +// If level is empty or invalid, defaults to INFO. +// Call this from main() at startup. +func InitLogger(level string) { + var logLevel slog.Level + + switch strings.ToLower(strings.TrimSpace(level)) { + case "debug": + logLevel = slog.LevelDebug + case "info", "": + logLevel = slog.LevelInfo + case "warn", "warning": + logLevel = slog.LevelWarn + case "error": + logLevel = slog.LevelError + default: + logLevel = slog.LevelInfo + } + + opts := &slog.HandlerOptions{ + Level: logLevel, + } + + handler := slog.NewTextHandler(os.Stdout, opts) + slog.SetDefault(slog.New(handler)) +} + +// SetupTestLogger configures logging for tests to reduce noise. +// Sets log level to WARN and outputs to io.Discard to suppress DEBUG and INFO messages. +// Returns a cleanup function that should be called when the test completes (use t.Cleanup). +func SetupTestLogger() func() { + // Save original logger to restore later + originalLogger := slog.Default() + + // Set level to WARN and discard output to silence tests + opts := &slog.HandlerOptions{ + Level: slog.LevelWarn, + } + handler := slog.NewTextHandler(io.Discard, opts) + slog.SetDefault(slog.New(handler)) + + // Return cleanup function + return func() { + slog.SetDefault(originalLogger) + } +} diff --git a/pkg/s3/types.go b/pkg/s3/types.go index 9c8d89b..ad4d3bf 100644 --- a/pkg/s3/types.go +++ b/pkg/s3/types.go @@ -5,7 +5,7 @@ package s3 import ( "fmt" - "log" + "log/slog" "strings" "github.com/aws/aws-sdk-go/aws" @@ -26,14 +26,16 @@ type S3Service struct { func NewS3Service(params map[string]any, disablePresigned bool, storageType string) (*S3Service, error) { // Check if presigned URLs are explicitly disabled if disablePresigned { - log.Printf("⚠️ S3 presigned URLs DISABLED by config (DISABLE_PRESIGNED_URLS=true)") - log.Printf(" All uploads will use buffered mode (parts buffered in hold service)") + slog.Warn("S3 presigned URLs DISABLED by config", + "reason", "DISABLE_PRESIGNED_URLS=true", + "uploadMode", "buffered") return &S3Service{}, nil } // Check if storage driver is S3 if storageType != "s3" { - log.Printf("Storage driver is %s (not S3), presigned URLs disabled", storageType) + slog.Info("Presigned URLs disabled for non-S3 storage", + "storageDriver", storageType) return &S3Service{}, nil } @@ -79,7 +81,10 @@ func NewS3Service(params map[string]any, disablePresigned bool, storageType stri s3PathPrefix = strings.TrimPrefix(rootDir, "/") } - log.Printf("✅ S3 presigned URLs enabled") + slog.Info("S3 presigned URLs enabled", + "bucket", bucket, + "region", region, + "pathPrefix", s3PathPrefix) // Create S3 client return &S3Service{ diff --git a/pkg/s3/types_test.go b/pkg/s3/types_test.go index d6c0330..db827bc 100644 --- a/pkg/s3/types_test.go +++ b/pkg/s3/types_test.go @@ -2,9 +2,13 @@ package s3 import ( "testing" + + "atcr.io/pkg/logging" ) func TestNewS3Service_PresignedDisabled(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "region": "us-west-2", @@ -24,6 +28,8 @@ func TestNewS3Service_PresignedDisabled(t *testing.T) { } func TestNewS3Service_NonS3Storage(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "rootdirectory": "/tmp/test", } @@ -42,6 +48,8 @@ func TestNewS3Service_NonS3Storage(t *testing.T) { } func TestNewS3Service_MissingBucket(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "region": "us-east-1", "accesskey": "test-key", @@ -56,6 +64,8 @@ func TestNewS3Service_MissingBucket(t *testing.T) { } func TestNewS3Service_Success(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "region": "us-west-2", @@ -80,6 +90,8 @@ func TestNewS3Service_Success(t *testing.T) { } func TestNewS3Service_WithEndpoint(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "region": "us-east-1", @@ -102,6 +114,8 @@ func TestNewS3Service_WithEndpoint(t *testing.T) { } func TestNewS3Service_DefaultRegion(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "accesskey": "test-key", @@ -123,6 +137,8 @@ func TestNewS3Service_DefaultRegion(t *testing.T) { } func TestNewS3Service_WithPathPrefix(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "region": "us-east-1", @@ -142,6 +158,8 @@ func TestNewS3Service_WithPathPrefix(t *testing.T) { } func TestNewS3Service_NoCredentials(t *testing.T) { + t.Cleanup(logging.SetupTestLogger()) + params := map[string]any{ "bucket": "test-bucket", "region": "us-east-1", diff --git a/scripts/migrate-image.sh b/scripts/migrate-image.sh index 1170986..b163b22 100755 --- a/scripts/migrate-image.sh +++ b/scripts/migrate-image.sh @@ -109,8 +109,8 @@ fi echo "Found multi-arch manifest list" echo "" -# Extract platform information and digests -PLATFORMS=$(echo "$MANIFEST_JSON" | jq -r '.manifests[] | "\(.platform.os)|\(.platform.architecture)|\(.platform.variant // "")|\(.digest)"') +# Extract platform information and digests (skip unknown/unknown for cosign artifacts) +PLATFORMS=$(echo "$MANIFEST_JSON" | jq -r '.manifests[] | select(.platform.os != "unknown" and .platform.architecture != "unknown") | "\(.platform.os)|\(.platform.architecture)|\(.platform.variant // "")|\(.digest)"') # Arrays to store pushed images for manifest creation declare -a PUSHED_IMAGES