Allow S2I behavior to be overriden by caller

pkg/build/interfaces.go

@@ -62,3 +62,9 @@ type SourceHandler interface {
 type LayeredDockerBuilder interface {
 	Builder
 }
+
+// Overrides are interfaces that may be passed into build strategies to
+// alter the behavior of a strategy.
+type Overrides struct {
+	Downloader Downloader
+}

pkg/build/strategies/layered/layered.go

@@ -27,7 +27,7 @@ type Layered struct {
 	scripts build.ScriptsHandler
 }
 
-func New(config *api.Config, scripts build.ScriptsHandler) (*Layered, error) {
+func New(config *api.Config, scripts build.ScriptsHandler, overrides build.Overrides) (*Layered, error) {
 	d, err := docker.New(config.DockerConfig, config.PullAuthentication)
 	if err != nil {
 		return nil, err
@@ -82,6 +82,7 @@ func (b *Layered) CreateDockerfile(config *api.Config) error {
 	return nil
 }
 
+// TODO: this should stop generating a file, and instead stream the tar.
 func (b *Layered) SourceTar(config *api.Config) (io.ReadCloser, error) {
 	uploadDir := filepath.Join(config.WorkingDir, "upload")
 	tarFileName, err := b.tar.CreateTarFile(b.config.WorkingDir, uploadDir)

pkg/build/strategies/onbuild/onbuild.go

@@ -38,7 +38,7 @@ type onBuildSourceHandler struct {
 }
 
 // New returns a new instance of OnBuild builder
-func New(config *api.Config) (*OnBuild, error) {
+func New(config *api.Config, overrides build.Overrides) (*OnBuild, error) {
 	dockerHandler, err := docker.New(config.DockerConfig, config.PullAuthentication)
 	if err != nil {
 		return nil, err
@@ -50,15 +50,25 @@ func New(config *api.Config) (*OnBuild, error) {
 		tar:    tar.New(),
 	}
 	// Use STI Prepare() and download the 'run' script optionally.
-	s, err := sti.New(config)
+	s, err := sti.New(config, overrides)
 	s.SetScripts([]string{}, []string{api.Assemble, api.Run})
-	downloader, sourceUrl, err := scm.DownloaderForSource(config.Source)
-	if err != nil {
-		return nil, err
+
+	downloader := overrides.Downloader
+	if downloader == nil {
+		d, sourceURL, err := scm.DownloaderForSource(config.Source)
+		if err != nil {
+			return nil, err
+		}
+		downloader = d
+		config.Source = sourceURL
+	}
+
+	b.source = onBuildSourceHandler{
+		Downloader: downloader,
+		Preparer:   s,
+		Ignorer:    &ignore.DockerIgnorer{},
 	}
-	config.Source = sourceUrl
 
-	b.source = onBuildSourceHandler{downloader, s, &ignore.DockerIgnorer{}}
 	b.garbage = &build.DefaultCleaner{b.fs, b.docker}
 	return b, nil
 }

pkg/build/strategies/sti/sti.go

@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 
 	"github.com/golang/glog"
 	"github.com/openshift/source-to-image/pkg/api"
@@ -66,7 +67,7 @@ type STI struct {
 // If the layeredBuilder parameter is specified, then the builder provided will
 // be used for the case that the base Docker image does not have 'tar' or 'bash'
 // installed.
-func New(req *api.Config) (*STI, error) {
+func New(req *api.Config, overrides build.Overrides) (*STI, error) {
 	docker, err := dockerpkg.New(req.DockerConfig, req.PullAuthentication)
 	if err != nil {
 		return nil, err
@@ -99,13 +100,18 @@ func New(req *api.Config) (*STI, error) {
 
 	// The sources are downloaded using the GIT downloader.
 	// TODO: Add more SCM in future.
-	b.source, req.Source, err = scm.DownloaderForSource(req.Source)
-	if err != nil {
-		return nil, err
+	b.source = overrides.Downloader
+	if b.source == nil {
+		downloader, sourceURL, err := scm.DownloaderForSource(req.Source)
+		if err != nil {
+			return nil, err
+		}
+		b.source = downloader
+		req.Source = sourceURL
 	}
 
 	b.garbage = &build.DefaultCleaner{b.fs, b.docker}
-	b.layered, err = layered.New(req, b)
+	b.layered, err = layered.New(req, b, overrides)
 
 	// Set interfaces
 	b.preparer = b
@@ -168,8 +174,10 @@ func (b *STI) Build(config *api.Config) (*api.Result, error) {
 // struct Build func leverages the STI struct Prepare func directly below
 func (b *STI) Prepare(config *api.Config) error {
 	var err error
-	if config.WorkingDir, err = b.fs.CreateWorkingDirectory(); err != nil {
-		return err
+	if len(config.WorkingDir) == 0 {
+		if config.WorkingDir, err = b.fs.CreateWorkingDirectory(); err != nil {
+			return err
+		}
 	}
 
 	b.result = &api.Result{
@@ -374,18 +382,6 @@ func (b *STI) Execute(command string, config *api.Config) error {
 
 	buildEnv := append(scripts.ConvertEnvironment(env), b.generateConfigEnv()...)
 
-	uploadDir := filepath.Join(config.WorkingDir, "upload")
-	tarFileName, err := b.tar.CreateTarFile(config.WorkingDir, uploadDir)
-	if err != nil {
-		return err
-	}
-
-	tarFile, err := b.fs.Open(tarFileName)
-	if err != nil {
-		return err
-	}
-	defer tarFile.Close()
-
 	errOutput := ""
 	outReader, outWriter := io.Pipe()
 	errReader, errWriter := io.Pipe()
@@ -410,8 +406,28 @@ func (b *STI) Execute(command string, config *api.Config) error {
 		Env:             buildEnv,
 		PostExec:        b.postExecutor,
 	}
+
 	if !config.LayeredBuild {
-		opts.Stdin = tarFile
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		uploadDir := filepath.Join(config.WorkingDir, "upload")
+
+		// TODO: be able to pass a stream directly to the Docker build to avoid the double temp hit
+		r, w := io.Pipe()
+		go func() {
+			var err error
+			defer func() {
+				w.CloseWithError(err)
+				if r := recover(); r != nil {
+					glog.Errorf("recovered panic: %#v", r)
+				}
+				wg.Done()
+			}()
+			err = b.tar.CreateTarStream(uploadDir, false, w)
+		}()
+
+		opts.Stdin = r
+		defer wg.Wait()
 	}
 
 	go func(reader io.Reader) {

pkg/build/strategies/sti/sti_test.go

@@ -3,13 +3,15 @@ package sti
 import (
 	"errors"
 	"fmt"
+	"io"
 	"reflect"
 	"testing"
 
 	"github.com/openshift/source-to-image/pkg/api"
 	"github.com/openshift/source-to-image/pkg/build"
 	stierr "github.com/openshift/source-to-image/pkg/errors"
 	"github.com/openshift/source-to-image/pkg/ignore"
+	"github.com/openshift/source-to-image/pkg/scm/file"
 	"github.com/openshift/source-to-image/pkg/scm/git"
 	"github.com/openshift/source-to-image/pkg/test"
 )
@@ -24,6 +26,7 @@ type FakeSTI struct {
 	ExistsError            error
 	BuildRequest           *api.Config
 	BuildResult            *api.Result
+	DownloadError          error
 	SaveArtifactsCalled    bool
 	SaveArtifactsError     error
 	FetchSourceCalled      bool
@@ -103,8 +106,8 @@ func (f *FakeSTI) fetchSource() error {
 	return f.FetchSourceError
 }
 
-func (f *FakeSTI) Download(*api.Config) error {
-	return nil
+func (f *FakeSTI) Download(*api.Config) (*api.SourceInfo, error) {
+	return nil, f.DownloadError
 }
 
 func (f *FakeSTI) Execute(command string, r *api.Config) error {
@@ -131,6 +134,41 @@ func (f *FakeDockerBuild) Build(*api.Config) (*api.Result, error) {
 	return nil, f.LayeredBuildError
 }
 
+func TestDefaultSource(t *testing.T) {
+	config := &api.Config{
+		Source:       "file://.",
+		DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"},
+	}
+	sti, err := New(config, build.Overrides{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if config.Source == "" {
+		t.Errorf("Config.Source not set: %v", config.Source)
+	}
+	if _, ok := sti.source.(*file.File); !ok || sti.source == nil {
+		t.Errorf("Source interface not set: %#v", sti.source)
+	}
+}
+
+func TestOverrides(t *testing.T) {
+	fd := &FakeSTI{}
+	sti, err := New(
+		&api.Config{
+			DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"},
+		},
+		build.Overrides{
+			Downloader: fd,
+		},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if sti.source != fd {
+		t.Errorf("Override of downloader not set: %#v", sti)
+	}
+}
+
 func TestBuild(t *testing.T) {
 	incrementalTest := []bool{false, true}
 	for _, incremental := range incrementalTest {
@@ -681,7 +719,8 @@ func TestExecuteOK(t *testing.T) {
 	if err != nil {
 		t.Errorf("Unexpected error returned: %v", err)
 	}
-	if th.CreateTarBase != "/working-dir" {
+	th = rh.tar.(*test.FakeTar).Copy()
+	if th.CreateTarBase != "" {
 		t.Errorf("Unexpected tar base directory: %s", th.CreateTarBase)
 	}
 	if th.CreateTarDir != "/working-dir/upload" {
@@ -691,26 +730,26 @@ func TestExecuteOK(t *testing.T) {
 	if !ok {
 		t.Fatalf("Unable to convert %v to FakeFilesystem", rh.fs)
 	}
-	if fh.OpenFile != "/working-dir/test.tar" {
-		t.Errorf("Unexpected file opened: %s", fh.OpenFile)
+	if fh.OpenFile != "" {
+		t.Fatalf("Unexpected file opened: %s", fh.OpenFile)
 	}
-	if !fh.OpenFileResult.CloseCalled {
-		t.Errorf("Tar file was not closed.")
+	if fh.OpenFileResult != nil {
+		t.Errorf("Tar file was opened.")
 	}
 	ro := fd.RunContainerOpts
 
 	if ro.Image != rh.config.BuilderImage {
 		t.Errorf("Unexpected Image passed to RunContainer")
 	}
-	if ro.Stdin != fh.OpenFileResult {
-		t.Errorf("Unexpected input stream: %#v", fd.RunContainerOpts.Stdin)
+	if _, ok := ro.Stdin.(*io.PipeReader); !ok {
+		t.Errorf("Unexpected input stream: %#v", ro.Stdin)
 	}
 	if !ro.PullImage {
 		t.Errorf("PullImage is not true for RunContainer")
 	}
 	if ro.Command != "test-command" {
 		t.Errorf("Unexpected command passed to RunContainer: %s",
-			fd.RunContainerOpts.Command)
+			ro.Command)
 	}
 	if pe.PostExecuteContainerID != "1234" {
 		t.Errorf("PostExecutor not called with expected ID: %s",
@@ -725,17 +764,15 @@ func TestExecuteErrorCreateTarFile(t *testing.T) {
 	rh := newFakeSTI(&FakeSTI{})
 	rh.tar.(*test.FakeTar).CreateTarError = errors.New("CreateTarError")
 	err := rh.Execute("test-command", rh.config)
-	if err == nil || err.Error() != "CreateTarError" {
+	if err != nil {
 		t.Errorf("An error was expected for CreateTarFile, but got different: %v", err)
 	}
-}
-
-func TestExecuteErrorOpenTarFile(t *testing.T) {
-	rh := newFakeSTI(&FakeSTI{})
-	rh.fs.(*test.FakeFileSystem).OpenError = errors.New("OpenTarError")
-	err := rh.Execute("test-command", rh.config)
-	if err == nil || err.Error() != "OpenTarError" {
-		t.Errorf("An error was expected for OpenTarFile, but got different: %v", err)
+	ro := rh.docker.(*test.FakeDocker).RunContainerOpts
+	if ro.Stdin == nil {
+		t.Fatalf("Stream not passed to Docker interface")
+	}
+	if _, err := ro.Stdin.Read(make([]byte, 5)); err == nil || err.Error() != "CreateTarError" {
+		t.Errorf("An error was expected for CreateTarFile, but got different: %#v", ro)
 	}
 }
 

pkg/build/strategies/sti/usage.go

@@ -21,7 +21,7 @@ type Usage struct {
 
 // NewUsage creates a new instance of the default Usage implementation
 func NewUsage(config *api.Config) (*Usage, error) {
-	b, err := New(config)
+	b, err := New(config, build.Overrides{})
 	if err != nil {
 		return nil, err
 	}

pkg/build/strategies/strategies.go

@@ -13,17 +13,24 @@ import (
 )
 
 // GetStrategy decides what build strategy will be used for the STI build.
+// TODO: deprecated, use Strategy() instead
 func GetStrategy(config *api.Config) (build.Builder, error) {
+	return Strategy(config, build.Overrides{})
+}
+
+// Strategy creates the appropriate build strategy for the provided config, using
+// the overrides provided. Not all strategies support all overrides.
+func Strategy(config *api.Config, overrides build.Overrides) (build.Builder, error) {
 	image, err := GetBuilderImage(config)
 	if err != nil {
 		return nil, err
 	}
 
 	if image.OnBuild {
-		return onbuild.New(config)
+		return onbuild.New(config, overrides)
 	}
 
-	return sti.New(config)
+	return sti.New(config, overrides)
 }
 
 // GetBuilderImage processes the config and performs operations necessary to make

pkg/tar/tar.go

@@ -36,7 +36,8 @@ type Tar interface {
 	CreateTarFile(base, dir string) (string, error)
 
 	// CreateTarStream creates a tar from the given directory
-	// and streams it to the given writer
+	// and streams it to the given writer.
+	// An error is returned if an error occurs during streaming.
 	CreateTarStream(dir string, includeDirInPath bool, writer io.Writer) error
 
 	// ExtractTarStream extracts files from a given tar stream.
@@ -87,6 +88,7 @@ func (t *stiTar) shouldExclude(path string) bool {
 // CreateTarStream creates a tar stream on the given writer from
 // the given directory while excluding files that match the given
 // exclusion pattern.
+// TODO: this should encapsulate the goroutine that generates the stream.
 func (t *stiTar) CreateTarStream(dir string, includeDirInPath bool, writer io.Writer) error {
 	tarWriter := tar.NewWriter(writer)
 	defer tarWriter.Close()
@@ -148,7 +150,7 @@ func (t *stiTar) writeTarHeader(tarWriter *tar.Writer, dir string, path string,
 		prefix = filepath.Dir(prefix)
 	}
 	header.Name = filepath.ToSlash(path[1+len(prefix):])
-	glog.V(3).Infof("Adding to tar: %s as %s", path, header.Name)
+	glog.V(5).Infof("Adding to tar: %s as %s", path, header.Name)
 	if err = tarWriter.WriteHeader(header); err != nil {
 		return err
 	}