chore: pass a custom logger from user for the builder factory #571
Conversation
WalkthroughThe changes involve significant modifications to the Changes
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 using PR comments)
Other keywords and placeholders
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: 2
🧹 Outside diff range and nitpick comments (2)
pkg/instance/build.go (2)
57-63: LGTM! Consider adding a comment for clarity.The refactoring to use
BuilderFactoryOptionsimproves code organization and aligns with the PR objective of passing a custom logger. This change enhances flexibility and maintainability.Consider adding a brief comment explaining the purpose of the
BuilderFactoryOptionsstruct for better code documentation:+// Use BuilderFactoryOptions to configure the builder factory with custom settings factory, err := container.NewBuilderFactory(container.BuilderFactoryOptions{ ImageName: image, BuildContext: b.getBuildDir(), ImageBuilder: b.instance.ImageBuilder, Args: args, Logger: b.instance.Logger, })
89-95: LGTM! Consider refactoring to reduce duplication.The changes in this method are consistent with those in
SetImage, improving code organization and aligning with the PR objective. This consistency is commendable.To further improve the code, consider extracting the common
BuilderFactorycreation logic into a separate method to reduce duplication:func (b *build) createBuilderFactory(imageName string, args []builder.ArgInterface) (*container.BuilderFactory, error) { return container.NewBuilderFactory(container.BuilderFactoryOptions{ ImageName: imageName, BuildContext: b.getBuildDir(), ImageBuilder: b.instance.ImageBuilder, Args: args, Logger: b.instance.Logger, }) }Then, you can use this method in both
SetImageandSetGitRepo:factory, err := b.createBuilderFactory(image, args)This refactoring would centralize the factory creation logic, making future modifications easier and reducing the risk of inconsistencies between the two methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- pkg/container/docker.go (6 hunks)
- pkg/container/errors.go (1 hunks)
- pkg/instance/build.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
pkg/container/errors.go (5)
28-28: LGTM: New error variable for empty image nameThe addition of
ErrImageNameEmptyis appropriate and follows the existing error definition pattern. It will help in providing more specific error messages when validating inputs for the builder factory.
29-29: LGTM: New error variable for empty build contextThe addition of
ErrBuildContextEmptyis appropriate and consistent with the existing error definition pattern. It will enhance error reporting when validating the build context for the builder factory.
30-30: LGTM: New error variable for empty image builderThe addition of
ErrImageBuilderEmptyis appropriate and follows the existing error definition pattern. It will provide more specific error messages when validating the image builder input for the builder factory.
31-31: LGTM: New error variable for empty loggerThe addition of
ErrLoggerEmptyis appropriate and consistent with the existing error definition pattern. This error specifically addresses the PR objective of allowing users to pass a custom logger to the builder factory, enhancing the validation process for this new feature.
28-31: Overall assessment: Excellent additions to error handlingThe four new error variables (
ErrImageNameEmpty,ErrBuildContextEmpty,ErrImageBuilderEmpty, andErrLoggerEmpty) are well-defined and consistent with the existing error handling pattern. These additions significantly improve input validation for the builder factory, aligning perfectly with the PR objectives. The changes enhance the robustness of the code without modifying existing functionality, which is a best practice in software development.pkg/instance/build.go (1)
Line range hint
1-368: Overall, the changes look good and achieve the PR objectives.The modifications to
SetImageandSetGitRepomethods successfully implement the ability to pass a custom logger to the builder factory. The use ofBuilderFactoryOptionsimproves code organization and flexibility. The changes are consistent across both methods, indicating a thoughtful approach to the refactoring.A few minor suggestions have been provided to further enhance code quality:
- Adding a brief comment to explain the purpose of
BuilderFactoryOptions.- Considering a refactoring to extract common
BuilderFactorycreation logic to reduce duplication.These suggestions are optional and do not impact the functionality or correctness of the implemented changes.
pkg/container/docker.go (1)
153-157: Same concerns as in lines 118-122 regarding logger's FormatterThe pattern here repeats the issues noted previously:
- Potential panic due to unchecked type assertion.
- Modifying the shared logger's formatter directly, which may cause unintended side effects.
Please address these concerns as suggested in the earlier comments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
pkg/container/docker.go (3)
28-33: LGTM: BuilderFactoryOptions struct introducedThe new
BuilderFactoryOptionsstruct effectively encapsulates all parameters for creating aBuilderFactory. This change improves code organization and makes it easier to extend the factory creation process in the future. The inclusion of theLoggerfield aligns with the PR objective of allowing custom loggers.Consider adding comments to each field in the struct to improve documentation:
type BuilderFactoryOptions struct { ImageName string // Name of the base image BuildContext string // Path to the build context directory ImageBuilder builder.Builder // Image builder implementation Args []builder.ArgInterface // Build arguments Logger *logrus.Logger // Custom logger instance }
37-52: LGTM: NewBuilderFactory refactored to use BuilderFactoryOptionsThe refactoring of
NewBuilderFactoryto useBuilderFactoryOptionssimplifies the function signature and improves extensibility. The addition of options verification enhances error handling, which is a good practice.Consider adding a comment to explain the purpose of the
verifyOptionsfunction:// verifyOptions checks if the provided BuilderFactoryOptions are valid if err := verifyOptions(opts); err != nil { return nil, err }The initialization of
BuilderFactoryusing the options struct is correct and includes the new logger field, which aligns with the PR objective.
207-218: LGTM: verifyOptions function addedThe addition of the
verifyOptionsfunction improves error handling by validating theBuilderFactoryOptions. The checks are comprehensive and return appropriate errors.Consider adding a check for
ImageBuilderas well, since it's used in other methods and has nil checks:if opts.ImageBuilder == nil { return ErrImageBuilderNotSet }This would provide more complete validation of the options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pkg/container/docker.go (6 hunks)
- pkg/container/errors.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/container/errors.go
🧰 Additional context used
🔇 Additional comments (7)
pkg/container/docker.go (7)
25-26: LGTM: Logger field added to BuilderFactoryThe addition of the
loggerfield to theBuilderFactorystruct aligns with the PR objective of enabling custom logging. This change enhances the factory's logging capabilities and allows for more flexible logging configurations.
90-90: LGTM: Debug log addedThe addition of this debug log improves visibility into the method's behavior when no changes are made to the image.
111-113: LGTM: Nil check for imageBuilder addedThe addition of this nil check prevents potential nil pointer dereferences and improves error handling.
147-148: LGTM: Debug log addedThe addition of this debug log improves visibility into the method's behavior when building an image from a git repository.
149-151: LGTM: Nil check for imageBuilder addedThe addition of this nil check prevents potential nil pointer dereferences and improves error handling.
202-202: LGTM: Debug log added for image hashThe addition of this debug log improves visibility by logging the generated image hash. This can be helpful for debugging and tracing purposes.
Line range hint
1-218: Overall assessment: LGTM with minor suggestionsThe changes in this file successfully implement the feature of passing a custom logger to the builder factory, aligning with the PR objective. The introduction of
BuilderFactoryOptionsand the refactoring ofNewBuilderFactoryimprove code organization and extensibility. The addition of debug logs and nil checks enhances error handling and visibility.Some minor improvements have been suggested:
- Adding comments to the
BuilderFactoryOptionsstruct fields.- Improving the handling of logger formatter modifications in
PushBuilderImageandBuildImageFromGitRepomethods.- Adding a check for
ImageBuilderin theverifyOptionsfunction.These suggestions, if implemented, would further enhance the code quality and robustness.
mojtaba-esk
deleted the
mojtaba/538-use-a-custom-logger-from-user-for-the-builder-2
branch
Closes #538
local tests are passing and the debug logs of the builder is shown when the debug level is set:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor