Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Enhancement - save to file #279

Merged
merged 3 commits into from Dec 12, 2018
Merged

Enhancement - save to file #279

merged 3 commits into from Dec 12, 2018

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Dec 7, 2018

This pull request will close #275, specifically allowing the user to specify a file to write to. Notes are included below.

What is the entrypoint for the user?

The application flow beings with the addition of a new variable, outputFile

cmd.Flags().StringVarP(&outputFile, "output", "w", "", "output file to write to (default writes to STDOUT).")

It's a string, the default being empty, that if not empty, will serve two purposes:

  • serve as a boolean to indicate we want to write to file
  • the string variable itself provides the path to the file.

With any write to file it goes without saying we don't want to overwrite by default, but we can give the user a --force variable to push it.

cmd.Flags().BoolVar(&forceWrite, "force", false, "force overwrite output file, if exists already.")

Variable Choices

I chose output for the command line flag (--output) because it reads nicely into a sentence, e.g., "--output=/path/to/file.json" reads to output goes to this file path. I also chose w for the single character (well, o was taken) but also because w can also stand for "write," sort of like the gofmt -w <file> command. So "-w /path/to/file.json" reads to "write to this file path).

What is the Client Flow?

The flag is revealed as an option --output if we do analyze or diff. Note that I also added a force option in the case that the file exists. The user can choose to overwrite it with --force.

$ ./out/container-diff analyze --help
Analyzes an image using the specifed analyzers as indicated via flags (see documentation for available ones).

Usage:
  container-diff analyze [flags]

Flags:
  -c, --cache-dir string   cache directory base to create .container-diff (default is $HOME).
      --force              force overwrite output file, if exists already.
  -h, --help               help for analyze
  -j, --json               JSON Output defines if the diff should be returned in a human readable format (false) or a JSON (true).
  -n, --no-cache           Set this to force retrieval of image filesystem on each run.
  -o, --order              Set this flag to sort any file/package results by descending size. Otherwise, they will be sorted by name.
  -w, --output string      output file to write to (default writes to the screen).
  -q, --quiet              Suppress output to stderr.
  -s, --save               Set this flag to save rather than remove the final image filesystems on exit.
  -t, --type Diff Types    This flag sets the list of analyzer types to use. Set it repeatedly to use multiple analyzers.

Global Flags:
      --format string      Format to output diff in.
  -v, --verbosity string   This flag controls the verbosity of container-diff. (default "warning")

And here is for diff, it's mostly the same.

$ ./out/container-diff diff --help
Usage:
  container-diff diff [flags]

Flags:
  -c, --cache-dir string   cache directory base to create .container-diff (default is $HOME).
  -f, --filename string    Set this flag to the path of a file in both containers to view the diff of the file. Must be used with --types=file flag.
      --force              force overwrite output file, if exists already.
  -h, --help               help for diff
  -j, --json               JSON Output defines if the diff should be returned in a human readable format (false) or a JSON (true).
  -n, --no-cache           Set this to force retrieval of image filesystem on each run.
  -o, --order              Set this flag to sort any file/package results by descending size. Otherwise, they will be sorted by name.
  -w, --output string      output file to write to (default writes to the screen).
  -q, --quiet              Suppress output to stderr.
  -s, --save               Set this flag to save rather than remove the final image filesystems on exit.
  -t, --type Diff Types    This flag sets the list of analyzer types to use. Set it repeatedly to use multiple analyzers.

Global Flags:
      --format string      Format to output diff in.
  -v, --verbosity string   This flag controls the verbosity of container-diff. (default "warning")

I specifically didn't add a short hand for "force." If a user wants to overwrite a file, it's a pretty big deal, and if they might mess up the specification of one letter (and accidentally overwrite) I don't want to risk this.

Manual Testing

We can first test that, without any specification of --output, it still works (and prints to stdout).

$ ./out/container-diff analyze ubuntu

-----Size-----

Analysis for ubuntu:
IMAGE         DIGEST                                                                         SIZE
ubuntu        sha256:acd85db6e4b18aafa7fcde5480872909bd8e6d5fbd4e5e790ecc09acc06a8b78        68.6M

Now let's write to a text file.

$ ./out/container-diff analyze --output /tmp/file.txt ubuntu
$ cat /tmp/file.txt

-----Size-----

Analysis for ubuntu:
IMAGE         DIGEST                                                                         SIZE
ubuntu        sha256:acd85db6e4b18aafa7fcde5480872909bd8e6d5fbd4e5e790ecc09acc06a8b78        68.6M

We can then try to write to the same file, it shouldn't allow us!

./out/container-diff analyze --output /tmp/file.txt ubuntu
ERRO[0001] file exist, will not overwrite.

...unless we tell it to force! (Use the force, logrus)

$ ./out/container-diff analyze --output /tmp/file.txt --force ubuntu

(Note no error messages).

We can do all of the above but specify json output.

$ ./out/container-diff analyze --output /tmp/file.json --json ubuntu
$ cat /tmp/file.json 
[
  {
    "Image": "ubuntu",
    "AnalyzeType": "Size",
    "Analysis": [
      {
        "Name": "ubuntu",
        "Digest": "sha256:acd85db6e4b18aafa7fcde5480872909bd8e6d5fbd4e5e790ecc09acc06a8b78",
        "Size": 71945016
      }
    ]
  }
]
$ ./out/container-diff analyze --output/tmp/file.json --json ubuntu
ERRO[0001] file exist, will not overwrite.              
$ ./out/container-diff analyze --output /tmp/file.json --force --json ubuntu

I think that's about it! I'm guessing you will want some tests? Let's have (high level discussion) on what we want to test (and recommendations for doing things like writing temporary files, organization and naming, etc.) and then I can give a GO at that too (pun, harhar).

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsoch thanks for sending this PR! Logic looks great, just a few small nits mostly related to style. Flag names, etc. all LGTM.

cmd/diff.go Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsoch thanks again for another great PR!

@nkubala nkubala merged commit ea8bccc into GoogleContainerTools:master Dec 12, 2018
@vsoch
Copy link
Contributor Author

vsoch commented Dec 12, 2018

Sweeet!! Oh man I am so excited to run this on our cluster! 😂 🎈 🐱 🥑

@nkubala
Copy link
Contributor

nkubala commented Dec 12, 2018

👍 I'm excited for you! Feel free to share any cool results you come up with 😄 I'll try and get a release out ASAP

@vsoch
Copy link
Contributor Author

vsoch commented Dec 20, 2018

Thanks again! Here is a little exploratory analysis and writeup I did --> https://twitter.com/vsoch/status/1075886704266014720

container-diff made the extraction of the Dockerfile and container metadata possible!

@nkubala
Copy link
Contributor

nkubala commented Jan 7, 2019

@vsoch super interesting writeup! I think it's awesome that you're not just using containers, but thinking about what they really are and what we can learn by understanding them on a fundamental level. and that you're using container-diff to do it 😆

@vsoch
Copy link
Contributor Author

vsoch commented Jan 7, 2019

Thank @nkubala ! I have another thing I'm working on (containertree) that also is using container diff to derive some better understanding, but I'm still working on the "prove it's useful with analysis examples" bit. I'll share more if/when I get there!

@nkubala
Copy link
Contributor

nkubala commented Jan 7, 2019

oooh....my interest is piqued 👀 please do!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to write output to file
2 participants