diff --git a/.redhat-ci.sh b/.redhat-ci.sh index 4a1081f8e6e..9ef9ede70da 100755 --- a/.redhat-ci.sh +++ b/.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 diff --git a/.travis.yml b/.travis.yml index 85240c0a404..3895b3aba1f 100644 --- a/.travis.yml +++ b/.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 diff --git a/Makefile b/Makefile index 4653c734f60..406355c1205 100644 --- a/Makefile +++ b/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: diff --git a/add.go b/add.go index 9fad01583df..685dc72ca21 100644 --- a/add.go +++ b/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 diff --git a/buildah.go b/buildah.go index b7e8fbee1eb..1e23d63995e 100644 --- a/buildah.go +++ b/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) diff --git a/cmd/buildah/config.go b/cmd/buildah/config.go index 8afe1d89696..401d193ac74 100644 --- a/cmd/buildah/config.go +++ 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") { diff --git a/cmd/buildah/main.go b/cmd/buildah/main.go index 21092f82bed..f9f773197c0 100644 --- a/cmd/buildah/main.go +++ b/cmd/buildah/main.go @@ -64,7 +64,7 @@ func main() { if err != nil { return err } - store.Shutdown(false) + _, _ = store.Shutdown(false) } return nil } diff --git a/cmd/buildah/rmi.go b/cmd/buildah/rmi.go index 796d6766661..680a2e53eeb 100644 --- a/cmd/buildah/rmi.go +++ b/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) diff --git a/config.go b/config.go index 47239ea6ba1..d26335d7eda 100644 --- a/config.go +++ b/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 diff --git a/image.go b/image.go index 41c24ea429b..d9abb4062e8 100644 --- a/image.go +++ b/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) } diff --git a/import.go b/import.go index d03392178b3..cfc67bf7bcd 100644 --- a/import.go +++ b/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, diff --git a/pull.go b/pull.go index bc5211c66de..8317b31ce16 100644 --- a/pull.go +++ b/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 } diff --git a/run.go b/run.go index 1282d070c37..aca29795a6f 100644 --- a/run.go +++ b/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 diff --git a/tests/validate/git-validation.sh b/tests/validate/git-validation.sh new file mode 100755 index 00000000000..eea0605eb26 --- /dev/null +++ b/tests/validate/git-validation.sh @@ -0,0 +1,13 @@ +#!/bin/bash +export PATH=${GOPATH%%:*}/bin:${PATH} +if ! which git-validation > /dev/null 2> /dev/null ; then + echo git-validation is not installed. + echo Try installing it with \"make install.tools\" or with + echo \"go get -u github.com/vbatts/git-validation\" + exit 1 +fi +if test "$TRAVIS" != true ; then + #GITVALIDATE_EPOCH=":/git-validation epoch" + GITVALIDATE_EPOCH="b1bb73e01c9bf0b1b75e50a2d1947b14a8174eee" +fi +exec git-validation -q -run DCO,short-subject ${GITVALIDATE_EPOCH:+-range "${GITVALIDATE_EPOCH}""..${GITVALIDATE_TIP:-@}"} ${GITVALIDATE_FLAGS} diff --git a/tests/validate/gofmt.sh b/tests/validate/gofmt.sh new file mode 100755 index 00000000000..acfb2091284 --- /dev/null +++ b/tests/validate/gofmt.sh @@ -0,0 +1,7 @@ +#!/bin/bash +if test $(find -name "*.go" -not -path "./vendor/*" -print0 | xargs -n 1 -0 gofmt -s -l | wc -l) -ne 0 ; then + echo Error: source files are not formatted according to recommendations. Run \"gofmt -s -w\" on: + find -name "*.go" -not -path "./vendor/*" -print0 | xargs -n 1 -0 gofmt -s -l + exit 1 +fi +exit 0 diff --git a/tests/validate/gometalinter.sh b/tests/validate/gometalinter.sh new file mode 100755 index 00000000000..e7a9ec49cd5 --- /dev/null +++ b/tests/validate/gometalinter.sh @@ -0,0 +1,20 @@ +#!/bin/bash +export PATH=${GOPATH%%:*}/bin:${PATH} +if ! which gometalinter.v1 > /dev/null 2> /dev/null ; then + echo gometalinter.v1 is not installed. + echo Try installing it with \"make install.tools\" or with + echo \"go get -u gopkg.in/alecthomas/gometalinter.v1\" + echo \"gometalinter.v1 --install --vendored-linters\" + exit 1 +fi +exec gometalinter.v1 \ + --exclude='error return value not checked.*(Close|Log|Print).*\(errcheck\)$' \ + --exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \ + --exclude='duplicate of.*_test.go.*\(dupl\)$' \ + --exclude='vendor\/.*' \ + --disable=gotype \ + --disable=gas \ + --disable=aligncheck \ + --cyclo-over=40 \ + --deadline=120s \ + --tests "$@" diff --git a/user.go b/user.go index 455622737cd..0e7716bb720 100644 --- a/user.go +++ b/user.go @@ -27,7 +27,7 @@ func getUser(rootdir, userspec string) (specs.User, error) { if uerr == nil && groupspec == "" { // We parsed the user name as a number, and there's no group // component, so we need to look up the user's primary GID. - name := "" + var name string name, gid64, gerr = lookupGroupForUIDInContainer(rootdir, uid64) if gerr == nil { userspec = name diff --git a/user_unix_cgo.go b/user_unix_cgo.go index 4ac1561d348..33a00f52939 100644 --- a/user_unix_cgo.go +++ b/user_unix_cgo.go @@ -40,7 +40,7 @@ func fopenContainerFile(rootdir, filename string) (C.pFILE, error) { return nil, fmt.Errorf("lstat(%q): %v", ctrfile, err) } if st.Dev != lst.Dev || st.Ino != lst.Ino { - return nil, fmt.Errorf("%q is not a regular file") + return nil, fmt.Errorf("%q is not a regular file", ctrfile) } return f, nil }