Fix: imgutil/local: Add a warning message when saving a local image and containerd storage is enable #284 #285
Conversation
Signed-off-by: wuhenggongzi <[email protected]>
This commit enhances the Logger functionality by integrating it with the existing automatic detection of the storage driver type in the NewStore function. The Logger interface has been added to log warnings related to the containerd storage driver. Test cases have been created to ensure that warnings are logged correctly when using containerd. Signed-off-by: wuhenggongzi <[email protected]>
The message parameter was not handled as specified before, but now the message parameter is printed. Signed-off-by: wuhenggongzi <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
===========================================
- Coverage 58.29% 39.52% -18.76%
===========================================
Files 32 35 +3
Lines 3694 3277 -417
===========================================
- Hits 2153 1295 -858
- Misses 1184 1787 +603
+ Partials 357 195 -162 |
There was a problem hiding this comment.
@wuhenggongzi Thanks a lot for this work! once I saw it in code I had some thoughts about it, but maybe @natalieparellano or @jabrown85 can take another look.
Wait a little to get more feedback before doing any change!
| @@ -17,6 +17,7 @@ type ImageOptions struct { | |||
| BaseImageRepoName string | |||
| PreviousImageRepoName string | |||
| Config *v1.Config | |||
| Logger Logger | |||
There was a problem hiding this comment.
Now that I see the option carefully, I think we shouldn't expose the Logger as an ImageOption, because it is not an Image attribute.
I will remove it from here and move it to local.NewImage constructor.
| @@ -43,6 +44,10 @@ type RegistrySetting struct { | |||
| Insecure bool | |||
| } | |||
|
|
|||
| type Logger interface { | |||
There was a problem hiding this comment.
Should we move the Logger interface to local or maybe to the root folder?
| @@ -47,7 +47,11 @@ func NewImage(repoName string, dockerClient DockerClient, ops ...imgutil.ImageOp | |||
| baseIdentifier = baseImage.identifier | |||
| store = baseImage.layerStore | |||
| } else { | |||
There was a problem hiding this comment.
Base on my comment on the ImageOption, maybe we should change the constructor to:
func NewImage(repoName string, dockerClient DockerClient, logger Logger, ops ...imgutil.ImageOption) (*Image, error) {
// create the Store with the Logger
}Also in the NewStore constructor we can receive the Logger instance.
I know this is going to break, imgutil library consumers, but the Logger inside the ImageOption doesn't make sense for me, at least conceptually
| @@ -43,6 +44,10 @@ type RegistrySetting struct { | |||
| Insecure bool | |||
| } | |||
|
|
|||
| type Logger interface { | |||
| Warn(msg string) | |||
There was a problem hiding this comment.
@natalieparellano should we add a Warnf(fmt string, v ...interface{}) method too?
|
I'm a little wary that we're making this more complicated than it needs to be - I was able to add the warning with just a few lines of code here: https://github.com/buildpacks/pack/pull/2284/files I guess, the downside is that when we fix this in imgutil (presumably when #242 (comment) is available via upstream docker) then we'll have to remember to remove the warning. But this could perhaps be made easier by creating various issues to capture the work mentioned in buildpacks/pack#2272 (comment) and then adding a note to the relevant issue in imgutil that we should remove the warning when fixed imgutil is part of pack. |
Fixse (imgutil) #282
This pr enhances the Logger functionality by integrating it with the existing
automatic detection of the storage driver type in the NewStore function.
The Logger interface has been added to log warnings related to the containerd storage driver.
Test cases have been created to ensure that warnings are logged correctly when using containerd.