Remove Docker dependency for retrieving image filesystem #97
Conversation
cmd/diff.go
Outdated
| } | ||
| return nil | ||
| } | ||
|
|
||
| func diffImages(image1Arg, image2Arg string, diffArgs []string) error { | ||
| diffTypes, err := differs.GetAnalyzers(diffArgs) | ||
| if err != nil { | ||
| glog.Error(err.Error()) | ||
| return errors.New("Could not perform image diff") | ||
| return fmt.Errorf("err msg: %s", err.Error()) |
There was a problem hiding this comment.
just return err or make this more informative?
There was a problem hiding this comment.
fixed up the errors in this file
cmd/diff.go
Outdated
| @@ -116,6 +116,14 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func processImageName(imageName string) string { | |||
| if pkgutil.IsTar(imageName) || strings.HasPrefix(imageName, "daemon://") { | |||
There was a problem hiding this comment.
What if it already has remote://? would this add it a second time?
There was a problem hiding this comment.
I wasn't really planning on advertising that prefix to users, but I guess there's nothing stopping them from adding it in which case this would break. fixed
pkg/util/image_prepper.go
Outdated
| glog.Infof("Starting prep for image %s", p.Source) | ||
| img := p.Source | ||
|
|
||
| var prepper Prepper | ||
|
|
||
| // first, respect prefixes on image names | ||
| if strings.HasPrefix(p.Source, "daemon://") { |
There was a problem hiding this comment.
It seems like this should be redundant with the loop below.
There was a problem hiding this comment.
this was stripping off the prefix of the image source before creating the prepper, but since the rest of the logic was essentially redundant I moved this into the SupportsImage() method.
pkg/util/cloud_prepper.go
Outdated
| @@ -47,7 +47,8 @@ func (p CloudPrepper) SupportsImage() bool { | |||
| } | |||
| strippedSource := strings.Replace(p.ImagePrepper.Source, RemotePrefix, "", -1) | |||
| _, err := reference.Parse(p.ImagePrepper.Source) | |||
| if err != nil { | |||
| if err == nil { | |||
There was a problem hiding this comment.
i would reverse these conditionals
if err != nil {
// log the error or inspect it
return false
}
p.ImagePrepper.Source = strippedSource
return trueI also don't like that calling a validation function actually mutates my p.ImagePrepper.Source. Is there a better pattern for prefixing these correctly? Seems very prone to break (repeated calls, calling this with the with/without a prefix, etc.)
There was a problem hiding this comment.
Thinking a little more, I think the real problem here is that ImagePrepper shouldn't have Source be a string but it should actually be a containers/image types.ImageReference, which is exactly what you can pass into all of the containers/images functions without have to worry about parsing it at the end.
https://github.com/containers/image/blob/master/types/types.go#L37-L90
There was a problem hiding this comment.
There are a few problems here, the biggest of which is that the ImagePrepper is supposed to be an abstraction inside of the Prepper interface, and not all implementations have a types.ImageReference (e.g. the TarPrepper) doesn't use this. So there isn't a 1:1 conversion of the source to the ImageReference.
I do agree that mutating the source in the validation function isn't a great pattern though, I'm working on fixing this. We should probably do all source parsing in the client code, and keep everything constant once we get down into the library code that actually does the processing.
…epper in validation
…per directly, no inference
| @@ -35,10 +35,10 @@ var diffCmd = &cobra.Command{ | |||
| Long: `Compares two images using the specifed analyzers as indicated via flags (see documentation for available ones).`, | |||
There was a problem hiding this comment.
We should probably document the available image name flags somewhere, does it make sense here?
There was a problem hiding this comment.
yeah I should put it here. should also go in the README somewhere
| } | ||
| return true | ||
| func (p CloudPrepper) GetImage() (Image, error) { | ||
| return getImage(p) |
There was a problem hiding this comment.
is getImage used anywhere else? Should you just inline it here instead of breaking it out?
There was a problem hiding this comment.
yeah it's actually the same across all the preppers (currently), but it made more sense to me to just have them all use a helper method and expose the GetImage() publicly on each of the preppers. WDYT?
|
Ok, third time's a charm. Closing this to split the refactoring, docker client change, and cleanup into separate PRs for readability. |
Also, add support for 'daemon' prefix to specify image in local docker daemon.