Add some validation checks

.redhat-ci.sh

@@ -8,15 +8,21 @@ export GOSRC=$HOME/gopath/src/github.com/projectatomic/buildah
 (mkdir -p $GOSRC && cd /code && cp -r . $GOSRC)
 
 dnf install -y \
-  make \
-  golang \
   bats \
   btrfs-progs-devel \
+  bzip2 \
   device-mapper-devel \
+  findutils \
+  git \
+  golang \
   gpgme-devel \
   libassuan-devel \
-  git \
-  bzip2
+  make \
+  which
 
-make -C $GOSRC
+# Red Hat CI adds a merge commit, for testing, which fails the
+# short-commit-subject validation test, so tell git-validate.sh to only check
+# up to, but not including, the merge commit.
+export GITVALIDATE_TIP=$(cd $GOSRC; git log -2 --pretty='%H' | tail -n 1)
+make -C $GOSRC install.tools all validate
 $GOSRC/tests/test_runner.sh

.travis.yml

@@ -8,7 +8,7 @@ sudo: required
 before_install:
     - sudo add-apt-repository -y ppa:duggan/bats
     - sudo apt-get -qq update
-    - sudo apt-get -qq install bats btrfs-tools libdevmapper-dev libgpgme11-dev
+    - sudo apt-get -qq install bats btrfs-tools git libdevmapper-dev libgpgme11-dev
 script:
-    - make
+    - make install.tools all validate
     - cd tests; sudo PATH="$PATH" ./test_runner.sh

Makefile

@@ -2,11 +2,12 @@ AUTOTAGS := $(shell ./btrfs_tag.sh) $(shell ./libdm_tag.sh)
 PREFIX := $(DESTDIR)/usr/local
 BINDIR := $(PREFIX)/bin
 BASHINSTALLDIR=${PREFIX}/share/bash-completion/completions
+BUILDFLAGS := -tags "$(AUTOTAGS) $(TAGS)"
 
-all: buildah install.tools docs
+all: buildah docs
 
 buildah: *.go cmd/buildah/*.go
-	go build -o buildah -tags "$(AUTOTAGS) $(TAGS)" ./cmd/buildah
+	go build -o buildah $(BUILDFLAGS) ./cmd/buildah
 
 .PHONY: clean
 clean:
@@ -15,7 +16,7 @@ clean:
 
 .PHONY: docs
 docs: ## build the docs on the host
-	$(MAKE) -C docs docs
+	$(MAKE) -C docs
 
 # For vendoring to work right, the checkout directory must be such that our top
 # level is at $GOPATH/src/github.com/projectatomic/buildah.
@@ -28,18 +29,23 @@ gopath:
 deps: gopath
 	env GOPATH=$(shell cd ../../../.. ; pwd) vndr
 
-install:
-	install -D -m0755 buildah $(BINDIR)/buildah
-	$(MAKE) -C docs install
-
+.PHONY: validate
+validate:
+	@./tests/validate/gofmt.sh
+	@./tests/validate/git-validation.sh
+	@./tests/validate/gometalinter.sh . cmd/buildah
 
 .PHONY: install.tools
+install.tools:
+	go get -u $(BUILDFLAGS) github.com/cpuguy83/go-md2man
+	go get -u $(BUILDFLAGS) github.com/vbatts/git-validation
+	go get -u $(BUILDFLAGS) gopkg.in/alecthomas/gometalinter.v1
+	gometalinter.v1 -i
 
-install.tools: .install.md2man
-
-.install.md2man:
-	go get github.com/cpuguy83/go-md2man
-
+.PHONY: install
+install:
+	install -D -m0755 buildah $(BINDIR)/buildah
+	$(MAKE) -C docs install
 
 .PHONY: install.completions
 install.completions:

add.go

@@ -16,10 +16,10 @@ import (
 	"github.com/containers/storage/pkg/chrootarchive"
 )
 
-// addUrl copies the contents of the source URL to the destination.  This is
+// addURL copies the contents of the source URL to the destination.  This is
 // its own function so that deferred closes happen after we're done pulling
 // down each item of potentially many.
-func addUrl(destination, srcurl string) error {
+func addURL(destination, srcurl string) error {
 	logrus.Debugf("saving %q to %q", srcurl, destination)
 	resp, err := http.Get(srcurl)
 	if err != nil {
@@ -31,18 +31,21 @@ func addUrl(destination, srcurl string) error {
 		return fmt.Errorf("error creating %q: %v", destination, err)
 	}
 	if last := resp.Header.Get("Last-Modified"); last != "" {
-		if mtime, err := time.Parse(time.RFC1123, last); err != nil {
-			logrus.Debugf("error parsing Last-Modified time %q: %v", last, err)
+		if mtime, err2 := time.Parse(time.RFC1123, last); err2 != nil {
+			logrus.Debugf("error parsing Last-Modified time %q: %v", last, err2)
 		} else {
 			defer func() {
-				if err := os.Chtimes(destination, time.Now(), mtime); err != nil {
-					logrus.Debugf("error setting mtime to Last-Modified time %q: %v", last, err)
+				if err3 := os.Chtimes(destination, time.Now(), mtime); err3 != nil {
+					logrus.Debugf("error setting mtime to Last-Modified time %q: %v", last, err3)
 				}
 			}()
 		}
 	}
 	defer f.Close()
 	n, err := io.Copy(f, resp.Body)
+	if err != nil {
+		return fmt.Errorf("error reading contents for %q: %v", destination, err)
+	}
 	if resp.ContentLength >= 0 && n != resp.ContentLength {
 		return fmt.Errorf("error reading contents for %q: wrong length (%d != %d)", destination, n, resp.ContentLength)
 	}
@@ -69,7 +72,7 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error
 	if destination != "" && filepath.IsAbs(destination) {
 		dest = filepath.Join(dest, destination)
 	} else {
-		if err := os.MkdirAll(filepath.Join(dest, b.Workdir), 0755); err != nil {
+		if err = os.MkdirAll(filepath.Join(dest, b.Workdir), 0755); err != nil {
 			return fmt.Errorf("error ensuring directory %q exists: %v)", filepath.Join(dest, b.Workdir), err)
 		}
 		dest = filepath.Join(dest, b.Workdir, destination)
@@ -78,12 +81,12 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error
 	// with a '/', create it so that we can be sure that it's a directory,
 	// and any files we're copying will be placed in the directory.
 	if len(destination) > 0 && destination[len(destination)-1] == os.PathSeparator {
-		if err := os.MkdirAll(dest, 0755); err != nil {
+		if err = os.MkdirAll(dest, 0755); err != nil {
 			return fmt.Errorf("error ensuring directory %q exists: %v)", dest, err)
 		}
 	}
 	// Make sure the destination's parent directory is usable.
-	if fi, err := os.Stat(filepath.Dir(dest)); err == nil && !fi.Mode().IsDir() {
+	if destpfi, err2 := os.Stat(filepath.Dir(dest)); err2 == nil && !destpfi.Mode().IsDir() {
 		return fmt.Errorf("%q already exists, but is not a subdirectory)", filepath.Dir(dest))
 	}
 	// Now look at the destination itself.
@@ -112,7 +115,7 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error
 			if destfi != nil && destfi.Mode().IsDir() {
 				d = filepath.Join(dest, path.Base(url.Path))
 			}
-			if err := addUrl(d, src); err != nil {
+			if err := addURL(d, src); err != nil {
 				return err
 			}
 			continue

buildah.go

@@ -155,7 +155,7 @@ func OpenBuilder(store storage.Store, container string) (*Builder, error) {
 		return nil, err
 	}
 	b := &Builder{}
-	err = json.Unmarshal([]byte(buildstate), &b)
+	err = json.Unmarshal(buildstate, &b)
 	if err != nil {
 		return nil, err
 	}
@@ -198,7 +198,7 @@ func OpenBuilderByPath(store storage.Store, path string) (*Builder, error) {
 			return nil, err
 		}
 		b := &Builder{}
-		err = json.Unmarshal([]byte(buildstate), &b)
+		err = json.Unmarshal(buildstate, &b)
 		if err == nil && b.Type == containerType && builderMatchesPath(b, abs) {
 			b.store = store
 			return b, nil
@@ -224,7 +224,7 @@ func OpenAllBuilders(store storage.Store) (builders []*Builder, err error) {
 			continue
 		}
 		b := &Builder{}
-		err = json.Unmarshal([]byte(buildstate), &b)
+		err = json.Unmarshal(buildstate, &b)
 		if err == nil && b.Type == containerType {
 			b.store = store
 			builders = append(builders, b)

cmd/buildah/config.go

@@ -108,8 +108,8 @@ func updateConfig(builder *buildah.Builder, c *cli.Context) {
 		}
 	}
 	if c.IsSet("env") {
-		for _, envSpec := range c.StringSlice("env") {
-			builder.Env = append(builder.Env, envSpec)
+		if envSpec := c.StringSlice("env"); len(envSpec) > 0 {
+			builder.Env = append(builder.Env, envSpec...)
 		}
 	}
 	if c.IsSet("entrypoint") {
@@ -129,8 +129,8 @@ func updateConfig(builder *buildah.Builder, c *cli.Context) {
 		}
 	}
 	if c.IsSet("volume") {
-		for _, volSpec := range c.StringSlice("volume") {
-			builder.Volumes = append(builder.Volumes, volSpec)
+		if volSpec := c.StringSlice("volume"); len(volSpec) > 0 {
+			builder.Volumes = append(builder.Volumes, volSpec...)
 		}
 	}
 	if c.IsSet("label") {

cmd/buildah/main.go

@@ -64,7 +64,7 @@ func main() {
 			if err != nil {
 				return err
 			}
-			store.Shutdown(false)
+			_, _ = store.Shutdown(false)
 		}
 		return nil
 	}

cmd/buildah/rmi.go

@@ -44,11 +44,11 @@ func rmiCmd(c *cli.Context) error {
 			// it and check if it corresponds to an image that
 			// actually exists.
 			if ref2, err2 := alltransports.ParseImageName(id); err2 == nil {
-				if img, err2 := ref2.NewImage(nil); err2 == nil {
+				if img, err3 := ref2.NewImage(nil); err3 == nil {
 					img.Close()
 					ref = ref2
 				} else {
-					logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err2)
+					logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err3)
 				}
 			} else {
 				logrus.Debugf("error parsing %q as an image reference: %v", id, err2)
@@ -59,11 +59,11 @@ func rmiCmd(c *cli.Context) error {
 				// if it corresponds to an image that actually
 				// exists.
 				if ref2, err2 := storage.Transport.ParseStoreReference(store, id); err2 == nil {
-					if img, err2 := ref2.NewImage(nil); err2 == nil {
+					if img, err3 := ref2.NewImage(nil); err3 == nil {
 						img.Close()
 						ref = ref2
 					} else {
-						logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err2)
+						logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err3)
 					}
 				} else {
 					logrus.Debugf("error parsing %q as a store reference: %v", id, err2)
@@ -78,11 +78,11 @@ func rmiCmd(c *cli.Context) error {
 				// the ID directly above, but it can't hurt,
 				// either.
 				if ref2, err2 := storage.Transport.ParseStoreReference(store, "@"+id); err2 == nil {
-					if img, err2 := ref2.NewImage(nil); err2 == nil {
+					if img, err3 := ref2.NewImage(nil); err3 == nil {
 						img.Close()
 						ref = ref2
 					} else {
-						logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err2)
+						logrus.Debugf("error confirming presence of image %q: %v", transports.ImageName(ref2), err3)
 					}
 				} else {
 					logrus.Debugf("error parsing %q as an image reference: %v", "@"+id, err2)

config.go

@@ -102,9 +102,7 @@ func (b *Builder) updatedConfig() []byte {
 		image.Config.WorkingDir = b.Workdir
 	}
 	if len(b.Env) > 0 {
-		for _, envSpec := range b.Env {
-			image.Config.Env = append(image.Config.Env, envSpec)
-		}
+		image.Config.Env = append(image.Config.Env, b.Env...)
 	}
 	if len(b.Cmd) > 0 {
 		image.Config.Cmd = b.Cmd

image.go

@@ -120,7 +120,6 @@ func (i *containerImageRef) NewImageSource(sc *types.SystemContext, manifestType
 
 	image.RootFS.Type = "layers"
 	image.RootFS.DiffIDs = []string{}
-	lastLayerDiffID := ""
 
 	for _, layerID := range layers {
 		rc, err := i.store.Diff("", layerID)
@@ -182,7 +181,7 @@ func (i *containerImageRef) NewImageSource(sc *types.SystemContext, manifestType
 			Size:      size,
 		}
 		manifest.Layers = append(manifest.Layers, layerDescriptor)
-		lastLayerDiffID = destHasher.Digest().String()
+		lastLayerDiffID := destHasher.Digest().String()
 		image.RootFS.DiffIDs = append(image.RootFS.DiffIDs, lastLayerDiffID)
 	}
 

import.go

@@ -10,7 +10,6 @@ import (
 func importBuilder(store storage.Store, options ImportOptions) (*Builder, error) {
 	manifest := []byte{}
 	config := []byte{}
-	name := ""
 	image := ""
 	imageName := ""
 
@@ -26,13 +25,13 @@ func importBuilder(store storage.Store, options ImportOptions) (*Builder, error)
 	systemContext := getSystemContext(options.SignaturePolicyPath)
 
 	if c.ImageID != "" {
-		ref, err := is.Transport.ParseStoreReference(store, "@"+c.ImageID)
-		if err != nil {
-			return nil, fmt.Errorf("no such image %q: %v", "@"+c.ImageID, err)
+		ref, err2 := is.Transport.ParseStoreReference(store, "@"+c.ImageID)
+		if err2 != nil {
+			return nil, fmt.Errorf("no such image %q: %v", "@"+c.ImageID, err2)
 		}
-		src, err := ref.NewImage(systemContext)
-		if err != nil {
-			return nil, fmt.Errorf("error instantiating image: %v", err)
+		src, err3 := ref.NewImage(systemContext)
+		if err3 != nil {
+			return nil, fmt.Errorf("error instantiating image: %v", err3)
 		}
 		defer src.Close()
 		config, err = src.ConfigBlob()
@@ -44,14 +43,14 @@ func importBuilder(store storage.Store, options ImportOptions) (*Builder, error)
 			return nil, fmt.Errorf("error reading image manifest: %v", err)
 		}
 		image = c.ImageID
-		if img, err := store.GetImage(image); err == nil {
+		if img, err4 := store.GetImage(image); err4 == nil {
 			if len(img.Names) > 0 {
 				imageName = img.Names[0]
 			}
 		}
 	}
 
-	name = options.Container
+	name := options.Container
 
 	builder := &Builder{
 		store:       store,

pull.go

@@ -69,9 +69,5 @@ func pullImage(store storage.Store, options BuilderOptions, sc *types.SystemCont
 
 	names := append(destImage.Names, options.FromImage, name)
 	err = store.SetNames(destImage.ID, names)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return err
 }

run.go

@@ -24,8 +24,14 @@ const (
 )
 
 const (
+	// DefaultTerminal indicates that this Run invocation should be
+	// connected to a pseudoterminal if we're connected to a terminal.
 	DefaultTerminal = iota
+	// WithoutTerminal indicates that this Run invocation should NOT be
+	// connected to a pseudoterminal.
 	WithoutTerminal
+	// WithTerminal indicates that this Run invocation should be connected
+	// to a pseudoterminal.
 	WithTerminal
 )
 
@@ -59,10 +65,6 @@ type RunOptions struct {
 	Terminal int
 }
 
-func getExportOptions() generate.ExportOptions {
-	return generate.ExportOptions{}
-}
-
 // Run runs the specified command in the container's root filesystem.
 func (b *Builder) Run(command []string, options RunOptions) error {
 	var user specs.User
@@ -137,7 +139,9 @@ func (b *Builder) Run(command []string, options RunOptions) error {
 		g.SetProcessTerminal(false)
 	}
 	if !options.NetworkDisabled {
-		g.RemoveLinuxNamespace("network")
+		if err = g.RemoveLinuxNamespace("network"); err != nil {
+			return fmt.Errorf("error removing network namespace for run: %v)", err)
+		}
 	}
 	if options.User != "" {
 		user, err = getUser(mountPoint, options.User)
@@ -153,7 +157,7 @@ func (b *Builder) Run(command []string, options RunOptions) error {
 	if spec.Process.Cwd == "" {
 		spec.Process.Cwd = DefaultWorkingDir
 	}
-	if err := os.MkdirAll(filepath.Join(mountPoint, b.Workdir), 0755); err != nil {
+	if err = os.MkdirAll(filepath.Join(mountPoint, b.Workdir), 0755); err != nil {
 		return fmt.Errorf("error ensuring working directory %q exists: %v)", b.Workdir, err)
 	}
 	mounts := options.Mounts