Single image analysis #20

cftorres · 2017-08-17T00:52:50Z

fixes #8
@aaron-prindle @nkubala @abbytiz

abbytiz · 2017-08-17T15:56:03Z

cmd/root.go

 		}

-		var wg sync.WaitGroup
-		wg.Add(2)
+		if len(args) == 1 {


It makes more sense to me to have analyze as a separate command (such that the logic wouldn't be in root.go) both for clarity and for example in case of future additional commands. If someone adds a command and pattern matches, they seem forced into expanding this if else structure. The primary logic already seems separated out so is there a reason not to pull it into it's own command file?

I just thought only one command would be preferable for our tool... @nkubala @aaron-prindle do you have any thoughts as to whether we should split this up into a separate command?

I actually think this structure is fine. We should probably at least put a comment here saying semantically what each case means (analyze = one image, diff = two), and then put something about it in the usage/--help command. I do like the tool choosing which path to take based on how many images you provide it though, seems intuitive to me

abbytiz · 2017-08-17T16:07:31Z

utils/diff_output_utils.go

+	OutputText(diffType string) error
+}
+
+type MultiPackageDiffResult struct {


nit: Did MultiVersionPackageDiffResult lose the "Version" part intentionally when it became MultiPackageAnalyzeResult? Also is SingleVersionDiffResult now SinglePackageAnalyzeResult? These names seem significantly less clear to me since the real crux of MultiVersion is that it supports multiple versions of the same package. Also SinglePackageAnalyzeResult seems like it would be an analysis of one package, rather than an analysis of all packages, given that each exists with a single version in the image.

I think I just omitted the "Version" part to shorten it because it seemed really long, but I can put it back... SingleVersionDiffResult never existed, I believe you called it PackageDiffResult and it's still there, but for consistency sake I can make it SingleVersionPackageDiffResult. These all just seem very long.

abbytiz · 2017-08-17T16:13:17Z

utils/docker_utils.go

@@ -110,10 +110,8 @@ func pullImageFromRepo(image string) (string, string, error) {
 }

 type HistDiff struct {
-	Image1 string


Where are the names being stored now?

I pulled them out into the DiffResult instead because they were previously in this Diff field of the DiffResult, and they didn't seem to be part of the actual diff per se

nkubala · 2017-08-17T16:52:33Z

cmd/root.go

 		}

-		var wg sync.WaitGroup
-		wg.Add(2)
+		if len(args) == 1 {


I actually think this structure is fine. We should probably at least put a comment here saying semantically what each case means (analyze = one image, diff = two), and then put something about it in the usage/--help command. I do like the tool choosing which path to take based on how many images you provide it though, seems intuitive to me

nkubala · 2017-08-17T16:57:45Z

cmd/root.go

+	image, err := utils.ImagePrepper{imageArg}.GetImage()
+	if err != nil {
+		glog.Error(err.Error())
+		os.Exit(1)


not sure if this is new to this PR, but GetImage() extracts the image filesystem and saves it on disk right? if so, calling os.Exit(1) here will skip the cleanup of that image if the user didn't want it around. probably want to defer an image cleanup in the root command, and then return an error here.

nkubala · 2017-08-17T16:59:28Z

cmd/root.go

+				glog.Error(err)
+			}
+		}
+		fmt.Println()


nit: use glog/the output buffer here?

Oh I actually don't even think this is necessary because there's a newline in the template, so I'll just get rid of it...

nkubala

sorry if i'm missing it, but can you add a test for this?

abbytiz · 2017-08-17T18:33:22Z

If I'm not mistaken this PR changes some names that are referenced in the Output section of the ReadMe and the Custom Differ Quickstart section. Also the new flag and it's usage should similarly be documented.

nkubala

code looks good, please update the docs/readme with the new names and the added flag

abbytiz · 2017-08-17T22:21:34Z

README.md

@@ -32,9 +33,19 @@ Download the [container-diff-windows-amd64.exe](https://storage.googleapis.com/c

 ## Quickstart

-To use container-diff you need two Docker images (in the form of an ID, tarball, or URL from a repo).  Once you have those images you can run any of the following differs:
+To use container-diff to perform single image analysis, you need one Docker image (in the form of an ID, tarball, or URL from a repo).  Once you have that images you can run any of the following analyzers:


nit: Once you have that, you can run (notice inserted comma and removed "images")

abbytiz · 2017-08-17T22:26:48Z

README.md

@@ -190,46 +240,43 @@ Version differences: None
 ```


-## Make your own differ
+## Make your own analyzer


The updates here look really good and seem really clear. Great work!

nkubala · 2017-08-17T22:52:52Z

README.md

@@ -32,9 +33,19 @@ Download the [container-diff-windows-amd64.exe](https://storage.googleapis.com/c

 ## Quickstart

-To use container-diff you need two Docker images (in the form of an ID, tarball, or URL from a repo).  Once you have those images you can run any of the following differs:
+To use container-diff to perform single image analysis, you need one Docker image (in the form of an ID, tarball, or URL from a repo).  Once you have that images you can run any of the following analyzers:


nit (for clarity): single image analysis -> analysis on a single image

nkubala · 2017-08-17T22:53:35Z

README.md

@@ -71,30 +83,79 @@ To use the docker client instead of shelling out to your local docker daemon, ad

 ```container-diff <img1> <img2> -e```

+## Analysis Result Format
+
+The jsons for analysis results are in the follow format:


follow -> following

nkubala · 2017-08-17T22:54:55Z

README.md


-The files system differ has the following json output structure: 
+The filessystem differ has the following json output structure: 


filessystem -> filesystem

cftorres · 2017-08-17T23:05:00Z

Edited README according to comments, let me know if good to merge!

cftorres added 2 commits August 16, 2017 17:37

Adding single image analysis

ad78c06

Merge remote-tracking branch 'upstream/master' into SingleImageAnalysis

6b359d2

cftorres force-pushed the SingleImageAnalysis branch from 6ce491c to 6b359d2 Compare August 17, 2017 16:02

abbytiz reviewed Aug 17, 2017

View reviewed changes

nkubala reviewed Aug 17, 2017

View reviewed changes

cftorres added 2 commits August 17, 2017 10:11

Addressing Abby's comments

414dd3a

Addressing Nick's comments

9d4ef87

cftorres force-pushed the SingleImageAnalysis branch from 26e670c to 9d4ef87 Compare August 17, 2017 17:50

nkubala reviewed Aug 17, 2017

View reviewed changes

Updated integration tests

b453dd4

nkubala suggested changes Aug 17, 2017

View reviewed changes

Modified README

2d09bc5

abbytiz reviewed Aug 17, 2017

View reviewed changes

Tweaked README after Abby's comment

f38e8a5

nkubala reviewed Aug 17, 2017

View reviewed changes

Tweaked README after Nick's comments

6c212b1

nkubala approved these changes Aug 18, 2017

View reviewed changes

cftorres merged commit 57cb4b6 into GoogleContainerTools:master Aug 18, 2017


		The files system differ has the following json output structure:
		The filessystem differ has the following json output structure:

Single image analysis #20

Single image analysis #20

Uh oh!

Conversation

cftorres commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

abbytiz commented Aug 17, 2017

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cftorres commented Aug 17, 2017

Uh oh!

Uh oh!