Diff within files #121

priyawadhwa · 2017-10-17T20:24:31Z

To diff within files, --types must include the file analyzer and the --filename flag must be set.
--filename points to the path of the file in both images.

ex. container-diff diff file1.tar file2.tar --types=file --filename=/app/file.txt

r2d4

Looking good! Just a few comments

r2d4 · 2017-10-17T20:29:41Z

cmd/diff.go

@@ -116,6 +130,20 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
 	return nil
 }

+func diffFile(imageMap map[string]*pkgutil.Image, image1Arg, image2Arg string) error {
+
+	image1FilePath := imageMap[image1Arg].FSPath + filename


these should probably use something like path.Join https://golang.org/pkg/path/#Join

r2d4 · 2017-10-17T20:32:10Z

pkg/util/fs_utils.go

@@ -53,6 +54,24 @@ func GetSize(path string) int64 {
 	return stat.Size()
 }

+//GetFileContents returns the contents of a file at the specified path
+func GetFileContents(path string) ([]string, error) {
+	stat, err := os.Stat(path)


Is this checking for the existence of a file?

It should be

if _, err := os.Stat("/path/to/whatever"); os.IsNotExist(err) { // path/to/whatever does not exist }

it does check for existence, but also checks that the path isn't a directory after, which is why I thought it might be easier to split it up?

I guess what is the expected behavior when the file doesn't exist? I think you wouldn't want to throw an error?
Different scenarios:

A: ""
B: no file
output: ?

A: "something"
B: no file
output: ?

A: no file
B: no file
output: ?

r2d4 · 2017-10-17T20:32:52Z

pkg/util/fs_utils.go

+	if err != nil {
+		return nil, err
+	}
+	return []string{string(contents)}, nil


Why doesn't this return a normal string?

the diff library I'm using wanted this format, but it probably makes more sense to move that to DiffFile

r2d4 · 2017-10-17T20:33:38Z

util/diff_utils.go

@@ -118,6 +123,36 @@ func DiffDirectory(d1, d2 pkgutil.Directory) (DirDiff, bool) {
 	return DirDiff{addedEntries, deletedEntries, modifiedEntries}, same
 }

+//DiffFile diffs within a file
+func DiffFile(path1, path2, image1, image2 string) (FileNameDiff, error) {
+


remove newline

r2d4 · 2017-10-17T20:34:06Z

util/diff_utils.go

+
+	image1Contents, err := pkgutil.GetFileContents(path1)
+	if err != nil {
+		return result, err


when you encounter an error, you should return nil, err

r2d4 · 2017-10-17T20:38:13Z

cmd/root.go

@@ -151,4 +157,5 @@ func addSharedFlags(cmd *cobra.Command) {
 	cmd.Flags().StringVarP(&types, "types", "t", "apt", "This flag sets the list of analyzer types to use.  It expects a comma separated list of supported analyzers.")
 	cmd.Flags().BoolVarP(&save, "save", "s", false, "Set this flag to save rather than remove the final image filesystems on exit.")
 	cmd.Flags().BoolVarP(&util.SortSize, "order", "o", false, "Set this flag to sort any file/package results by descending size. Otherwise, they will be sorted by name.")
+	cmd.Flags().StringVarP(&filename, "filename", "f", "", "View the diff of a file in both containers (can only be used with container-diff diff)")


You should add this flag to the diff subcommand specific flags instead of have it be at the root level. That will let you remove a lot of the flag validation.

r2d4 · 2017-10-17T20:39:31Z

util/diff_utils.go

@@ -118,6 +123,36 @@ func DiffDirectory(d1, d2 pkgutil.Directory) (DirDiff, bool) {
 	return DirDiff{addedEntries, deletedEntries, modifiedEntries}, same
 }

+//DiffFile diffs within a file
+func DiffFile(path1, path2, image1, image2 string) (FileNameDiff, error) {


This doesn't satisfy any interface right? In that case, you should probably pass the filename parameter into here too and set it on the FileNameDiff

r2d4 · 2017-10-17T20:40:04Z

cmd/diff.go

+	if err != nil {
+		return err
+	}
+	diff.Filename = filename


Can you pass filename into util.DiffFile so you don't have to set this after the fact?

r2d4 · 2017-10-17T20:43:02Z

cmd/diff.go

+	image1FilePath := imageMap[image1Arg].FSPath + filename
+	image2FilePath := imageMap[image2Arg].FSPath + filename
+
+	diff, err := util.DiffFile(image1FilePath, image2FilePath, image1Arg, image2Arg)


I think util.DiffFile can do the path joining actually.

You could make util.DiffFile look like

func DiffFile(image1, image2, filename string) (FileNameDiff, error) { // join paths // run diff // return full FileNameDiff object }

r2d4 · 2017-10-17T20:43:30Z

cmd/analyze.go

@@ -56,6 +59,13 @@ func checkAnalyzeArgNum(args []string) error {
 	return nil
 }

+func checkAnalyzeFilenameFlag() error {


remove this once you've added the flag under container-diff diff only

dlorenc

Nice!

nkubala · 2017-10-19T16:29:16Z

can you add some documentation about this in the README?

Priya Wadhwa added 2 commits October 17, 2017 12:58

Diff by filename

62ea1b2

merged with master

2326e06

r2d4 reviewed Oct 17, 2017

View reviewed changes

Moved filename flag to diff, rewrote DiffFile

553d639

priyawadhwa force-pushed the diff-within-files branch from 6314a0a to 553d639 Compare October 17, 2017 23:41

filename flag must be used with --types=file flag

f376e3a

dlorenc approved these changes Oct 19, 2017

View reviewed changes

dlorenc merged commit cf169f6 into GoogleContainerTools:master Oct 19, 2017

Diff within files #121

Diff within files #121

Uh oh!

Conversation

priyawadhwa commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r2d4 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!

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!

dlorenc left a comment

Choose a reason for hiding this comment

Uh oh!

nkubala commented Oct 19, 2017

Uh oh!

Uh oh!

priyawadhwa commented Oct 17, 2017 •

edited

Loading