iDiff refactor with interfaces #243
Conversation
…ease output additions
…imes-common into DiffRefactorOutput
iDiff/differs/differs.go
Outdated
| "linux": AptDiff, | ||
| "pip": PipDiff, | ||
| "node": NodeDiff, | ||
| type ImageDiff struct { |
There was a problem hiding this comment.
nit: This struct name is a little confusing to me just because ImageDiff makes me think the diff has been done but it hasn't yet... not super important just something to consider
iDiff/utils/docker_utils.go
Outdated
|
|
||
| strhistory := make([]string, len(history)) | ||
| for i, layer := range history { | ||
| layerDescription := strings.TrimSpace(layer.CreatedBy) |
There was a problem hiding this comment.
nit: no need to instantiate a new string each time here. You can either declare the var outside the loop and overwrite each time, or just pass the results of strings.TrimSpace() directly to Sprintf().
iDiff/utils/format_utils.go
Outdated
| // return err | ||
| // } | ||
| // return templateOutput(diff) | ||
| // } |
iDiff/differs/differs.go
Outdated
| img1 := diff.Image1 | ||
| img2 := diff.Image2 | ||
| differ := diff.DiffType | ||
| eng := diff.UseDocker |
There was a problem hiding this comment.
This is a little weird to me; it seems like UseDocker is more like a global state of the program rather than a field in a struct. Can we maybe have this as a util function somewhere and then call that where ever you use this field?
There was a problem hiding this comment.
yeah honestly it was only included there because that's where all the other args (the two images and differ) are collected from the command line, but this eng bool that's currently required in a lot of our functions is in the process of being removed because @abbytiz is looking into Docker Registry stuff as per Dan's suggestion to remove Docker dependencies entirely... but in the meantime, should I go ahead and do that?
There was a problem hiding this comment.
Ah ok. Yeah since this is a refactor anyway, I'd say go ahead and just pull it out into a util function. Then whenever @abbytiz removes the Docker dependency it'll be easy to just swap it out in that util function or just remove it entirely
iDiff/utils/image_utils.go
Outdated
| @@ -7,7 +7,7 @@ import ( | |||
| "os" | |||
| "path/filepath" | |||
| "regexp" | |||
| "strings" | |||
| // "strings" | |||
cftorres
force-pushed
the
DiffRefactorOutput
branch
from
a8a0ea7
to
78934ea
Compare
| glog.Error(err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| if diff, err := differs.Diff(args[2], args[3], args[1], json, eng); err == nil { | ||
| fmt.Println(diff) | ||
| utils.SetDockerEngine(eng) |
There was a problem hiding this comment.
Eventually I think this should be determined automatically by the tool (and not rely on a flag being passed in by the user). This is fine for now though.
* Differ + image refactor * Output and DiffResult refactor * Breaking up JSON and text outputs * Refactored if branch chains to use reflection for image source determination * Extracted out eng
* Differ + image refactor * Output and DiffResult refactor * Breaking up JSON and text outputs * Refactored if branch chains to use reflection for image source determination * Extracted out eng
@nkubala @aaron-prindle @abbytiz