unify git parsing logic

pkg/api/describe/describer.go

@@ -25,9 +25,11 @@ func Config(client docker.Client, config *api.Config) string {
 		}
 		describeBuilderImage(client, config, config.BuilderImage, out)
 		describeRuntimeImage(config, out)
-		fmt.Fprintf(out, "Source:\t%s\n", config.Source)
-		if len(config.Ref) > 0 {
-			fmt.Fprintf(out, "Source Ref:\t%s\n", config.Ref)
+		sourceNoFragment := *config.Source
+		sourceNoFragment.Fragment = ""
+		fmt.Fprintf(out, "Source:\t%s\n", sourceNoFragment.String())
+		if len(config.Source.Fragment) > 0 {
+			fmt.Fprintf(out, "Source Ref:\t%s\n", config.Source.Fragment)
 		}
 		if len(config.ContextDir) > 0 {
 			fmt.Fprintf(out, "Context Directory:\t%s\n", config.ContextDir)

pkg/api/types.go

@@ -113,10 +113,7 @@ type Config struct {
 	IgnoreSubmodules bool
 
 	// Source URL describing the location of sources used to build the result image.
-	Source string
-
-	// Ref is a tag/branch to be used for build.
-	Ref string
+	Source *url.URL
 
 	// Tag is a result image tag name.
 	Tag string

pkg/api/validation/validation_test.go

@@ -1,20 +1,29 @@
 package validation
 
 import (
+	"net/url"
 	"reflect"
 	"testing"
 
 	"github.com/openshift/source-to-image/pkg/api"
 )
 
+func mustParse(s string) *url.URL {
+	u, err := url.Parse(s)
+	if err != nil {
+		panic(err)
+	}
+	return u
+}
+
 func TestValidation(t *testing.T) {
 	testCases := []struct {
 		value    *api.Config
 		expected []Error
 	}{
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				BuilderPullPolicy: api.DefaultBuilderPullPolicy,
@@ -23,7 +32,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				DockerNetworkMode: "foobar",
@@ -33,7 +42,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				DockerNetworkMode: api.NewDockerNetworkModeContainer("8d873e496bc3e80a1cb22e67f7de7be5b0633e27916b1144978d1419c0abfcdb"),
@@ -43,7 +52,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "",
+				Source:            nil,
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				DockerNetworkMode: api.NewDockerNetworkModeContainer("8d873e496bc3e80a1cb22e67f7de7be5b0633e27916b1144978d1419c0abfcdb"),
@@ -53,7 +62,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				BuilderPullPolicy: api.DefaultBuilderPullPolicy,
@@ -63,7 +72,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				BuilderPullPolicy: api.DefaultBuilderPullPolicy,
@@ -73,7 +82,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				BuilderPullPolicy: api.DefaultBuilderPullPolicy,
@@ -83,7 +92,7 @@ func TestValidation(t *testing.T) {
 		},
 		{
 			&api.Config{
-				Source:            "http://github.com/openshift/source",
+				Source:            mustParse("http://github.com/openshift/source"),
 				BuilderImage:      "openshift/builder",
 				DockerConfig:      &api.DockerConfig{Endpoint: "/var/run/docker.socket"},
 				BuilderPullPolicy: api.DefaultBuilderPullPolicy,

pkg/build/config.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/openshift/source-to-image/pkg/api"
 	"github.com/openshift/source-to-image/pkg/docker"
+	"github.com/openshift/source-to-image/pkg/scm/git"
 )
 
 // GenerateConfigFromLabels generates the S2I Config struct from the Docker
@@ -41,13 +42,17 @@ func GenerateConfigFromLabels(config *api.Config, metadata *docker.PullResult) e
 	}
 
 	if repo, ok := labels[api.DefaultNamespace+"build.source-location"]; ok {
-		config.Source = repo
+		var err error
+		config.Source, err = git.ParseRepository(repo)
+		if err != nil {
+			return err
+		}
 	} else {
 		return fmt.Errorf("required label %q not found in image", api.DefaultNamespace+"source-location")
 	}
 
 	config.ContextDir = labels[api.DefaultNamespace+"build.source-context-dir"]
-	config.Ref = labels[api.DefaultNamespace+"build.commit.ref"]
+	config.Source.Fragment = labels[api.DefaultNamespace+"build.commit.ref"]
 
 	return nil
 }

pkg/build/strategies/sti/sti.go

@@ -346,7 +346,7 @@ func (builder *STI) Prepare(config *api.Config) error {
 	}
 
 	// fetch sources, for their .s2i/bin might contain s2i scripts
-	if len(config.Source) > 0 {
+	if config.Source != nil {
 		if builder.sourceInfo, err = builder.source.Download(config); err != nil {
 			builder.result.BuildInfo.FailureReason = utilstatus.NewFailureReason(
 				utilstatus.ReasonFetchSourceFailed,

pkg/build/strategies/sti/sti_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/url"
 	"path/filepath"
 	"reflect"
 	"regexp/syntax"
@@ -145,7 +146,7 @@ func (f *FakeDockerBuild) Build(*api.Config) (*api.Result, error) {
 
 func TestDefaultSource(t *testing.T) {
 	config := &api.Config{
-		Source:       "file://.",
+		Source:       &url.URL{Scheme: "file", Path: "."},
 		DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"},
 	}
 	client, err := docker.NewEngineAPIClient(config.DockerConfig)
@@ -156,7 +157,7 @@ func TestDefaultSource(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if config.Source == "" {
+	if config.Source == nil {
 		t.Errorf("Config.Source not set: %v", config.Source)
 	}
 	if _, ok := sti.source.(*file.File); !ok || sti.source == nil {
@@ -166,7 +167,7 @@ func TestDefaultSource(t *testing.T) {
 
 func TestEmptySource(t *testing.T) {
 	config := &api.Config{
-		Source:       "",
+		Source:       nil,
 		DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"},
 	}
 	client, err := docker.NewEngineAPIClient(config.DockerConfig)
@@ -177,7 +178,7 @@ func TestEmptySource(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if config.Source != "" {
+	if config.Source != nil {
 		t.Errorf("Config.Source unexpectantly changed: %v", config.Source)
 	}
 	if _, ok := sti.source.(*empty.Noop); !ok || sti.source == nil {
@@ -585,10 +586,10 @@ func TestFetchSource(t *testing.T) {
 		gh := bh.git.(*test.FakeGit)
 
 		bh.config.WorkingDir = "/working-dir"
+		bh.config.Source = &url.URL{Path: "a-repo-source"}
 		if ft.refSpecified {
-			bh.config.Ref = "a-branch"
+			bh.config.Source.Fragment = "a-branch"
 		}
-		bh.config.Source = "a-repo-source"
 
 		expectedTargetDir := "/working-dir/upload/src"
 		_, e := bh.source.Download(bh.config)

pkg/cmd/cli/cmd/build.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 
+	"github.com/openshift/source-to-image/pkg/scm/git"
 	utilglog "github.com/openshift/source-to-image/pkg/util/glog"
 	"github.com/spf13/cobra"
 
@@ -27,6 +28,7 @@ var glog = utilglog.StderrLog
 
 // NewCmdBuild implements the S2I cli build command.
 func NewCmdBuild(cfg *api.Config) *cobra.Command {
+	var ref string
 	useConfig := false
 	oldScriptsFlag := ""
 	oldDestination := ""
@@ -52,7 +54,13 @@ $ s2i build . centos/ruby-22-centos7 hello-world-app
 
 			// If user specifies the arguments, then we override the stored ones
 			if len(args) >= 2 {
-				cfg.Source = args[0]
+				var err error
+				cfg.Source, err = git.ParseRepository(args[0])
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "ERROR: Couldn't parse argument %s\n", args[0])
+					return
+				}
+				cfg.Source.Fragment = ref
 				cfg.BuilderImage = args[1]
 				if len(args) >= 3 {
 					cfg.Tag = args[2]
@@ -162,7 +170,7 @@ $ s2i build . centos/ruby-22-centos7 hello-world-app
 	buildCmd.Flags().BoolVar(&(cfg.RunImage), "run", false, "Run resulting image as part of invocation of this command")
 	buildCmd.Flags().BoolVar(&(cfg.IgnoreSubmodules), "ignore-submodules", false, "Ignore all git submodules when cloning application repository")
 	buildCmd.Flags().VarP(&(cfg.Environment), "env", "e", "Specify an single environment variable in NAME=VALUE format")
-	buildCmd.Flags().StringVarP(&(cfg.Ref), "ref", "r", "", "Specify a ref to check-out")
+	buildCmd.Flags().StringVarP(&(ref), "ref", "r", "", "Specify a ref to check-out")
 	buildCmd.Flags().StringVarP(&(cfg.AssembleUser), "assemble-user", "", "", "Specify the user to run assemble with")
 	buildCmd.Flags().StringVarP(&(cfg.ContextDir), "context-dir", "", "", "Specify the sub-directory inside the repository with the application sources")
 	buildCmd.Flags().StringVarP(&(cfg.ExcludeRegExp), "exclude", "", tar.DefaultExclusionPattern.String(), "Regular expression for selecting files from the source tree to exclude from the build, where the default excludes the '.git' directory (see https://golang.org/pkg/regexp for syntax, but note that \"\" will be interpreted as allow all files and exclude no files)")

pkg/config/config.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/openshift/source-to-image/pkg/api"
+	"github.com/openshift/source-to-image/pkg/scm/git"
 	utilglog "github.com/openshift/source-to-image/pkg/util/glog"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
@@ -31,7 +32,7 @@ type Config struct {
 func Save(config *api.Config, cmd *cobra.Command) {
 	c := Config{
 		BuilderImage: config.BuilderImage,
-		Source:       config.Source,
+		Source:       config.Source.String(),
 		Tag:          config.Tag,
 		Flags:        make(map[string]string),
 	}
@@ -74,7 +75,11 @@ func Restore(config *api.Config, cmd *cobra.Command) {
 	}
 
 	config.BuilderImage = c.BuilderImage
-	config.Source = c.Source
+	config.Source, err = git.ParseRepository(c.Source)
+	if err != nil {
+		glog.V(1).Infof("Unable to restore %s: %v", DefaultConfigPath, err)
+		return
+	}
 	config.Tag = c.Tag
 
 	envOverride := false

pkg/scm/file/download.go

@@ -2,7 +2,6 @@ package file
 
 import (
 	"path/filepath"
-	"strings"
 
 	"github.com/openshift/source-to-image/pkg/api"
 	"github.com/openshift/source-to-image/pkg/util"
@@ -20,11 +19,10 @@ type File struct {
 // Download copies sources from a local directory into the working directory
 func (f *File) Download(config *api.Config) (*api.SourceInfo, error) {
 	config.WorkingSourceDir = filepath.Join(config.WorkingDir, api.Source)
-	source := strings.TrimPrefix(config.Source, "file://")
 
-	copySrc := source
+	copySrc := config.Source.Path
 	if len(config.ContextDir) > 0 {
-		copySrc = filepath.Join(source, config.ContextDir)
+		copySrc = filepath.Join(config.Source.Path, config.ContextDir)
 	}
 
 	glog.V(1).Infof("Copying sources from %q to %q", copySrc, config.WorkingSourceDir)
@@ -36,7 +34,7 @@ func (f *File) Download(config *api.Config) (*api.SourceInfo, error) {
 	}
 
 	return &api.SourceInfo{
-		Location:   source,
+		Location:   config.Source.Path,
 		ContextDir: config.ContextDir,
 	}, nil
 }

pkg/scm/file/download_test.go

@@ -1,6 +1,7 @@
 package file
 
 import (
+	"net/url"
 	"path/filepath"
 	"testing"
 
@@ -13,7 +14,7 @@ func TestDownload(t *testing.T) {
 	f := &File{fs}
 
 	config := &api.Config{
-		Source: "file:///foo",
+		Source: &url.URL{Scheme: "file", Path: "/foo"},
 	}
 	info, err := f.Download(config)
 	if err != nil {
@@ -22,7 +23,7 @@ func TestDownload(t *testing.T) {
 	if fs.CopySource != "/foo" {
 		t.Errorf("Unexpected fs.CopySource %s", fs.CopySource)
 	}
-	if info.Location != config.Source[7:] || info.ContextDir != config.ContextDir {
+	if info.Location != config.Source.Path || info.ContextDir != config.ContextDir {
 		t.Errorf("Unexpected info")
 	}
 }
@@ -32,7 +33,7 @@ func TestDownloadWithContext(t *testing.T) {
 	f := &File{fs}
 
 	config := &api.Config{
-		Source:     "file:///foo",
+		Source:     &url.URL{Scheme: "file", Path: "/foo"},
 		ContextDir: "bar",
 	}
 	info, err := f.Download(config)
@@ -42,7 +43,7 @@ func TestDownloadWithContext(t *testing.T) {
 	if filepath.ToSlash(fs.CopySource) != "/foo/bar" {
 		t.Errorf("Unexpected fs.CopySource %s", fs.CopySource)
 	}
-	if info.Location != config.Source[7:] || info.ContextDir != config.ContextDir {
+	if info.Location != config.Source.Path || info.ContextDir != config.ContextDir {
 		t.Errorf("Unexpected info")
 	}
 }