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

Commit 53c6341

Browse files
dlorencdlorenc
authored
Cleanup flag parsing. (#123)
* Cleanup flag parsing. This also changes the types flag from a single comma separated list to a repeated flag that fills an array. This changes the name from types to type, which is a minor breaking change. Let me know what you think. * Update docs and tests.
1 parent ba42184 commit 53c6341

File tree

6 files changed

+74
-69
lines changed

6 files changed

+74
-69
lines changed

‎README.md

Lines changed: 15 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -41,48 +41,37 @@ To use `container-diff analyze` to perform analysis on a single image, you need
4141

4242
```
4343
container-diff analyze <img> [Run default analyzers]
44-
container-diff analyze <img> --types=history [History]
45-
container-diff analyze <img> --types=file [File System]
46-
container-diff analyze <img> --types=pip [Pip]
47-
container-diff analyze <img> --types=apt [Apt]
48-
container-diff analyze <img> --types=node [Node]
49-
container-diff analyze <img> --types=apt,node [Apt and Node]
50-
# --types=<analyzer1>,<analyzer2>,<analyzer3>,...
44+
container-diff analyze <img> --type=history [History]
45+
container-diff analyze <img> --type=file [File System]
46+
container-diff analyze <img> --type=pip [Pip]
47+
container-diff analyze <img> --type=apt [Apt]
48+
container-diff analyze <img> --type=node [Node]
49+
container-diff analyze <img> --type=apt --type=node [Apt and Node]
50+
# --type=<analyzer1> --type=<analyzer2> --type=<analyzer3>,...
5151
```
5252

5353
To use container-diff to perform a diff analysis on two images, 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:
5454
```
5555
container-diff diff <img1> <img2> [Run all differs]
56-
container-diff diff <img1> <img2> --types=history [History]
57-
container-diff diff <img1> <img2> --types=file [File System]
58-
container-diff diff <img1> <img2> --types=pip [Pip]
59-
container-diff diff <img1> <img2> --types=apt [Apt]
60-
container-diff diff <img1> <img2> --types=node [Node]
56+
container-diff diff <img1> <img2> --type=history [History]
57+
container-diff diff <img1> <img2> --type=file [File System]
58+
container-diff diff <img1> <img2> --type=pip [Pip]
59+
container-diff diff <img1> <img2> --type=apt [Apt]
60+
container-diff diff <img1> <img2> --type=node [Node]
6161
```
6262

6363
You can similarly run many analyzers at once:
6464

6565
```
66-
container-diff diff <img1> <img2> --types=history,apt,node
66+
container-diff diff <img1> <img2> --type=history --type=apt --type=node
6767
```
6868

6969
To view the diff of an individual file in two different images, you can use the filename flag in conjuction with the file system diff analyzer.
7070

7171
```
72-
container-diff diff <img1> <img2> --types=file --filename=/path/to/file
72+
container-diff diff <img1> <img2> --type=file --filename=/path/to/file
7373
```
7474

75-
All of the analyzer flags with their long versions can be seen below:
76-
77-
| Differ | Short flag | Long Flag |
78-
| ------------------------- |:----------:| ----------:|
79-
| File system diff | -f | --file |
80-
| History | -d | --history |
81-
| npm installed packages | -n | --node |
82-
| pip installed packages | -p | --pip |
83-
| apt-get installed packages| -a | --apt |
84-
85-
8675
## Image Sources
8776

8877
container-diff supports Docker images located in both a local Docker daemon and a remote registry. To explicitly specify a local image, use the `daemon://` prefix on the image name; similarly, for an explicitly remote image, use the `remote://` prefix.
@@ -242,7 +231,7 @@ Tarballs provided directly to the tool must be in the Docker format (i.e. have a
242231
## Example Run
243232

244233
```
245-
$ container-diff diff gcr.io/google-appengine/python:2017-07-21-123058 gcr.io/google-appengine/python:2017-06-29-190410 --types=apt,node,pip
234+
$ container-diff diff gcr.io/google-appengine/python:2017-07-21-123058 gcr.io/google-appengine/python:2017-06-29-190410 --type=apt --type=node --type=pip
246235
247236
-----AptDiffer-----
248237

‎cmd/analyze.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"errors"
2121
"fmt"
2222
"os"
23-
"strings"
2423

2524
"github.com/GoogleCloudPlatform/container-diff/differs"
2625
pkgutil "github.com/GoogleCloudPlatform/container-diff/pkg/util"
@@ -33,16 +32,13 @@ var analyzeCmd = &cobra.Command{
3332
Short: "Analyzes an image: [image]",
3433
Long: `Analyzes an image using the specifed analyzers as indicated via flags (see documentation for available ones).`,
3534
Args: func(cmd *cobra.Command, args []string) error {
36-
if err := validateArgs(args, checkAnalyzeArgNum); err != nil {
37-
return err
38-
}
39-
if err := checkIfValidAnalyzer(types); err != nil {
35+
if err := validateArgs(args, checkAnalyzeArgNum, checkIfValidAnalyzer); err != nil {
4036
return err
4137
}
4238
return nil
4339
},
4440
Run: func(cmd *cobra.Command, args []string) {
45-
if err := analyzeImage(args[0], strings.Split(types, ",")); err != nil {
41+
if err := analyzeImage(args[0], types); err != nil {
4642
logrus.Error(err)
4743
os.Exit(1)
4844
}

‎cmd/diff.go

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,14 @@ package cmd
1919
import (
2020
"errors"
2121
"fmt"
22+
"os"
23+
"sync"
24+
2225
"github.com/GoogleCloudPlatform/container-diff/differs"
2326
pkgutil "github.com/GoogleCloudPlatform/container-diff/pkg/util"
2427
"github.com/GoogleCloudPlatform/container-diff/util"
2528
"github.com/sirupsen/logrus"
2629
"github.com/spf13/cobra"
27-
"os"
28-
"strings"
29-
"sync"
3030
)
3131

3232
var filename string
@@ -36,19 +36,13 @@ var diffCmd = &cobra.Command{
3636
Short: "Compare two images: [image1] [image2]",
3737
Long: `Compares two images using the specifed analyzers as indicated via flags (see documentation for available ones).`,
3838
Args: func(cmd *cobra.Command, args []string) error {
39-
if err := validateArgs(args, checkDiffArgNum); err != nil {
40-
return err
41-
}
42-
if err := checkIfValidAnalyzer(types); err != nil {
43-
return err
44-
}
45-
if err := checkFilenameFlag(types); err != nil {
39+
if err := validateArgs(args, checkDiffArgNum, checkIfValidAnalyzer, checkFilenameFlag); err != nil {
4640
return err
4741
}
4842
return nil
4943
},
5044
Run: func(cmd *cobra.Command, args []string) {
51-
if err := diffImages(args[0], args[1], strings.Split(types, ",")); err != nil {
45+
if err := diffImages(args[0], args[1], types); err != nil {
5246
logrus.Error(err)
5347
os.Exit(1)
5448
}
@@ -62,13 +56,16 @@ func checkDiffArgNum(args []string) error {
6256
return nil
6357
}
6458

65-
func checkFilenameFlag(types string) error {
66-
if filename != "" {
67-
if !strings.Contains(types, "file") {
68-
return errors.New("Please include --types=file with the --filename flag")
59+
func checkFilenameFlag(_ []string) error {
60+
if filename == "" {
61+
return nil
62+
}
63+
for _, t := range types {
64+
if t == "file" {
65+
return nil
6966
}
7067
}
71-
return nil
68+
return errors.New("Please include --types=file with the --filename flag")
7269
}
7370

7471
func diffImages(image1Arg, image2Arg string, diffArgs []string) error {

‎cmd/root.go

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package cmd
1818

1919
import (
20-
"errors"
2120
goflag "flag"
2221
"fmt"
2322
"os"
@@ -35,7 +34,7 @@ import (
3534

3635
var json bool
3736
var save bool
38-
var types string
37+
var types diffTypes
3938

4039
var LogLevel string
4140

@@ -104,12 +103,11 @@ func validateArgs(args []string, validatefxns ...validatefxn) error {
104103
return nil
105104
}
106105

107-
func checkIfValidAnalyzer(flagtypes string) error {
108-
if flagtypes == "" {
109-
return errors.New("Please provide at least one analyzer to run")
106+
func checkIfValidAnalyzer(_ []string) error {
107+
if len(types) == 0 {
108+
types = []string{"apt"}
110109
}
111-
analyzers := strings.Split(flagtypes, ",")
112-
for _, name := range analyzers {
110+
for _, name := range types {
113111
if _, exists := differs.Analyzers[name]; !exists {
114112
return fmt.Errorf("Argument %s is not a valid analyzer", name)
115113
}
@@ -146,9 +144,35 @@ func init() {
146144
pflag.CommandLine.AddGoFlagSet(goflag.CommandLine)
147145
}
148146

147+
// Define a type named "diffSlice" as a slice of strings
148+
type diffTypes []string
149+
150+
// Now, for our new type, implement the two methods of
151+
// the flag.Value interface...
152+
// The first method is String() string
153+
func (d *diffTypes) String() string {
154+
return strings.Join(*d, ",")
155+
}
156+
157+
// The second method is Set(value string) error
158+
func (d *diffTypes) Set(value string) error {
159+
// Dedupe repeated elements.
160+
for _, t := range *d {
161+
if t == value {
162+
return nil
163+
}
164+
}
165+
*d = append(*d, value)
166+
return nil
167+
}
168+
169+
func (d *diffTypes) Type() string {
170+
return "Diff Types"
171+
}
172+
149173
func addSharedFlags(cmd *cobra.Command) {
150174
cmd.Flags().BoolVarP(&json, "json", "j", false, "JSON Output defines if the diff should be returned in a human readable format (false) or a JSON (true).")
151-
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.")
175+
cmd.Flags().VarP(&types, "type", "t", "This flag sets the list of analyzer types to use. Set it repeatedly to use multiple analyzers.")
152176
cmd.Flags().BoolVarP(&save, "save", "s", false, "Set this flag to save rather than remove the final image filesystems on exit.")
153177
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.")
154178
}

‎docker_diff.bzl

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,7 @@ def _impl(ctx):
2323
#!/bin/bash
2424
set -e
2525
./%s diff %s %s""" % (container_diff_loction, image_location, ctx.attr.diff_base)
26-
if ctx.attr.diff_types:
27-
content += " --types=%s" % (",".join(ctx.attr.diff_types))
26+
content += " ".join(["--type=%s" % t for t in ctx.attr.diff_types])
2827

2928
ctx.file_action(
3029
output = ctx.outputs.executable,

‎tests/integration_test.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -97,83 +97,83 @@ func TestDiffAndAnalysis(t *testing.T) {
9797
subcommand: "diff",
9898
imageA: diffBase,
9999
imageB: diffModified,
100-
differFlags: []string{"--types=file"},
100+
differFlags: []string{"--type=file"},
101101
expectedFile: "file_diff_expected.json",
102102
},
103103
{
104104
description: "apt differ",
105105
subcommand: "diff",
106106
imageA: aptBase,
107107
imageB: aptModified,
108-
differFlags: []string{"--types=apt"},
108+
differFlags: []string{"--type=apt"},
109109
expectedFile: "apt_diff_expected.json",
110110
},
111111
{
112112
description: "node differ",
113113
subcommand: "diff",
114114
imageA: nodeBase,
115115
imageB: nodeModified,
116-
differFlags: []string{"--types=node"},
116+
differFlags: []string{"--type=node"},
117117
expectedFile: "node_diff_order_expected.json",
118118
},
119119
{
120120
description: "multi differ",
121121
subcommand: "diff",
122122
imageA: multiBase,
123123
imageB: multiModified,
124-
differFlags: []string{"--types=node,pip,apt"},
124+
differFlags: []string{"--type=node", "--type=pip", "--type=apt"},
125125
expectedFile: "multi_diff_expected.json",
126126
},
127127
{
128128
description: "multi differ local",
129129
subcommand: "diff",
130130
imageA: multiBaseLocal,
131131
imageB: multiModifiedLocal,
132-
differFlags: []string{"--types=node,pip,apt"},
132+
differFlags: []string{"--type=node", "--type=pip", "--type=apt"},
133133
expectedFile: "multi_diff_expected.json",
134134
},
135135
{
136136
description: "history differ",
137137
subcommand: "diff",
138138
imageA: diffBase,
139139
imageB: diffModified,
140-
differFlags: []string{"--types=history"},
140+
differFlags: []string{"--type=history"},
141141
expectedFile: "hist_diff_expected.json",
142142
},
143143
{
144144
description: "apt sorted differ",
145145
subcommand: "diff",
146146
imageA: aptBase,
147147
imageB: aptModified,
148-
differFlags: []string{"--types=apt", "-o"},
148+
differFlags: []string{"--type=apt", "-o"},
149149
expectedFile: "apt_sorted_diff_expected.json",
150150
},
151151
{
152152
description: "apt analysis",
153153
subcommand: "analyze",
154154
imageA: aptModified,
155-
differFlags: []string{"--types=apt"},
155+
differFlags: []string{"--type=apt"},
156156
expectedFile: "apt_analysis_expected.json",
157157
},
158158
{
159159
description: "file sorted analysis",
160160
subcommand: "analyze",
161161
imageA: diffModified,
162-
differFlags: []string{"--types=file", "-o"},
162+
differFlags: []string{"--type=file", "-o"},
163163
expectedFile: "file_sorted_analysis_expected.json",
164164
},
165165
{
166166
description: "pip analysis",
167167
subcommand: "analyze",
168168
imageA: pipModified,
169-
differFlags: []string{"--types=pip"},
169+
differFlags: []string{"--type=pip"},
170170
expectedFile: "pip_analysis_expected.json",
171171
},
172172
{
173173
description: "node analysis",
174174
subcommand: "analyze",
175175
imageA: nodeModified,
176-
differFlags: []string{"--types=node"},
176+
differFlags: []string{"--type=node"},
177177
expectedFile: "node_analysis_expected.json",
178178
},
179179
}

0 commit comments

Comments
 (0)