Changed logging to use zerolog #31
Conversation
zerolog logs any non-error information to stdout and any errors to stderr. Zot registry logs to stdout by default
WalkthroughThis update significantly enhances configuration management, logging, and error handling across multiple aspects of the project, including registry setup, dependency upgrades, and core functionality improvements. Key changes involve context-driven logging, refined error management, dependency updates for improved performance and security, and more flexible configuration handling via Viper. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Main as main.go
participant Config as config.toml
participant Logger as logger/logger.go
participant Registry as registry/launch-registry.go
User->>Main: Run the Go program
Main->>Config: Load configuration using Viper
Config-->>Main: Return configuration
Main->>Logger: Initialize logger with log level
Logger-->>Main: Returns logger context
Main->>Registry: Launch default registry with config path
Registry-->>Main: Registry launched status
Main->>User: Logs status and errors (if any)
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
internal/satellite/satellite.go (1)
Line range hint
25-75: LGTM! But check the error return value ofs.replicator.DeleteExtraImages.The update to include logging for error handling and context-aware operations improves error handling and aligns with context-aware operations. However, the error return value of
s.replicator.DeleteExtraImagesis not checked.- s.replicator.DeleteExtraImages(ctx, imgs) + if err := s.replicator.DeleteExtraImages(ctx, imgs); err != nil { + errLog.Error().Err(err).Msg("Error deleting extra images") + return err + }Tools
golangci-lint
44-44: Error return value of
s.replicator.DeleteExtraImagesis not checked(errcheck)
internal/store/in-memory-store.go (2)
Line range hint
43-159: Fix unchecked error and ineffectual assignment.
- The return value of
s.AddImageis not checked.- The assignment to
changeis ineffectual.- s.AddImage(ctx, img.Name) + if err := s.AddImage(ctx, img.Name); err != nil { + errLog.Error().Msgf("Failed to add image: %s, error: %v", img.Name, err) + } - change = false
Line range hint
210-243: Simplify the logic and address the TODO.The function is complex and has a TODO comment indicating it needs reworking. Simplify the logic for better readability and maintainability.
- for storeDigest, storeImage := range s.images { - if storeImage == image { - if storeDigest == digest { - tag := strings.Split(image, ":")[1] - localRegistryDigest, err := GetLocalDigest(context.Background(), tag) - if err != nil { - errLog.Error().Msgf("Error getting digest from local registry: %v", err) - return false - } - return digest == localRegistryDigest - } - s.Remove(context.Background(), storeDigest, storeImage) - s.Add(context.Background(), digest, image) - return false - } - } - err := s.Add(context.Background(), digest, image) - return err == nil + if existingImage, exists := s.images[digest]; exists { + if existingImage == image { + tag := strings.Split(image, ":")[1] + localRegistryDigest, err := GetLocalDigest(context.Background(), tag) + if err != nil { + errLog.Error().Msgf("Error getting digest from local registry: %v", err) + return false + } + return digest == localRegistryDigest + } + s.Remove(context.Background(), digest, existingImage) + } + return s.Add(context.Background(), digest, image) == nil
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (12)
- .vscode/launch.json (1 hunks)
- config.toml (1 hunks)
- go.mod (11 hunks)
- internal/replicate/replicate.go (8 hunks)
- internal/satellite/satellite.go (2 hunks)
- internal/store/file-fetch.go (3 hunks)
- internal/store/http-fetch.go (5 hunks)
- internal/store/in-memory-store.go (11 hunks)
- logger/logger.go (1 hunks)
- main.go (5 hunks)
- registry/config.json (1 hunks)
- registry/launch-registry.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .vscode/launch.json
- registry/config.json
Additional context used
golangci-lint
internal/satellite/satellite.go
44-44: Error return value of
s.replicator.DeleteExtraImagesis not checked(errcheck)
72-72: Error return value of
s.replicator.DeleteExtraImagesis not checked(errcheck)
internal/replicate/replicate.go
48-48: Error return value is not checked
(errcheck)
internal/store/in-memory-store.go
62-62: Error return value of
s.AddImageis not checked(errcheck)
156-156: ineffectual assignment to change
(ineffassign)
Additional comments not posted (43)
registry/launch-registry.go (1)
7-13: LGTM!The dynamic setting of
zotConfigPathimproves flexibility in configuration management.config.toml (5)
6-6: LGTM!The addition of
own_registry_portprovides better control over the registry configuration.
14-14: LGTM!The addition of
zotConfigPathallows for dynamic configuration path setting.
17-17: LGTM!The addition of
log_levelallows for setting the logging level dynamically.
11-11: LGTM!The update to
url_or_fileprovides a different default value for the registry URL or file path.
20-20: LGTM!The commented out configuration
url_or_fileprovides an example for testing purposes.internal/store/file-fetch.go (3)
28-28: LGTM!The update to accept a
context.Contextparameter aligns with context-aware operations and improves consistency.
32-37: LGTM!The update to accept a
context.Contextparameter and include logging for error handling improves error handling and aligns with context-aware operations.
50-64: LGTM!The update to include logging for error handling improves error handling by logging errors.
internal/satellite/satellite.go (1)
17-17: LGTM!The update to accept a
context.Contextparameter aligns with context-aware operations and improves consistency.logger/logger.go (3)
15-43: LGTM!The function correctly sets up loggers for stdout and stderr, and sets the global log level based on the provided string.
45-55: LGTM!The function correctly extracts the main logger from the context and provides a useful fallback logger with an appropriate error message.
57-67: LGTM!The function correctly extracts the error logger from the context and provides a useful fallback logger with an appropriate error message.
internal/store/http-fetch.go (4)
28-30: LGTM!The function correctly initializes a
RemoteImageListstruct with the provided URL.
34-36: LGTM!The method correctly returns the type of the image list.
Line range hint
39-94: LGTM!The function correctly fetches and returns a list of images from a remote registry, using context-based logging and appropriate error handling.
Line range hint
100-119: LGTM!The function correctly fetches the digest of an image from a remote registry, using context-based logging and appropriate error handling.
main.go (1)
Line range hint
34-182: LGTM!The
mainfunction correctly sets up configuration, logging, and runs the satellite service, using context-based logging and appropriate error handling.internal/replicate/replicate.go (5)
39-41: LGTM!The function correctly initializes a
BasicReplicator.
44-50: LGTM!The function correctly copies images from the source registry to the local registry, using context-based logging and appropriate error handling.
Tools
golangci-lint
48-48: Error return value is not checked
(errcheck)
Line range hint
61-96: LGTM!The function correctly deletes images from the local registry that are not in the provided list, using context-based logging and appropriate error handling.
Line range hint
103-125: LGTM!The function correctly constructs the source URL for pulling an image, using context-based logging and appropriate error handling.
Line range hint
156-207: LGTM!The function correctly copies an image from the source registry to the local registry, using context-based logging and appropriate error handling.
internal/store/in-memory-store.go (7)
35-38: LGTM!The code changes are approved.
165-173: LGTM!The code changes are approved.
179-182: LGTM!The code changes are approved.
187-195: LGTM!The code changes are approved.
201-204: LGTM!The code changes are approved.
Line range hint
250-265: LGTM!The code changes are approved.
Line range hint
270-278:go.mod (13)
16-21: LGTM!The dependency updates and additions are approved.
59-59: LGTM!The dependency updates and additions are approved.
125-125: LGTM!The dependency updates and additions are approved.
151-151: LGTM!The dependency updates and additions are approved.
175-175: LGTM!The dependency updates and additions are approved.
242-242: LGTM!The dependency updates and additions are approved.
258-258: LGTM!The dependency updates and additions are approved.
337-337: LGTM!The dependency updates and additions are approved.
367-367: LGTM!The dependency updates and additions are approved.
396-396: LGTM!The dependency updates and additions are approved.
401-403: LGTM!The dependency updates and additions are approved.
405-405: LGTM!The dependency updates and additions are approved.
410-412: LGTM!The dependency updates and additions are approved.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- logger/logger.go (1 hunks)
- main.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- logger/logger.go
Additional comments not posted (8)
main.go (8)
7-7: Importing logger package.The
loggerpackage is imported to facilitate structured logging withzerolog.
21-21: Importing logger and registry packages.The
loggerandregistrypackages are imported to handle logging and registry launching respectively.
27-27: Importing Viper for configuration management.The
viperpackage is imported to enhance configuration management.
56-61: Initializing loggers and adding to context.The logger is added to the context and loggers for different levels are initialized correctly.
96-106: Validating IP address for the registry.The IP validation logic is correct and error messages are logged properly.
110-122: Launching default registry and handling errors.The default registry is launched correctly and errors are handled properly.
130-141: Validating input path and logging errors.The input path validation is robust and errors are logged properly.
167-182: Loading environment variables and handling errors.The environment variables are loaded correctly and errors are logged properly.
| viper.SetConfigName("config") | ||
| viper.SetConfigType("toml") | ||
| viper.AddConfigPath(".") | ||
| if err := viper.ReadInConfig(); err != nil { | ||
| fmt.Println("Error reading config file, ", err) | ||
| fmt.Println("Exiting Satellite") | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Use structured logging for configuration errors.
Replace fmt.Println with structured logging for better error handling and consistency.
if err := viper.ReadInConfig(); err != nil {
- fmt.Println("Error reading config file, ", err)
- fmt.Println("Exiting Satellite")
+ logger.ErrorLoggerFromContext(context.Background()).Err(err).Msg("Error reading config file")
+ logger.ErrorLoggerFromContext(context.Background()).Msg("Exiting Satellite")
os.Exit(1)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| viper.SetConfigName("config") | |
| viper.SetConfigType("toml") | |
| viper.AddConfigPath(".") | |
| if err := viper.ReadInConfig(); err != nil { | |
| fmt.Println("Error reading config file, ", err) | |
| fmt.Println("Exiting Satellite") | |
| os.Exit(1) | |
| } | |
| viper.SetConfigName("config") | |
| viper.SetConfigType("toml") | |
| viper.AddConfigPath(".") | |
| if err := viper.ReadInConfig(); err != nil { | |
| logger.ErrorLoggerFromContext(context.Background()).Err(err).Msg("Error reading config file") | |
| logger.ErrorLoggerFromContext(context.Background()).Msg("Exiting Satellite") | |
| os.Exit(1) | |
| } |
Removed stdout logging Satellite now exclusively logs towards stderr with a single logger that is passed and retrieved via context Fixed some environment variables names not being set properly and restored configurations to default states
There was a problem hiding this comment.
Actionable comments posted: 22
Outside diff range and nitpick comments (5)
config.toml (2)
2-2: Typo in comment.Correct the typo in the comment from "Wether" to "Whether".
- # Wether to us the built-in Zot registry or not + # Whether to use the built-in Zot registry or not
5-6: Consider renamingown_registry_adrtoown_registry_addr.For consistency and readability, consider renaming
own_registry_adrtoown_registry_addr.- own_registry_adr = "127.0.0.1" + own_registry_addr = "127.0.0.1"logger/logger.go (1)
14-14: Typo in comment.Correct the typo in the comment from "adn" to "and".
- // AddLoggerToContext creates a new context with a zerolog logger for stdout adn stderr and sets the global log level. + // AddLoggerToContext creates a new context with a zerolog logger for stdout and stderr and sets the global log level.internal/satellite/satellite.go (1)
17-20: Consider documenting theNewSatellitefunction.Add a comment to document the
NewSatellitefunction for better clarity.+ // NewSatellite creates a new Satellite instance.internal/store/in-memory-store.go (1)
Line range hint
209-222:
Ensure logger is non-nil.Before using the logger, ensure it is non-nil to avoid potential nil pointer dereference.
log := logger.FromContext(ctx) if log == nil { return false }Tools
golangci-lint
155-155: ineffectual assignment to change
(ineffassign)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- config.toml (1 hunks)
- image-list/images.json (1 hunks)
- internal/replicate/replicate.go (6 hunks)
- internal/satellite/satellite.go (2 hunks)
- internal/store/file-fetch.go (3 hunks)
- internal/store/http-fetch.go (5 hunks)
- internal/store/in-memory-store.go (10 hunks)
- logger/logger.go (1 hunks)
- main.go (4 hunks)
- registry/config.json (1 hunks)
Files skipped from review due to trivial changes (2)
- image-list/images.json
- registry/config.json
Additional context used
golangci-lint
internal/satellite/satellite.go
43-43: Error return value of
s.replicator.DeleteExtraImagesis not checked(errcheck)
71-71: Error return value of
s.replicator.DeleteExtraImagesis not checked(errcheck)
internal/replicate/replicate.go
48-48: Error return value is not checked
(errcheck)
internal/store/in-memory-store.go
61-61: Error return value of
s.AddImageis not checked(errcheck)
155-155: ineffectual assignment to change
(ineffassign)
Additional comments not posted (10)
config.toml (1)
15-15: Ensure log level is appropriately set.The default log level is set to "info". Ensure this is the desired log level for production environments. Consider documenting acceptable log levels.
internal/store/http-fetch.go (1)
28-30: LGTM!The
RemoteImageListFetcherfunction is straightforward and correctly initializes aRemoteImageListstruct.internal/replicate/replicate.go (2)
39-41: LGTM!The
NewReplicatorfunction is straightforward and correctly initializes aBasicReplicatorstruct.
89-95: Handle potential errors fromcrane.Delete.The error return value from
crane.Deleteshould be logged and handled properly.- err := crane.Delete(fmt.Sprintf("%s:%s", localRegistry, localImage)) + if err := crane.Delete(fmt.Sprintf("%s:%s", localRegistry, localImage)); err != nil { + log.Error().Msgf("failed to delete image: %v", err) + return err + }Likely invalid or redundant comment.
main.go (1)
48-52: Use structured logging for configuration errors.Replace
fmt.Printlnwith structured logging for better error handling and consistency.- fmt.Println("Error reading config file, ", err) - fmt.Println("Exiting Satellite") + logger.ErrorLoggerFromContext(context.Background()).Err(err).Msg("Error reading config file") + logger.ErrorLoggerFromContext(context.Background()).Msg("Exiting Satellite")internal/store/in-memory-store.go (5)
35-38: LGTM!The
NewInMemoryStorefunction is straightforward and correctly initializes aninMemoryStorestruct.
164-172: LGTM!The
Addfunction is straightforward and correctly adds an image to the in-memory store.
178-181: LGTM!The
AddImagefunction is straightforward and correctly adds an image to the in-memory store.
186-194: LGTM!The
Removefunction is straightforward and correctly removes an image from the in-memory store.
200-203: LGTM!The
RemoveImagefunction is straightforward and correctly removes an image from the in-memory store.
| // Set log level to configured value | ||
| switch logLevel { | ||
| case "debug": | ||
| zerolog.SetGlobalLevel(zerolog.DebugLevel) | ||
| case "info": | ||
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | ||
| case "warn": | ||
| zerolog.SetGlobalLevel(zerolog.WarnLevel) | ||
| case "error": | ||
| zerolog.SetGlobalLevel(zerolog.ErrorLevel) | ||
| case "fatal": | ||
| zerolog.SetGlobalLevel(zerolog.FatalLevel) | ||
| case "panic": | ||
| zerolog.SetGlobalLevel(zerolog.PanicLevel) | ||
| default: | ||
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | ||
| } |
There was a problem hiding this comment.
Consider adding a default case for unsupported log levels.
Ensure that unsupported log levels are handled appropriately by logging a warning or error message.
default:
zerolog.SetGlobalLevel(zerolog.InfoLevel)
log.Warn().Msgf("Unsupported log level: %s, defaulting to info", logLevel)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set log level to configured value | |
| switch logLevel { | |
| case "debug": | |
| zerolog.SetGlobalLevel(zerolog.DebugLevel) | |
| case "info": | |
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | |
| case "warn": | |
| zerolog.SetGlobalLevel(zerolog.WarnLevel) | |
| case "error": | |
| zerolog.SetGlobalLevel(zerolog.ErrorLevel) | |
| case "fatal": | |
| zerolog.SetGlobalLevel(zerolog.FatalLevel) | |
| case "panic": | |
| zerolog.SetGlobalLevel(zerolog.PanicLevel) | |
| default: | |
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | |
| } | |
| // Set log level to configured value | |
| switch logLevel { | |
| case "debug": | |
| zerolog.SetGlobalLevel(zerolog.DebugLevel) | |
| case "info": | |
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | |
| case "warn": | |
| zerolog.SetGlobalLevel(zerolog.WarnLevel) | |
| case "error": | |
| zerolog.SetGlobalLevel(zerolog.ErrorLevel) | |
| case "fatal": | |
| zerolog.SetGlobalLevel(zerolog.FatalLevel) | |
| case "panic": | |
| zerolog.SetGlobalLevel(zerolog.PanicLevel) | |
| default: | |
| zerolog.SetGlobalLevel(zerolog.InfoLevel) | |
| log.Warn().Msgf("Unsupported log level: %s, defaulting to info", logLevel) | |
| } |
| defaultLogger := zerolog.New(os.Stderr).With().Timestamp().Logger() | ||
| defaultLogger.Error().Msg("Failed to extract logger from context") | ||
| return &defaultLogger |
There was a problem hiding this comment.
Avoid logging errors to the default logger.
Instead of logging the error to the default logger, consider returning a default logger without logging the error. This avoids potential infinite loops if the logger itself fails.
- defaultLogger.Error().Msg("Failed to extract logger from context")
+ // Log error to a different logger or handle it appropriatelyCommittable suggestion was skipped due to low confidence.
| return nil, err | ||
| } | ||
|
|
||
| var imageData ImageData | ||
| // Parse the JSON data | ||
| err = json.Unmarshal(data, &imageData) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error unmarshalling JSON data") |
There was a problem hiding this comment.
Improve error message for unmarshalling JSON.
Include the file path in the error message to provide more context.
- log.Error().Err(err).Msg("Error unmarshalling JSON data")
+ log.Error().Err(err).Str("path", client.Path).Msg("Error unmarshalling JSON data")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Error().Err(err).Msg("Error unmarshalling JSON data") | |
| log.Error().Err(err).Str("path", client.Path).Msg("Error unmarshalling JSON data") |
| var images []Image | ||
|
|
||
| // Read the file | ||
| data, err := os.ReadFile(client.Path) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error reading file") |
There was a problem hiding this comment.
Improve error message for reading file.
Include the file path in the error message to provide more context.
- log.Error().Err(err).Msg("Error reading file")
+ log.Error().Err(err).Str("path", client.Path).Msg("Error reading file")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Error().Err(err).Msg("Error reading file") | |
| log.Error().Err(err).Str("path", client.Path).Msg("Error reading file") |
| func FileImageListFetcher(ctx context.Context, relativePath string) *FileImageList { | ||
| log := logger.FromContext(ctx) | ||
| // Get the current working directory | ||
| dir, err := os.Getwd() | ||
| if err != nil { | ||
| fmt.Println("Error getting current directory:", err) | ||
| log.Error().Err(err).Msg("Error getting current directory") |
There was a problem hiding this comment.
Return nil, err instead of nil.
To maintain consistency and clarity, return nil, err when an error occurs.
- return nil
+ return nil, errCommittable suggestion was skipped due to low confidence.
| log.Error().Err(err).Msg("Error loading.env file") | ||
| return err |
There was a problem hiding this comment.
Handle potential errors from godotenv.Load.
The error return value from godotenv.Load should be logged and handled properly.
if err := godotenv.Load(); err != nil {
log.Error().Err(err).Msg("Error loading .env file")
return err
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Error().Err(err).Msg("Error loading.env file") | |
| return err | |
| if err := godotenv.Load(); err != nil { | |
| log.Error().Err(err).Msg("Error loading .env file") | |
| return err | |
| } |
| ip := net.ParseIP(registryAdr) | ||
| if ip == nil { | ||
| log.Error().Msg("Invalid IP address") | ||
| return errors.New("invalid IP address") | ||
| } | ||
| if ip.To4() != nil { | ||
| log.Info().Msg("IP address is valid IPv4") | ||
| } else { | ||
| log.Error().Msg("IP address is IPv6 format and unsupported") | ||
| return errors.New("IP address is IPv6 format and unsupported") | ||
| } |
There was a problem hiding this comment.
Improve IP address validation.
Consider using net.IP's To4 and To16 methods for more accurate IP address validation.
ip := net.ParseIP(registryAdr)
if ip == nil {
log.Error().Msg("Invalid IP address")
return errors.New("invalid IP address")
}
if ip.To4() != nil {
log.Info().Msg("IP address is valid IPv4")
} else if ip.To16() != nil {
log.Info().Msg("IP address is valid IPv6")
} else {
log.Error().Msg("Invalid IP address format")
return errors.New("invalid IP address format")
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ip := net.ParseIP(registryAdr) | |
| if ip == nil { | |
| log.Error().Msg("Invalid IP address") | |
| return errors.New("invalid IP address") | |
| } | |
| if ip.To4() != nil { | |
| log.Info().Msg("IP address is valid IPv4") | |
| } else { | |
| log.Error().Msg("IP address is IPv6 format and unsupported") | |
| return errors.New("IP address is IPv6 format and unsupported") | |
| } | |
| ip := net.ParseIP(registryAdr) | |
| if ip == nil { | |
| log.Error().Msg("Invalid IP address") | |
| return errors.New("invalid IP address") | |
| } | |
| if ip.To4() != nil { | |
| log.Info().Msg("IP address is valid IPv4") | |
| } else if ip.To16() != nil { | |
| log.Info().Msg("IP address is valid IPv6") | |
| } else { | |
| log.Error().Msg("Invalid IP address format") | |
| return errors.New("invalid IP address format") | |
| } |
| log := logger.FromContext(ctx) | ||
|
|
There was a problem hiding this comment.
Ensure logger is non-nil.
Before using the logger, ensure it is non-nil to avoid potential nil pointer dereference.
log := logger.FromContext(ctx)
if log == nil {
return "", fmt.Errorf("logger not found in context")
}| @@ -58,7 +61,7 @@ func (s *inMemoryStore) List(ctx context.Context) ([]Image, error) { | |||
| s.AddImage(ctx, img.Name) | |||
There was a problem hiding this comment.
Check the error return value.
The error return value from s.AddImage is not checked, which could lead to silent failures.
- s.AddImage(ctx, img.Name)
+ if err := s.AddImage(ctx, img.Name); err != nil {
+ log.Error().Msgf("failed to add image: %v", err)
+ return nil, err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.AddImage(ctx, img.Name) | |
| if err := s.AddImage(ctx, img.Name); err != nil { | |
| log.Error().Msgf("failed to add image: %v", err) | |
| return nil, err | |
| } |
Tools
golangci-lint
61-61: Error return value of
s.AddImageis not checked(errcheck)
| @@ -148,58 +151,64 @@ func (s *inMemoryStore) List(ctx context.Context) ([]Image, error) { | |||
|
|
|||
| } | |||
| if change { | |||
| fmt.Println("Changes detected in the store") | |||
| log.Info().Msg("Changes detected in the store") | |||
| change = false | |||
There was a problem hiding this comment.
Ineffectual assignment to change.
The variable change is assigned but not used.
if change {
log.Info().Msg("Changes detected in the store")
change = false
return imageList, nil
} else {
log.Info().Msg("No changes detected in the store")
return nil, nil
}Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
155-155: ineffectual assignment to change
(ineffassign)
|
@OneFlyingBanana, can we merge those changes? |
|
@Vad1mo solved the conflicts, should be good to merge now |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- go.mod (12 hunks)
Additional comments not posted (10)
go.mod (10)
18-18: Approved: Addedzerologdependency.The addition of
zerologaligns with the PR objective to change the logging mechanism.
20-20: Approved: Removedzapdependency.The removal of
zapaligns with the PR objective to replace it withzerolog.
60-60: Approved: Updatedhcsshimdependency.The update from
v0.12.3tov0.12.4likely includes bug fixes or improvements.
126-126: Approved: Updatedcontainerddependency.The update from
v1.7.17tov1.7.18likely includes bug fixes or improvements.
152-152: Approved: Updateddockerdependency.The update from
v26.1.3+incompatibletov27.0.0+incompatiblelikely includes bug fixes or improvements.
176-176: Approved: Updatedlogrdependency.The update from
v1.4.1tov1.4.2likely includes bug fixes or improvements.
244-244: Approved: Updatedcompressdependency.The update from
v1.17.8tov1.17.9likely includes bug fixes or improvements.
413-413: Approved: Updatedgenproto/googleapis/rpcdependency.The update from
v0.0.0-20240515191416-fc5f0ca64291tov0.0.0-20240617180043-68d350f18fd4likely includes bug fixes or improvements.
415-415: Approved: Updatedprotobufdependency.The update from
v1.34.1tov1.34.2likely includes bug fixes or improvements.
260-260: Approved: Addedplan9statsandgo-sysconfdependencies.The addition of these dependencies likely supports new features or improvements.
Also applies to: 368-368
zerolog logs any non-error information to stdout and any errors to stderr. Zot registry logs to stdout by default
Summary by CodeRabbit
New Features
Configurations
Dependency Updates
Improvements