From 7da3d3e97565e652a59fd81286f3956fd43e85a8 Mon Sep 17 00:00:00 2001 From: Jim Minter Date: Mon, 10 Jul 2017 13:55:16 +0100 Subject: [PATCH] git URL parsing rewrite --- hack/test-stirunimage.sh | 13 +- pkg/api/describe/describer.go | 3 - pkg/api/types.go | 5 +- pkg/api/validation/validation_test.go | 17 +- pkg/build/config.go | 11 +- pkg/build/strategies/onbuild/onbuild.go | 4 +- pkg/build/strategies/sti/sti.go | 7 +- pkg/build/strategies/sti/sti_test.go | 15 +- pkg/cmd/cli/cmd/build.go | 11 +- pkg/config/config.go | 11 +- pkg/scm/downloaders/file/download.go | 11 +- pkg/scm/downloaders/file/download_test.go | 9 +- pkg/scm/downloaders/git/clone.go | 34 +- pkg/scm/downloaders/git/clone_test.go | 3 +- pkg/scm/git/git.go | 441 +++------------------- pkg/scm/git/git_munge_test.go | 153 -------- pkg/scm/git/git_test.go | 153 ++------ pkg/scm/git/testhelpers.go | 50 ++- pkg/scm/git/url.go | 195 ++++++++++ pkg/scm/git/url_test.go | 295 +++++++++++++++ pkg/scm/scm.go | 58 ++- pkg/scm/scm_test.go | 63 ++-- pkg/test/git.go | 26 +- test/integration/integration_test.go | 11 +- 24 files changed, 743 insertions(+), 856 deletions(-) delete mode 100644 pkg/scm/git/git_munge_test.go create mode 100644 pkg/scm/git/url.go create mode 100644 pkg/scm/git/url_test.go diff --git a/hack/test-stirunimage.sh b/hack/test-stirunimage.sh index 187841e5d..a3a7eb88f 100755 --- a/hack/test-stirunimage.sh +++ b/hack/test-stirunimage.sh @@ -82,11 +82,6 @@ test_debug "s2i build with relative path without file://" s2i build cakephp-ex openshift/php-55-centos7 test --loglevel=5 &> "${WORK_DIR}/s2i-rel-noproto.log" check_result $? "${WORK_DIR}/s2i-rel-noproto.log" -test_debug "s2i build with relative path with file://" - -s2i build file://./cakephp-ex openshift/php-55-centos7 test --loglevel=5 &> "${WORK_DIR}/s2i-rel-proto.log" -check_result $? "${WORK_DIR}/s2i-rel-proto.log" - test_debug "s2i build with volume options" s2i build cakephp-ex openshift/php-55-centos7 test --volume "${WORK_DIR}:/home/:z" --loglevel=5 &> "${WORK_DIR}/s2i-volume-correct.log" check_result $? "${WORK_DIR}/s2i-volume-correct.log" @@ -95,7 +90,13 @@ popd test_debug "s2i build with absolute path with file://" -s2i build "file://${S2I_WORK_DIR}/cakephp-ex" openshift/php-55-centos7 test --loglevel=5 &> "${WORK_DIR}/s2i-abs-proto.log" +if [[ "$OSTYPE" == "cygwin" ]]; then + S2I_WORK_DIR_URL="file:///${S2I_WORK_DIR//\\//}/cakephp-ex" +else + S2I_WORK_DIR_URL="file://${S2I_WORK_DIR}/cakephp-ex" +fi + +s2i build "${S2I_WORK_DIR_URL}" openshift/php-55-centos7 test --loglevel=5 &> "${WORK_DIR}/s2i-abs-proto.log" check_result $? "${WORK_DIR}/s2i-abs-proto.log" test_debug "s2i build with absolute path without file://" diff --git a/pkg/api/describe/describer.go b/pkg/api/describe/describer.go index e51323bc7..275dc4a76 100644 --- a/pkg/api/describe/describer.go +++ b/pkg/api/describe/describer.go @@ -26,9 +26,6 @@ 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) - } if len(config.ContextDir) > 0 { fmt.Fprintf(out, "Context Directory:\t%s\n", config.ContextDir) } diff --git a/pkg/api/types.go b/pkg/api/types.go index 66672f3c8..c023ae8f4 100644 --- a/pkg/api/types.go +++ b/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 *git.URL // Tag is a result image tag name. Tag string diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 218bd9351..243e3763f 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/openshift/source-to-image/pkg/api" + "github.com/openshift/source-to-image/pkg/scm/git" ) func TestValidation(t *testing.T) { @@ -14,7 +15,7 @@ func TestValidation(t *testing.T) { }{ { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, BuilderPullPolicy: api.DefaultBuilderPullPolicy, @@ -23,7 +24,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, DockerNetworkMode: "foobar", @@ -33,7 +34,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, DockerNetworkMode: api.NewDockerNetworkModeContainer("8d873e496bc3e80a1cb22e67f7de7be5b0633e27916b1144978d1419c0abfcdb"), @@ -43,7 +44,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 +54,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, BuilderPullPolicy: api.DefaultBuilderPullPolicy, @@ -63,7 +64,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, BuilderPullPolicy: api.DefaultBuilderPullPolicy, @@ -73,7 +74,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, BuilderPullPolicy: api.DefaultBuilderPullPolicy, @@ -83,7 +84,7 @@ func TestValidation(t *testing.T) { }, { &api.Config{ - Source: "http://github.com/openshift/source", + Source: git.MustParse("http://github.com/openshift/source"), BuilderImage: "openshift/builder", DockerConfig: &api.DockerConfig{Endpoint: "/var/run/docker.socket"}, BuilderPullPolicy: api.DefaultBuilderPullPolicy, diff --git a/pkg/build/config.go b/pkg/build/config.go index 28ac98831..28f439646 100644 --- a/pkg/build/config.go +++ b/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 + source, err := git.Parse(repo) + if err != nil { + return fmt.Errorf("couldn't parse label %q value %s: %v", api.DefaultNamespace+"build.source-location", repo, err) + } + config.Source = source } else { - return fmt.Errorf("required label %q not found in image", api.DefaultNamespace+"source-location") + return fmt.Errorf("required label %q not found in image", api.DefaultNamespace+"build.source-location") } config.ContextDir = labels[api.DefaultNamespace+"build.source-context-dir"] - config.Ref = labels[api.DefaultNamespace+"build.commit.ref"] + config.Source.URL.Fragment = labels[api.DefaultNamespace+"build.commit.ref"] return nil } diff --git a/pkg/build/strategies/onbuild/onbuild.go b/pkg/build/strategies/onbuild/onbuild.go index 541424617..083975f84 100644 --- a/pkg/build/strategies/onbuild/onbuild.go +++ b/pkg/build/strategies/onbuild/onbuild.go @@ -56,12 +56,10 @@ func New(client docker.Client, config *api.Config, fs fs.FileSystem, overrides b downloader := overrides.Downloader if downloader == nil { - d, sourceURL, err := scm.DownloaderForSource(builder.fs, config.Source, config.ForceCopy) + downloader, err = scm.DownloaderForSource(builder.fs, config.Source, config.ForceCopy) if err != nil { return nil, err } - downloader = d - config.Source = sourceURL } builder.source = onBuildSourceHandler{ diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index f5bea4df1..3c20c2407 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/pkg/build/strategies/sti/sti.go @@ -150,14 +150,11 @@ func New(client dockerpkg.Client, config *api.Config, fs fs.FileSystem, override // which would lead to replacing this quick short circuit (so this change is tactical) builder.source = overrides.Downloader if builder.source == nil && !config.Usage { - var downloader build.Downloader - var sourceURL string - downloader, sourceURL, err = scm.DownloaderForSource(builder.fs, config.Source, config.ForceCopy) + downloader, err := scm.DownloaderForSource(builder.fs, config.Source, config.ForceCopy) if err != nil { return nil, err } builder.source = downloader - config.Source = sourceURL } builder.garbage = build.NewDefaultCleaner(builder.fs, builder.docker) @@ -351,7 +348,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, diff --git a/pkg/build/strategies/sti/sti_test.go b/pkg/build/strategies/sti/sti_test.go index 6cebffe54..91ac9004d 100644 --- a/pkg/build/strategies/sti/sti_test.go +++ b/pkg/build/strategies/sti/sti_test.go @@ -147,7 +147,7 @@ func (f *FakeDockerBuild) Build(*api.Config) (*api.Result, error) { func TestDefaultSource(t *testing.T) { config := &api.Config{ - Source: "file://.", + Source: git.MustParse("."), DockerConfig: &api.DockerConfig{Endpoint: "unix:///var/run/docker.sock"}, } client, err := docker.NewEngineAPIClient(config.DockerConfig) @@ -158,7 +158,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 { @@ -168,7 +168,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) @@ -179,7 +179,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 { @@ -587,18 +587,17 @@ func TestFetchSource(t *testing.T) { gh := bh.git.(*test.FakeGit) bh.config.WorkingDir = "/working-dir" - gh.ValidCloneSpecResult = true + bh.config.Source = git.MustParse("a-repo-source") if ft.refSpecified { - bh.config.Ref = "a-branch" + bh.config.Source.URL.Fragment = "a-branch" } - bh.config.Source = "a-repo-source" expectedTargetDir := "/working-dir/upload/src" _, e := bh.source.Download(bh.config) if e != nil { t.Errorf("Unexpected error %v [%d]", e, testNum) } - if gh.CloneSource != "a-repo-source" { + if gh.CloneSource.StringNoFragment() != "a-repo-source" { t.Errorf("Clone was not called with the expected source. Got %s, expected %s [%d]", gh.CloneSource, "a-source-repo-source", testNum) } if filepath.ToSlash(gh.CloneTarget) != expectedTargetDir { diff --git a/pkg/cmd/cli/cmd/build.go b/pkg/cmd/cli/cmd/build.go index cdb040281..96cd3f84a 100644 --- a/pkg/cmd/cli/cmd/build.go +++ b/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,12 @@ $ 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] + source, err := git.Parse(args[0]) + if err != nil { + fmt.Fprintf(os.Stderr, "ERROR: couldn't parse %q: %v\n", args[0], err) + } + cfg.Source = source + cfg.Source.URL.Fragment = ref cfg.BuilderImage = args[1] if len(args) >= 3 { cfg.Tag = args[2] @@ -162,7 +169,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)") diff --git a/pkg/config/config.go b/pkg/config/config.go index e5abcaabc..7de2a0ed0 100644 --- a/pkg/config/config.go +++ b/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), } @@ -73,8 +74,14 @@ func Restore(config *api.Config, cmd *cobra.Command) { return } + source, err := git.Parse(c.Source) + if err != nil { + glog.V(1).Infof("Unable to parse %s: %v", c.Source, err) + return + } + config.BuilderImage = c.BuilderImage - config.Source = c.Source + config.Source = source config.Tag = c.Tag envOverride := false diff --git a/pkg/scm/downloaders/file/download.go b/pkg/scm/downloaders/file/download.go index 63e803300..67f91c1d3 100644 --- a/pkg/scm/downloaders/file/download.go +++ b/pkg/scm/downloaders/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/scm/git" @@ -18,14 +17,14 @@ type File struct { fs.FileSystem } -// Download copies sources from a local directory into the working directory +// Download copies sources from a local directory into the working directory. +// Caller guarantees that config.Source.IsLocal() is true. func (f *File) Download(config *api.Config) (*git.SourceInfo, error) { config.WorkingSourceDir = filepath.Join(config.WorkingDir, api.Source) - source := strings.TrimPrefix(config.Source, "file://") - copySrc := source + copySrc := config.Source.LocalPath() if len(config.ContextDir) > 0 { - copySrc = filepath.Join(source, config.ContextDir) + copySrc = filepath.Join(copySrc, config.ContextDir) } glog.V(1).Infof("Copying sources from %q to %q", copySrc, config.WorkingSourceDir) @@ -37,7 +36,7 @@ func (f *File) Download(config *api.Config) (*git.SourceInfo, error) { } return &git.SourceInfo{ - Location: source, + Location: config.Source.LocalPath(), ContextDir: config.ContextDir, }, nil } diff --git a/pkg/scm/downloaders/file/download_test.go b/pkg/scm/downloaders/file/download_test.go index 0c06eedf7..7e7641aa4 100644 --- a/pkg/scm/downloaders/file/download_test.go +++ b/pkg/scm/downloaders/file/download_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/openshift/source-to-image/pkg/api" + "github.com/openshift/source-to-image/pkg/scm/git" testfs "github.com/openshift/source-to-image/pkg/test/fs" ) @@ -13,7 +14,7 @@ func TestDownload(t *testing.T) { f := &File{fs} config := &api.Config{ - Source: "file:///foo", + Source: git.MustParse("/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.URL.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: git.MustParse("/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.URL.Path || info.ContextDir != config.ContextDir { t.Errorf("Unexpected info") } } diff --git a/pkg/scm/downloaders/git/clone.go b/pkg/scm/downloaders/git/clone.go index 06bd89df2..098a41cac 100644 --- a/pkg/scm/downloaders/git/clone.go +++ b/pkg/scm/downloaders/git/clone.go @@ -1,16 +1,13 @@ package git import ( - "fmt" "os" "path/filepath" "runtime" - "strings" "github.com/golang/glog" "github.com/openshift/source-to-image/pkg/api" "github.com/openshift/source-to-image/pkg/scm/git" - "github.com/openshift/source-to-image/pkg/util/cygpath" "github.com/openshift/source-to-image/pkg/util/fs" ) @@ -26,32 +23,9 @@ func (c *Clone) Download(config *api.Config) (*git.SourceInfo, error) { targetSourceDir := filepath.Join(config.WorkingDir, api.Source) config.WorkingSourceDir = targetSourceDir - ok, err := c.ValidCloneSpec(config.Source) - if err != nil { - return nil, err - } - if !ok { - glog.Errorf("Clone.Download was passed an invalid source %s", config.Source) - return nil, fmt.Errorf("invalid source %s", config.Source) - } - - ref := "HEAD" - if config.Ref != "" { - ref = config.Ref - } - - if strings.HasPrefix(config.Source, "file://") { - s := strings.TrimPrefix(config.Source, "file://") - - if cygpath.UsingCygwinGit { - var err error - s, err = cygpath.ToSlashCygwin(s) - if err != nil { - glog.V(0).Infof("error: Cygwin path conversion failed: %v", err) - return nil, err - } - } - config.Source = "file://" + s + ref := config.Source.URL.Fragment + if ref == "" { + ref = "HEAD" } if len(config.ContextDir) > 0 { @@ -68,7 +42,7 @@ func (c *Clone) Download(config *api.Config) (*git.SourceInfo, error) { } cloneConfig := git.CloneConfig{Quiet: true} - err = c.Clone(config.Source, targetSourceDir, cloneConfig) + err := c.Clone(config.Source, targetSourceDir, cloneConfig) if err != nil { glog.V(0).Infof("error: git clone failed: %v", err) return nil, err diff --git a/pkg/scm/downloaders/git/clone_test.go b/pkg/scm/downloaders/git/clone_test.go index b9b5a3c5b..1139d24f6 100644 --- a/pkg/scm/downloaders/git/clone_test.go +++ b/pkg/scm/downloaders/git/clone_test.go @@ -18,9 +18,8 @@ func TestCloneWithContext(t *testing.T) { c := &Clone{gh, fs} fakeConfig := &api.Config{ - Source: "https://foo/bar.git", + Source: git.MustParse("https://foo/bar.git#ref1"), ContextDir: "subdir", - Ref: "ref1", IgnoreSubmodules: true, } info, err := c.Download(fakeConfig) diff --git a/pkg/scm/git/git.go b/pkg/scm/git/git.go index 0ea88cc99..1e29f5483 100644 --- a/pkg/scm/git/git.go +++ b/pkg/scm/git/git.go @@ -2,10 +2,8 @@ package git import ( "bufio" + "bytes" "fmt" - "io" - "io/ioutil" - "net/url" "os" "os/exec" "path/filepath" @@ -15,7 +13,6 @@ import ( log "github.com/golang/glog" - s2ierr "github.com/openshift/source-to-image/pkg/errors" "github.com/openshift/source-to-image/pkg/util/cmd" "github.com/openshift/source-to-image/pkg/util/cygpath" "github.com/openshift/source-to-image/pkg/util/fs" @@ -27,10 +24,7 @@ var lsTreeRegexp = regexp.MustCompile("([0-7]{6}) [^ ]+ [0-9a-f]{40}\t(.*)") // Git is an interface used by main STI code to extract/checkout git repositories type Git interface { - ValidCloneSpec(source string) (bool, error) - ValidCloneSpecRemoteOnly(source string) bool - MungeNoProtocolURL(source string, url *url.URL) error - Clone(source, target string, opts CloneConfig) error + Clone(source *URL, target string, opts CloneConfig) error Checkout(repo, ref string) error SubmoduleUpdate(repo string, init, recursive bool) error LsTree(repo, ref string, recursive bool) ([]os.FileInfo, error) @@ -50,37 +44,6 @@ type stiGit struct { cmd.CommandRunner } -// URLMods encapsulates potential changes to similarly named fields in the URL struct defined in golang -// when a protocol is not explicitly specified in a clone spec (which is possible for certain file:// and ssh:// permutations) -type URLMods struct { - Scheme string - User string - Host string - Path string - // Corresponds to Fragment in the URL struct, but we use Ref since URL Fragments are used and git commit refs - Ref string -} - -// FileProtoDetails encapsulates certain determinations from examining a given clone spec under the assumption -// that it is a local file protocol clone spec -type FileProtoDetails struct { - // Using the clone spec as a literal file path, does it actually exis - FileExists bool - // Use OS level file copy commands instead of the git binary - UseCopy bool - // Did the clone spec have the prefix of file:// - ProtoSpecified bool - // Was the text for a fragment/ref that follows the last # incorrect - BadRef bool -} - -var gitSSHURLUser = regexp.MustCompile(`^([\w\-_\.]+)$`) -var gitSSHURLIPv4 = regexp.MustCompile(`^([\w\.\-_]+)$`) // should cover textual hostnames and x.x.x.x -var gitSSHURLIPv6 = regexp.MustCompile(`^(\[[a-fA-F0-9\:]+\])$`) // should cover [hex:hex ... :hex] -var gitSSHURLPathRef = regexp.MustCompile(`^([\w\.\-_+\/\\]+)$`) - -var allowedSchemes = []string{"git", "http", "https", "file", "ssh"} - func cloneConfigToArgs(opts CloneConfig) []string { result := []string{} if opts.Quiet { @@ -102,279 +65,6 @@ func stringInSlice(s string, slice []string) bool { return false } -// ValidCloneSpec determines if the given string reference points to a valid git -// repository -func (h *stiGit) ValidCloneSpec(source string) (bool, error) { - details, _, err := ParseFile(h.FileSystem, source) - if err != nil { - return false, err - } - - if details.FileExists && !details.BadRef { - return true, nil - } - - return h.ValidCloneSpecRemoteOnly(source), nil -} - -// ValidCloneSpecRemoteOnly determines if the given string reference points to a valid remote git -// repository; valid local repository specs will result in a return of false -func (h *stiGit) ValidCloneSpecRemoteOnly(source string) bool { - _, err := ParseSSH(source) - if err == nil { - return true - } - - url, err := ParseURL(source) - if url != nil && err == nil && url.Scheme != "file" { - return true - } - - return false -} - -// MungeNoProtocolURL will take a URL returned from net/url.Parse and make -// corrections to the URL when no protocol is specified in the Scheme, where there -// are valid protocol-less git url spec formats that result in either file:// or ssh:// protocol usage; -// if an explicit protocol is already specified, then the -// URL is left unchaged and the method simply returns with no error reported, -// since the URL is -func (h *stiGit) MungeNoProtocolURL(source string, uri *url.URL) error { - if uri == nil { - return nil - } - - // only deal with protocol-less url's - if uri.Scheme != "" { - return nil - } - - details, mods, err := ParseFile(h.FileSystem, source) - if err != nil { - return err - } - - if details.BadRef { - return fmt.Errorf("bad reference following # in %s", source) - } - if !details.FileExists { - mods2, err := ParseSSH(source) - if err != nil { - glog.Errorf("ssh git clone spec error: %v", err) - return err - } - mods = mods2 - } - - // update the either file or ssh url accordingly - if mods != nil { - if len(mods.User) > 0 { - uri.User = url.User(mods.User) - } - if len(mods.Scheme) > 0 { - uri.Scheme = mods.Scheme - } - if len(mods.Host) > 0 { - uri.Host = mods.Host - } - if len(mods.Path) > 0 { - uri.Path = mods.Path - } - if len(mods.Ref) > 0 { - uri.Fragment = mods.Ref - } - } - return nil -} - -// ParseURL sees if the spec is properly handled by golang's URL parser, -// with a valid, explicit protocol specified, -// and deals with some of the known issues we have seen with the logic -func ParseURL(source string) (*url.URL, error) { - uri, err := url.Parse(source) - if err != nil { - return nil, err - } - - // to work around the url.Parse bug, deal with the fact it seemed to split on just : - if strings.Index(source, "://") == -1 && uri.Scheme != "" { - uri.Scheme = "" - return nil, fmt.Errorf("url source %s mistakingly interpreted as protocol %s by golang", source, uri.Scheme) - } - - // now see if an invalid protocol is specified - if !stringInSlice(uri.Scheme, allowedSchemes) { - return nil, fmt.Errorf("unsupported protocol specfied: %s", uri.Scheme) - } - - // have a valid protocol, return success - return uri, nil -} - -// makePathAbsolute mimic the path munging (make paths absolute to fix file:// with non-absolute paths) done when scm.go:DownloderForSource valided git file urls -func makePathAbsolute(source string) string { - glog.V(4).Infof("makePathAbsolute %s", source) - if !strings.HasPrefix(source, "/") { - absolutePath, err := filepath.Abs(source) - if err == nil { - glog.V(4).Infof("makePathAbsolute new path %s success", absolutePath) - return absolutePath - } - glog.V(4).Infof("makePathAbsolute source path %s err %v", source, err) - } - return source -} - -// ParseFile will see if the input string is a valid file location, where -// file names have a great deal of flexibility and can even match -// expect git clone spec syntax; it also provides details if the file:// -// proto was explicitly specified, if we should use OS copy vs. the git -// binary, and if a frag/ref has a bad format -func ParseFile(fs fs.FileSystem, source string) (*FileProtoDetails, *URLMods, error) { - // Checking to see if the user included a "file://" in the call - protoSpecified := false - if strings.HasPrefix(source, "file://") && len(source) > 7 { - protoSpecified = true - } - - refSpecified := false - path, ref := "", "" - if strings.LastIndex(source, "#") != -1 { - refSpecified = true - - segments := strings.SplitN(source, "#", 2) - path = segments[0] - ref = segments[1] - } else { - path = source - } - - // in each valid case, like the prior logic in scm.go did, we'll make the - // paths absolute and prepend file:// to the path which callers should - // switch to - _, err := fs.Stat(strings.TrimPrefix(path, "file://")) - if err == nil { - // Is there even a valid .git repository? - isValidGit, err := isValidGitRepository(fs, path) - hasGit := false - if isValidGit { - hasGit = hasGitBinary() - } - - if err != nil || !isValidGit || !hasGit { - details := &FileProtoDetails{ - UseCopy: true, - FileExists: true, - BadRef: false, - ProtoSpecified: protoSpecified, - } - mods := &URLMods{ - Scheme: "file", - Path: makePathAbsolute(strings.TrimPrefix(path, "file://")), - Ref: ref, - } - return details, mods, err - } - - // Check is the #ref is valid - badRef := refSpecified && !gitSSHURLPathRef.MatchString(ref) - - details := &FileProtoDetails{ - BadRef: badRef, - FileExists: true, - ProtoSpecified: protoSpecified, - // this value doesn't really matter, we should not proceed if the git ref is bad - // but let's fallback to "copy" mode if the ref is invalid. - UseCopy: badRef, - } - - mods := &URLMods{ - Scheme: "file", - Path: makePathAbsolute(strings.TrimPrefix(path, "file://")), - Ref: ref, - } - return details, mods, nil - } - - // File does not exist, return bad - details := &FileProtoDetails{ - UseCopy: false, - FileExists: false, - BadRef: false, - ProtoSpecified: protoSpecified, - } - return details, nil, nil -} - -// ParseSSH will see if the input string is a valid git clone spec -// which follows the rules for using the ssh protocol either with or without -// the ssh:// prefix -func ParseSSH(source string) (*URLMods, error) { - // if not ssh protcol, return bad - if strings.Index(source, "://") != -1 && !strings.HasPrefix(source, "ssh") { - return nil, fmt.Errorf("not ssh protocol: %s", source) - } - - lastColonIdx := strings.LastIndex(source, ":") - atSignPresent := strings.Index(source, "@") != -1 - if lastColonIdx != -1 { - host, path, user, ref := "", "", "", "" - - if atSignPresent { - segments := strings.SplitN(source, "@", 2) - // with index check above, can assume there are 2 segments - user, host = segments[0], segments[1] - - // bad user, return - if !gitSSHURLUser.MatchString(user) { - return nil, fmt.Errorf("invalid user name provided: %s from %s", user, source) - } - - // because of ipv6, need to redo last index of : - lastColonIdx = strings.LastIndex(host, ":") - if lastColonIdx != -1 { - path = host[lastColonIdx+1:] - host = host[0:lastColonIdx] - } else { - return nil, fmt.Errorf("invalid git ssh clone spec, the @ precedes the last: %s", source) - } - } else { - host = source[0:lastColonIdx] - path = source[lastColonIdx+1:] - } - - // bad host, either ipv6 or ipv4 - if !gitSSHURLIPv6.MatchString(host) && !gitSSHURLIPv4.MatchString(host) { - return nil, fmt.Errorf("invalid host provided: %s from %s", host, source) - } - - segments := strings.SplitN(path, "#", 2) - if len(segments) == 2 { - path, ref = segments[0], segments[1] - - // bad ref/frag - if !gitSSHURLPathRef.MatchString(ref) { - return nil, fmt.Errorf("invalid reference provided: %s from %s", ref, source) - } - } - - // bad path - if !gitSSHURLPathRef.MatchString(path) { - return nil, fmt.Errorf("invalid path provided: %s from %s", path, source) - } - - // return good - return &URLMods{ - Scheme: "ssh", - User: user, - Host: host, - Path: path, - Ref: ref, - }, nil - } - return nil, fmt.Errorf("unable to parse ssh git clone specification: %s", source) -} - // followGitSubmodule looks at a .git /file/ and tries to retrieve from inside // it the gitdir value, which is supposed to indicate the location of the // corresponding .git /directory/. Note: the gitdir value should point directly @@ -411,19 +101,29 @@ func followGitSubmodule(fs fs.FileSystem, gitPath string) (string, error) { return "", fmt.Errorf("unable to parse .git file %q", gitPath) } -// isValidGitRepository checks to see if there is a git repository in the -// directory and if the repository is valid -- i.e. it has remotes or commits -func isValidGitRepository(fs fs.FileSystem, dir string) (bool, error) { - gitPath := filepath.Join(strings.TrimPrefix(dir, "file://"), ".git") - - fi, err := fs.Stat(gitPath) +// IsLocalNonBareGitRepository returns true if dir hosts a non-bare git +// repository, i.e. it contains a ".git" subdirectory or file (submodule case). +func IsLocalNonBareGitRepository(fs fs.FileSystem, dir string) (bool, error) { + _, err := fs.Stat(filepath.Join(dir, ".git")) if os.IsNotExist(err) { - // The directory is not a git repo, no error return false, nil } if err != nil { return false, err } + return true, nil +} + +// LocalNonBareGitRepositoryIsEmpty returns true if the non-bare git repository +// at dir has no refs or objects. It also handles the case of dir being a +// checked out git submodule. +func LocalNonBareGitRepositoryIsEmpty(fs fs.FileSystem, dir string) (bool, error) { + gitPath := filepath.Join(dir, ".git") + + fi, err := fs.Stat(gitPath) + if err != nil { + return false, err + } if !fi.IsDir() { gitPath, err = followGitSubmodule(fs, gitPath) @@ -432,81 +132,72 @@ func isValidGitRepository(fs fs.FileSystem, dir string) (bool, error) { } } - // Search the content of the .git directory for content - directories := [2]string{ - filepath.Join(gitPath, "objects"), - filepath.Join(gitPath, "refs"), - } + // Search for any file in .git/{objects,refs}. We don't just search the + // base .git directory because of the hook samples that are normally + // generated with `git init` + found := false + for _, dir := range []string{"objects", "refs"} { + err := fs.Walk(filepath.Join(gitPath, dir), func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if !info.IsDir() { + found = true + } - // For the directories we search, if the git repo has been used, there will - // be some file. We don't just search the base git repository because of the - // hook samples that are normally generated with `git init` - isEmpty := true - for _, dir := range directories { - err := fs.Walk(dir, func(path string, info os.FileInfo, err error) error { - // If we find a file, the git directory is "not empty" - // We're looking for object blobs, and ref files - if info != nil && !info.IsDir() { - isEmpty = false + if found { return filepath.SkipDir } - return err + return nil }) - if err != nil && err != filepath.SkipDir { - // There is a .git, but we've encountered an error - return true, err + if err != nil { + return false, err } - if !isEmpty { - return true, nil + if found { + return false, nil } } - // Since we know there's a .git directory, but there is nothing in it, we - // throw an error - return true, s2ierr.NewEmptyGitRepositoryError(dir) + return true, nil } -// hasGitBinary checks if the 'git' binary is available on the system -func hasGitBinary() bool { +// HasGitBinary checks if the 'git' binary is available on the system +func HasGitBinary() bool { _, err := exec.LookPath("git") return err == nil } -// Clone clones a git repository to a specific target directory -func (h *stiGit) Clone(source, target string, c CloneConfig) error { +// Clone clones a git repository to a specific target directory. +func (h *stiGit) Clone(src *URL, target string, c CloneConfig) error { + var err error + + source := *src + if cygpath.UsingCygwinGit { - var err error + if source.IsLocal() { + source.URL.Path, err = cygpath.ToSlashCygwin(source.LocalPath()) + if err != nil { + return err + } + } + target, err = cygpath.ToSlashCygwin(target) if err != nil { return err } } - // NOTE, we don NOT pass in both stdout and stderr, because - // with running with --quiet, and no output heading to stdout, hangs were occurring with the coordination - // of underlying channel management in the Go layer when dealing with the Go Cmd wrapper around - // git, sending of stdout/stderr to the Pipes created here, and the glog routines sent to pipeToLog - // - // It was agreed that we wanted to keep --quiet and no stdout output ....leaving stderr only since - // --quiet does not suppress that anyway reduced the frequency of the hang, but it still occurred. - // the pipeToLog method has been left for now for historical purposes, but if this implemenetation - // of git clone holds, we'll want to delete that at some point. - cloneArgs := append([]string{"clone"}, cloneConfigToArgs(c)...) - cloneArgs = append(cloneArgs, []string{source, target}...) - errReader, errWriter, _ := os.Pipe() - opts := cmd.CommandOpts{Stderr: errWriter} - err := h.RunWithOptions(opts, "git", cloneArgs...) - errWriter.Close() + cloneArgs = append(cloneArgs, []string{source.StringNoFragment(), target}...) + stderr := &bytes.Buffer{} + opts := cmd.CommandOpts{Stderr: stderr} + err = h.RunWithOptions(opts, "git", cloneArgs...) if err != nil { - out, _ := ioutil.ReadAll(errReader) - // If we captured errors via stderr, print them out. - if len(out) > 0 { - glog.Errorf("Clone failed: source %s, target %s, with output %s", source, target, out) - } + glog.Errorf("Clone failed: source %s, target %s, with output %q", source, target, stderr.String()) return err } return nil @@ -635,17 +326,3 @@ func (h *stiGit) GetInfo(repo string) *SourceInfo { Message: git("--no-pager", "show", "-s", "--format=%<(80,trunc)%s", "HEAD"), } } - -func pipeToLog(reader io.Reader, log func(...interface{})) { - scanner := bufio.NewReader(reader) - for { - if text, err := scanner.ReadString('\n'); err != nil { - if err != io.ErrClosedPipe { - glog.Errorf("Error reading stdout, %v", err) - } - break - } else { - log(text) - } - } -} diff --git a/pkg/scm/git/git_munge_test.go b/pkg/scm/git/git_munge_test.go deleted file mode 100644 index 9021e590d..000000000 --- a/pkg/scm/git/git_munge_test.go +++ /dev/null @@ -1,153 +0,0 @@ -// +build !windows - -package git - -import ( - "net/url" - "os" - "testing" - - "github.com/openshift/source-to-image/pkg/util/cmd" - "github.com/openshift/source-to-image/pkg/util/fs" -) - -// NB: MungeNoProtocolURL is only called by OpenShift, running on a Linux -// system. It is unclear what its behaviour should be running on Windows / when -// passed Windows-style paths, therefore for now this test does not run on -// Windows builds. -// TODO: fix this. -func TestMungeNoProtocolURL(t *testing.T) { - gitLocalDir := CreateLocalGitDirectory(t) - defer os.RemoveAll(gitLocalDir) - - tests := map[string]url.URL{ - "git@github.com:user/repo.git": { - Scheme: "ssh", - Host: "github.com", - User: url.User("git"), - Path: "user/repo.git", - }, - "git://github.com/user/repo.git": { - Scheme: "git", - Host: "github.com", - Path: "/user/repo.git", - }, - "git://github.com/user/repo": { - Scheme: "git", - Host: "github.com", - Path: "/user/repo", - }, - "http://github.com/user/repo.git": { - Scheme: "http", - Host: "github.com", - Path: "/user/repo.git", - }, - "http://github.com/user/repo": { - Scheme: "http", - Host: "github.com", - Path: "/user/repo", - }, - "https://github.com/user/repo.git": { - Scheme: "https", - Host: "github.com", - Path: "/user/repo.git", - }, - "https://github.com/user/repo": { - Scheme: "https", - Host: "github.com", - Path: "/user/repo", - }, - "file://" + gitLocalDir: { - Scheme: "file", - Path: gitLocalDir, - }, - gitLocalDir: { - Scheme: "file", - Path: gitLocalDir, - }, - "git@192.168.122.1:repositories/authooks": { - Scheme: "ssh", - Host: "192.168.122.1", - User: url.User("git"), - Path: "repositories/authooks", - }, - "mbalazs@build.ulx.hu:/var/git/eap-ulx.git": { - Scheme: "ssh", - Host: "build.ulx.hu", - User: url.User("mbalazs"), - Path: "/var/git/eap-ulx.git", - }, - "ssh://git@[2001:db8::1]/repository.git": { - Scheme: "ssh", - Host: "[2001:db8::1]", - User: url.User("git"), - Path: "/repository.git", - }, - "ssh://git@mydomain.com:8080/foo/bar": { - Scheme: "ssh", - Host: "mydomain.com:8080", - User: url.User("git"), - Path: "/foo/bar", - }, - "git@[2001:db8::1]:repository.git": { - Scheme: "ssh", - Host: "[2001:db8::1]", - User: url.User("git"), - Path: "repository.git", - }, - "git@[2001:db8::1]:/repository.git": { - Scheme: "ssh", - Host: "[2001:db8::1]", - User: url.User("git"), - Path: "/repository.git", - }, - } - - gh := New(fs.NewFileSystem(), cmd.NewCommandRunner()) - - for scenario, test := range tests { - uri, err := url.Parse(scenario) - if err != nil { - t.Errorf("Could not parse url %q", scenario) - continue - } - - err = gh.MungeNoProtocolURL(scenario, uri) - if err != nil { - t.Errorf("MungeNoProtocolURL returned err: %v", err) - continue - } - - // reflect.DeepEqual was not dealing with url.URL correctly, have to check each field individually - // First, the easy string compares - equal := uri.Scheme == test.Scheme && uri.Opaque == test.Opaque && uri.Host == test.Host && uri.Path == test.Path && uri.RawQuery == test.RawQuery && uri.Fragment == test.Fragment - if equal { - // now deal with User, a Userinfo struct ptr - if uri.User == nil && test.User != nil { - equal = false - } else if uri.User != nil && test.User == nil { - equal = false - } else if uri.User != nil && test.User != nil { - equal = uri.User.String() == test.User.String() - } - } - if !equal { - t.Errorf(`URL string %q, field by field check: -- Scheme: got %v, ok? %v -- Opaque: got %v, ok? %v -- Host: got %v, ok? %v -- Path: got %v, ok? %v -- RawQuery: got %v, ok? %v -- Fragment: got %v, ok? %v -- User: got %v`, - scenario, - uri.Scheme, uri.Scheme == test.Scheme, - uri.Opaque, uri.Opaque == test.Opaque, - uri.Host, uri.Host == test.Host, - uri.Path, uri.Path == test.Path, - uri.RawQuery, uri.RawQuery == test.RawQuery, - uri.Fragment, uri.Fragment == test.Fragment, - uri.User) - } - } -} diff --git a/pkg/scm/git/git_test.go b/pkg/scm/git/git_test.go index ab4dd4ece..46e90604c 100644 --- a/pkg/scm/git/git_test.go +++ b/pkg/scm/git/git_test.go @@ -7,151 +7,68 @@ import ( "reflect" "testing" - s2ierr "github.com/openshift/source-to-image/pkg/errors" testcmd "github.com/openshift/source-to-image/pkg/test/cmd" testfs "github.com/openshift/source-to-image/pkg/test/fs" - "github.com/openshift/source-to-image/pkg/util/cmd" "github.com/openshift/source-to-image/pkg/util/fs" ) func TestIsValidGitRepository(t *testing.T) { fs := fs.NewFileSystem() - d := CreateLocalGitDirectory(t) + // a local git repo with a commit + d, err := CreateLocalGitDirectory() + if err != nil { + t.Fatal(err) + } defer os.RemoveAll(d) - // We have a .git that is populated - // Should return true with no error - ok, err := isValidGitRepository(fs, d) - if !ok { - t.Errorf("The %q directory is git repository", d) + ok, err := IsLocalNonBareGitRepository(fs, d) + if !ok || err != nil { + t.Errorf("IsLocalNonBareGitRepository returned %v, %v", ok, err) + } + empty, err := LocalNonBareGitRepositoryIsEmpty(fs, d) + if empty || err != nil { + t.Errorf("LocalNonBareGitRepositoryIsEmpty returned %v, %v", ok, err) } + // a local git repo with no commit + d, err = CreateEmptyLocalGitDirectory() if err != nil { - t.Errorf("isValidGitRepository returned an unexpected error: %q", err.Error()) + t.Fatal(err) } - - d = CreateEmptyLocalGitDirectory(t) defer os.RemoveAll(d) - // There are no tracking objects in the .git repository - // Should return true with an EmptyGitRepositoryError - ok, err = isValidGitRepository(fs, d) - if !ok { - t.Errorf("The %q directory is a git repository, but is empty", d) + ok, err = IsLocalNonBareGitRepository(fs, d) + if !ok || err != nil { + t.Errorf("IsLocalNonBareGitRepository returned %v, %v", ok, err) } - - if err != nil { - var e s2ierr.Error - if e, ok = err.(s2ierr.Error); !ok || e.ErrorCode != s2ierr.EmptyGitRepositoryError { - t.Errorf("isValidGitRepository returned an unexpected error: %q, expecting EmptyGitRepositoryError", err.Error()) - } - } else { - t.Errorf("isValidGitRepository returned no error, expecting EmptyGitRepositoryError") + empty, err = LocalNonBareGitRepositoryIsEmpty(fs, d) + if !empty || err != nil { + t.Errorf("LocalNonBareGitRepositoryIsEmpty returned %v, %v", ok, err) } + // a directory which is not a git repo d = filepath.Join(d, ".git") - // There is no .git in the provided directory - // Should return false with no error - if ok, err = isValidGitRepository(fs, d); ok { - t.Errorf("The %q directory is not git repository", d) + ok, err = IsLocalNonBareGitRepository(fs, d) + if ok || err != nil { + t.Errorf("IsLocalNonBareGitRepository returned %v, %v", ok, err) } + // a submodule git repo with a commit + d, err = CreateLocalGitDirectoryWithSubmodule() if err != nil { - t.Errorf("isValidGitRepository returned an unexpected error: %q", err.Error()) + t.Fatal(err) } - - d = CreateLocalGitDirectoryWithSubmodule(t) defer os.RemoveAll(d) - ok, err = isValidGitRepository(fs, filepath.Join(d, "submodule")) + ok, err = IsLocalNonBareGitRepository(fs, filepath.Join(d, "submodule")) if !ok || err != nil { - t.Errorf("Expected isValidGitRepository to return true, nil on submodule; got %v, %v", ok, err) + t.Errorf("IsLocalNonBareGitRepository returned %v, %v", ok, err) } -} - -func TestValidCloneSpec(t *testing.T) { - gitLocalDir := CreateLocalGitDirectory(t) - defer os.RemoveAll(gitLocalDir) - - valid := []string{"git@github.com:user/repo.git", - "git://github.com/user/repo.git", - "git://github.com/user/repo", - "http://github.com/user/repo.git", - "http://github.com/user/repo", - "https://github.com/user/repo.git", - "https://github.com/user/repo", - "file://" + gitLocalDir, - "file://" + gitLocalDir + "#master", - gitLocalDir, - gitLocalDir + "#master", - "git@192.168.122.1:repositories/authooks", - "mbalazs@build.ulx.hu:/var/git/eap-ulx.git", - "ssh://git@[2001:db8::1]/repository.git", - "ssh://git@mydomain.com:8080/foo/bar", - "git@[2001:db8::1]:repository.git", - "git@[2001:db8::1]:/repository.git", - "g_m@foo.bar:foo/bar", - "g-m@foo.bar:foo/bar", - "g.m@foo.bar:foo/bar", - "github.com:openshift/origin.git", - "http://github.com/user/repo#1234", - } - invalid := []string{"g&m@foo.bar:foo.bar", - "~git@test.server/repo.git", - "some/lo:cal/path", // a local path that does not exist, but "some/lo" is not a valid host name - "http://github.com/user/repo#%%%%", - } - - gh := New(fs.NewFileSystem(), cmd.NewCommandRunner()) - - for _, scenario := range valid { - result, _ := gh.ValidCloneSpec(scenario) - if result == false { - t.Error(scenario) - } - } - for _, scenario := range invalid { - result, _ := gh.ValidCloneSpec(scenario) - if result { - t.Error(scenario) - } - } -} - -func TestValidCloneSpecRemoteOnly(t *testing.T) { - valid := []string{"git://github.com/user/repo.git", - "git://github.com/user/repo", - "http://github.com/user/repo.git", - "http://github.com/user/repo", - "https://github.com/user/repo.git", - "https://github.com/user/repo", - "ssh://git@[2001:db8::1]/repository.git", - "ssh://git@mydomain.com:8080/foo/bar", - "git@github.com:user/repo.git", - "git@192.168.122.1:repositories/authooks", - "mbalazs@build.ulx.hu:/var/git/eap-ulx.git", - "git@[2001:db8::1]:repository.git", - "git@[2001:db8::1]:/repository.git", - } - invalid := []string{"file:///home/user/code/repo.git", - "/home/user/code/repo.git", - } - - gh := New(fs.NewFileSystem(), cmd.NewCommandRunner()) - - for _, scenario := range valid { - result := gh.ValidCloneSpecRemoteOnly(scenario) - if result == false { - t.Error(scenario) - } - } - for _, scenario := range invalid { - result := gh.ValidCloneSpecRemoteOnly(scenario) - if result { - t.Error(scenario) - } + empty, err = LocalNonBareGitRepositoryIsEmpty(fs, filepath.Join(d, "submodule")) + if empty || err != nil { + t.Errorf("LocalNonBareGitRepositoryIsEmpty returned %v, %v", ok, err) } } @@ -164,7 +81,7 @@ func getGit() (Git, *testcmd.FakeCmdRunner) { func TestGitClone(t *testing.T) { gh, ch := getGit() - err := gh.Clone("source1", "target1", CloneConfig{Quiet: true, Recursive: true}) + err := gh.Clone(MustParse("source1"), "target1", CloneConfig{Quiet: true, Recursive: true}) if err != nil { t.Errorf("Unexpected error returned from clone: %v", err) } @@ -180,7 +97,7 @@ func TestGitCloneError(t *testing.T) { gh, ch := getGit() runErr := fmt.Errorf("Run Error") ch.Err = runErr - err := gh.Clone("source1", "target1", CloneConfig{}) + err := gh.Clone(MustParse("source1"), "target1", CloneConfig{}) if err != runErr { t.Errorf("Unexpected error returned from clone: %v", err) } diff --git a/pkg/scm/git/testhelpers.go b/pkg/scm/git/testhelpers.go index 32f7e4a4f..8b1b0cb9b 100644 --- a/pkg/scm/git/testhelpers.go +++ b/pkg/scm/git/testhelpers.go @@ -4,69 +4,83 @@ import ( "io/ioutil" "os" "path/filepath" - "testing" "github.com/openshift/source-to-image/pkg/util/cmd" "github.com/openshift/source-to-image/pkg/util/cygpath" ) // CreateLocalGitDirectory creates a git directory with a commit -func CreateLocalGitDirectory(t *testing.T) string { +func CreateLocalGitDirectory() (string, error) { cr := cmd.NewCommandRunner() - dir := CreateEmptyLocalGitDirectory(t) + + dir, err := CreateEmptyLocalGitDirectory() + if err != nil { + return "", err + } + f, err := os.Create(filepath.Join(dir, "testfile")) if err != nil { - t.Fatal(err) + return "", err } f.Close() + err = cr.RunWithOptions(cmd.CommandOpts{Dir: dir}, "git", "add", ".") if err != nil { - t.Fatal(err) + return "", err } + err = cr.RunWithOptions(cmd.CommandOpts{Dir: dir, EnvAppend: []string{"GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@test", "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@test"}}, "git", "commit", "-m", "testcommit") if err != nil { - t.Fatal(err) + return "", err } - return dir + return dir, nil } // CreateEmptyLocalGitDirectory creates a git directory with no checkin yet -func CreateEmptyLocalGitDirectory(t *testing.T) string { +func CreateEmptyLocalGitDirectory() (string, error) { cr := cmd.NewCommandRunner() dir, err := ioutil.TempDir(os.TempDir(), "gitdir-s2i-test") if err != nil { - t.Fatal(err) + return "", err } + err = cr.RunWithOptions(cmd.CommandOpts{Dir: dir}, "git", "init") if err != nil { - t.Fatal(err) + return "", err } - return dir + return dir, nil } // CreateLocalGitDirectoryWithSubmodule creates a git directory with a submodule -func CreateLocalGitDirectoryWithSubmodule(t *testing.T) string { +func CreateLocalGitDirectoryWithSubmodule() (string, error) { cr := cmd.NewCommandRunner() - submodule := CreateLocalGitDirectory(t) + submodule, err := CreateLocalGitDirectory() + if err != nil { + return "", err + } defer os.RemoveAll(submodule) if cygpath.UsingCygwinGit { var err error submodule, err = cygpath.ToSlashCygwin(submodule) if err != nil { - t.Fatal(err) + return "", err } } - dir := CreateEmptyLocalGitDirectory(t) - err := cr.RunWithOptions(cmd.CommandOpts{Dir: dir}, "git", "submodule", "add", submodule, "submodule") + dir, err := CreateEmptyLocalGitDirectory() + if err != nil { + return "", err + } + + err = cr.RunWithOptions(cmd.CommandOpts{Dir: dir}, "git", "submodule", "add", submodule, "submodule") if err != nil { - t.Fatal(err) + return "", err } - return dir + return dir, nil } diff --git a/pkg/scm/git/url.go b/pkg/scm/git/url.go new file mode 100644 index 000000000..c10bfa1f7 --- /dev/null +++ b/pkg/scm/git/url.go @@ -0,0 +1,195 @@ +package git + +import ( + "fmt" + "net/url" + "path/filepath" + "regexp" + "runtime" + "strings" +) + +// According to git-clone(1), a "Git URL" can be in one of three broad types: +// 1) A standards-compliant URL. +// a) The scheme may be followed by '://', +// e.g. https://github.com/openshift/origin, file:///foo/bar, etc. In +// this case, note that among other things, a standards-compliant file URL +// must have an empty host part, an absolute path and no backslashes. The +// Git for Windows URL parser accepts many non-compliant URLs, but we +// don't. +// b) Alternatively, the scheme may be followed by '::', in which case it is +// treated as an transport/opaque address pair, e.g. +// http::http://github.com/openshift/origin.git . +// 2) The "alternative scp-like syntax", including a ':' with no preceding '/', +// but not of the form C:... on Windows, e.g. +// git@github.com:openshift/origin, etc. +// 3) An OS-specific relative or absolute local file path, e.g. foo/bar, +// C:\foo\bar, etc. +// +// We extend all of the above URL types to additionally accept an optional +// appended #fragment, which is given to specify a git reference. +// +// The git client allows Git URL rewriting rules to be defined. The meaning of +// a Git URL cannot be 100% guaranteed without consulting the rewriting rules. + +// URLType indicates the type of the URL (see above) +type URLType int + +const ( + // URLTypeURL is the URL type (see above) + URLTypeURL URLType = iota + // URLTypeSCP is the SCP type (see above) + URLTypeSCP + // URLTypeLocal is the local type (see above) + URLTypeLocal +) + +// String returns a string representation of the URLType +func (t URLType) String() string { + switch t { + case URLTypeURL: + return "URLTypeURL" + case URLTypeSCP: + return "URLTypeSCP" + case URLTypeLocal: + return "URLTypeLocal" + } + panic("unknown URLType") +} + +// GoString returns a Go string representation of the URLType +func (t URLType) GoString() string { + return t.String() +} + +// URL represents a "Git URL" +type URL struct { + URL url.URL + Type URLType +} + +var urlSchemeRegexp = regexp.MustCompile("(?i)^[a-z][-a-z0-9+.]*:") // matches scheme: according to RFC3986 +var dosDriveRegexp = regexp.MustCompile("(?i)^[a-z]:") +var scpRegexp = regexp.MustCompile("^" + + "(?:([^@/]*)@)?" + // user@ (optional) + "([^/]*):" + // host: + "(.*)" + // path + "$") + +func splitOnByte(s string, c byte) (string, string) { + if i := strings.IndexByte(s, c); i != -1 { + return s[:i], s[i+1:] + } + return s, "" +} + +// Parse parses a "Git URL" +func Parse(rawurl string) (*URL, error) { + if urlSchemeRegexp.MatchString(rawurl) && + (runtime.GOOS != "windows" || !dosDriveRegexp.MatchString(rawurl)) { + u, err := url.Parse(rawurl) + if err != nil { + return nil, err + } + + if u.Scheme == "file" && u.Opaque == "" { + if u.Host != "" { + return nil, fmt.Errorf("url %q has non-empty host", rawurl) + } + if runtime.GOOS == "windows" && (len(u.Path) == 0 || !filepath.IsAbs(u.Path[1:])) { + return nil, fmt.Errorf("url %q has non-absolute path", rawurl) + } + } + + return &URL{ + URL: *u, + Type: URLTypeURL, + }, nil + } + + s, fragment := splitOnByte(rawurl, '#') + + if m := scpRegexp.FindStringSubmatch(s); m != nil && + (runtime.GOOS != "windows" || !dosDriveRegexp.MatchString(s)) { + u := &url.URL{ + Host: m[2], + Path: m[3], + Fragment: fragment, + } + if m[1] != "" { + u.User = url.User(m[1]) + } + + return &URL{ + URL: *u, + Type: URLTypeSCP, + }, nil + } + + return &URL{ + URL: url.URL{ + Path: s, + Fragment: fragment, + }, + Type: URLTypeLocal, + }, nil +} + +// MustParse parses a "Git URL" and panics on failure +func MustParse(rawurl string) *URL { + u, err := Parse(rawurl) + if err != nil { + panic(err) + } + return u +} + +// String returns a string representation of the URL +func (u URL) String() string { + var s string + switch u.Type { + case URLTypeURL: + return u.URL.String() + case URLTypeSCP: + if u.URL.User != nil { + s = u.URL.User.Username() + "@" + } + s += u.URL.Host + ":" + u.URL.Path + case URLTypeLocal: + s = u.URL.Path + } + if u.URL.RawQuery != "" { + s += "?" + u.URL.RawQuery + } + if u.URL.Fragment != "" { + s += "#" + u.URL.Fragment + } + return s +} + +// StringNoFragment returns a string representation of the URL without its +// fragment +func (u URL) StringNoFragment() string { + u.URL.Fragment = "" + return u.String() +} + +// IsLocal returns true if the Git URL refers to a local repository +func (u URL) IsLocal() bool { + return u.Type == URLTypeLocal || (u.Type == URLTypeURL && u.URL.Scheme == "file" && u.URL.Opaque == "") +} + +// LocalPath returns the path to a local repository in OS-native format. It is +// assumed that IsLocal() is true +func (u URL) LocalPath() string { + switch { + case u.Type == URLTypeLocal: + return u.URL.Path + case u.Type == URLTypeURL && u.URL.Scheme == "file" && u.URL.Opaque == "": + if runtime.GOOS == "windows" && len(u.URL.Path) > 0 && u.URL.Path[0] == '/' { + return filepath.FromSlash(u.URL.Path[1:]) + } + return filepath.FromSlash(u.URL.Path) + } + panic("LocalPath called on non-local URL") +} diff --git a/pkg/scm/git/url_test.go b/pkg/scm/git/url_test.go new file mode 100644 index 000000000..637d7b7cd --- /dev/null +++ b/pkg/scm/git/url_test.go @@ -0,0 +1,295 @@ +package git + +import ( + "net/url" + "reflect" + "runtime" + "strings" + "testing" +) + +type parseTest struct { + rawurl string + expectedGitURL *URL + expectedError bool +} + +func TestParse(t *testing.T) { + var tests []parseTest + + switch runtime.GOOS { + case "windows": + tests = append(tests, + parseTest{ + rawurl: "file://relative/path", + expectedError: true, + }, + parseTest{ + rawurl: "file:///relative/path", + expectedError: true, + }, + parseTest{ + rawurl: "file:///c:/absolute/path?query#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "file", + Path: "/c:/absolute/path", + RawQuery: "query", + Fragment: "fragment", + }, + Type: URLTypeURL, + }, + }, + ) + + default: + tests = append(tests, + parseTest{ + rawurl: "file://relative/path", + expectedError: true, + }, + parseTest{ + rawurl: "file:///absolute/path?query#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "file", + Path: "/absolute/path", + RawQuery: "query", + Fragment: "fragment", + }, + Type: URLTypeURL, + }, + }, + ) + } + + tests = append(tests, + // http:// + parseTest{ + rawurl: "http://user:pass@github.com:443/user/repo.git?query#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "http", + User: url.UserPassword("user", "pass"), + Host: "github.com:443", + Path: "/user/repo.git", + RawQuery: "query", + Fragment: "fragment", + }, + Type: URLTypeURL, + }, + }, + parseTest{ + rawurl: "http://user@1.2.3.4:443/repo?query#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "http", + User: url.User("user"), + Host: "1.2.3.4:443", + Path: "/repo", + RawQuery: "query", + Fragment: "fragment", + }, + Type: URLTypeURL, + }, + }, + parseTest{ + rawurl: "http://[::ffff:1.2.3.4]:443", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "http", + Host: "[::ffff:1.2.3.4]:443", + }, + Type: URLTypeURL, + }, + }, + parseTest{ + rawurl: "http://github.com/openshift/origin", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "http", + Host: "github.com", + Path: "/openshift/origin", + }, + Type: URLTypeURL, + }, + }, + + // transport::opaque + parseTest{ + rawurl: "http::http://github.com/openshift/origin", + expectedGitURL: &URL{ + URL: url.URL{ + Scheme: "http", + Opaque: ":http://github.com/openshift/origin", + }, + Type: URLTypeURL, + }, + }, + + // git@host ... + parseTest{ + rawurl: "user@github.com:/user/repo.git#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + User: url.User("user"), + Host: "github.com", + Path: "/user/repo.git", + Fragment: "fragment", + }, + Type: URLTypeSCP, + }, + }, + parseTest{ + rawurl: "user@github.com:user/repo.git#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + User: url.User("user"), + Host: "github.com", + Path: "user/repo.git", + Fragment: "fragment", + }, + Type: URLTypeSCP, + }, + }, + parseTest{ + rawurl: "user@1.2.3.4:repo#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + User: url.User("user"), + Host: "1.2.3.4", + Path: "repo", + Fragment: "fragment", + }, + Type: URLTypeSCP, + }, + }, + parseTest{ + rawurl: "[::ffff:1.2.3.4]:", + expectedGitURL: &URL{ + URL: url.URL{ + Host: "[::ffff:1.2.3.4]", + }, + Type: URLTypeSCP, + }, + }, + parseTest{ + rawurl: "git@github.com:openshift/origin", + expectedGitURL: &URL{ + URL: url.URL{ + User: url.User("git"), + Host: "github.com", + Path: "openshift/origin", + }, + Type: URLTypeSCP, + }, + }, + + // path ... + parseTest{ + rawurl: "/absolute#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Path: "/absolute", + Fragment: "fragment", + }, + Type: URLTypeLocal, + }, + }, + parseTest{ + rawurl: "relative#fragment", + expectedGitURL: &URL{ + URL: url.URL{ + Path: "relative", + Fragment: "fragment", + }, + Type: URLTypeLocal, + }, + }, + ) + + for _, test := range tests { + url, err := Parse(test.rawurl) + if test.expectedError != (err != nil) { + t.Errorf("%s: Parse() returned err: %v", test.rawurl, err) + } + if err != nil { + continue + } + + if !reflect.DeepEqual(url, test.expectedGitURL) { + t.Errorf("%s: Parse() returned\n\t%#v\nWanted\n\t%#v", test.rawurl, url, test.expectedGitURL) + } + + if url.String() != test.rawurl { + t.Errorf("%s: String() returned %s", test.rawurl, url.String()) + } + + if url.StringNoFragment() != strings.SplitN(test.rawurl, "#", 2)[0] { + t.Errorf("%s: StringNoFragment() returned %s", test.rawurl, url.StringNoFragment()) + } + } +} + +func TestStringNoFragment(t *testing.T) { + u := MustParse("part#fragment") + if u.StringNoFragment() != "part" { + t.Errorf("StringNoFragment() returned %s", u.StringNoFragment()) + } + if !reflect.DeepEqual(u, MustParse("part#fragment")) { + t.Errorf("StringNoFragment() modified its argument") + } +} + +type localPathTest struct { + url *URL + expected string +} + +func TestLocalPath(t *testing.T) { + var tests []localPathTest + + switch runtime.GOOS { + case "windows": + tests = append(tests, + localPathTest{ + url: MustParse("file:///c:/foo/bar"), + expected: `c:\foo\bar`, + }, + localPathTest{ + url: MustParse(`c:\foo\bar`), + expected: `c:\foo\bar`, + }, + localPathTest{ + url: MustParse(`\foo\bar`), + expected: `\foo\bar`, + }, + localPathTest{ + url: MustParse(`foo\bar`), + expected: `foo\bar`, + }, + localPathTest{ + url: MustParse(`foo`), + expected: `foo`, + }, + ) + + default: + tests = append(tests, + localPathTest{ + url: MustParse("file:///foo/bar"), + expected: "/foo/bar", + }, + localPathTest{ + url: MustParse("/foo/bar"), + expected: "/foo/bar", + }, + ) + } + + for i, test := range tests { + if test.url.LocalPath() != test.expected { + t.Errorf("%d: LocalPath() returned %s", i, test.url.LocalPath()) + } + } +} diff --git a/pkg/scm/scm.go b/pkg/scm/scm.go index 2a6ba8741..60054fb7c 100644 --- a/pkg/scm/scm.go +++ b/pkg/scm/scm.go @@ -1,10 +1,8 @@ package scm import ( - "fmt" - "github.com/openshift/source-to-image/pkg/build" - s2ierr "github.com/openshift/source-to-image/pkg/errors" + "github.com/openshift/source-to-image/pkg/errors" "github.com/openshift/source-to-image/pkg/scm/downloaders/empty" "github.com/openshift/source-to-image/pkg/scm/downloaders/file" gitdownloader "github.com/openshift/source-to-image/pkg/scm/downloaders/git" @@ -18,44 +16,38 @@ var glog = utilglog.StderrLog // DownloaderForSource determines what SCM plugin should be used for downloading // the sources from the repository. -func DownloaderForSource(fs fs.FileSystem, s string, forceCopy bool) (build.Downloader, string, error) { +func DownloaderForSource(fs fs.FileSystem, s *git.URL, forceCopy bool) (build.Downloader, error) { glog.V(4).Infof("DownloadForSource %s", s) - if len(s) == 0 { - return &empty.Noop{}, s, nil + if s == nil { + return &empty.Noop{}, nil } - details, mods, err := git.ParseFile(fs, s) - glog.V(4).Infof("return from ParseFile file exists %v proto specified %v use copy %v", details.FileExists, details.ProtoSpecified, details.UseCopy) - if err != nil { - if e, ok := err.(s2ierr.Error); !forceCopy || !(ok && (e.ErrorCode == s2ierr.EmptyGitRepositoryError)) { - return nil, s, err + if s.IsLocal() { + if forceCopy { + return &file.File{FileSystem: fs}, nil } - } - - if details.FileExists && details.BadRef { - return nil, s, fmt.Errorf("local location referenced by %s exists but the input after the # is malformed", s) - } - if details.FileExists && mods != nil { - glog.V(4).Infof("new source from parse file %s", mods.Path) - s = "file://" + mods.Path - } - - if details.FileExists && (details.UseCopy || forceCopy) { - return &file.File{FileSystem: fs}, s, nil - } + isLocalNonBareGitRepo, err := git.IsLocalNonBareGitRepository(fs, s.LocalPath()) + if err != nil { + return nil, err + } + if !isLocalNonBareGitRepo { + return &file.File{FileSystem: fs}, nil + } - // If s is a valid Git clone spec, use git to download the source - g := git.New(fs, cmd.NewCommandRunner()) - ok, err := g.ValidCloneSpec(s) - if err != nil { - return nil, s, err - } + isEmpty, err := git.LocalNonBareGitRepositoryIsEmpty(fs, s.LocalPath()) + if err != nil { + return nil, err + } + if isEmpty { + return nil, errors.NewEmptyGitRepositoryError(s.LocalPath()) + } - if ok { - return &gitdownloader.Clone{Git: g, FileSystem: fs}, s, nil + if !git.HasGitBinary() { + return &file.File{FileSystem: fs}, nil + } } - return nil, s, fmt.Errorf("no downloader defined for location: %q", s) + return &gitdownloader.Clone{Git: git.New(fs, cmd.NewCommandRunner()), FileSystem: fs}, nil } diff --git a/pkg/scm/scm_test.go b/pkg/scm/scm_test.go index 6cb03aeef..9d9ff1c6d 100644 --- a/pkg/scm/scm_test.go +++ b/pkg/scm/scm_test.go @@ -3,8 +3,8 @@ package scm import ( "io/ioutil" "os" + "path/filepath" "reflect" - "strings" "testing" "github.com/openshift/source-to-image/pkg/scm/git" @@ -12,47 +12,36 @@ import ( ) func TestDownloaderForSource(t *testing.T) { - gitLocalDir := git.CreateLocalGitDirectory(t) + gitLocalDir, err := git.CreateLocalGitDirectory() + if err != nil { + t.Fatal(err) + } defer os.RemoveAll(gitLocalDir) localDir, _ := ioutil.TempDir(os.TempDir(), "localdir-s2i-test") defer os.RemoveAll(localDir) - tc := map[string]string{ + tc := map[*git.URL]string{ // Valid Git clone specs - "git://github.com/bar": "git.Clone", - "https://github.com/bar": "git.Clone", - "git@github.com:foo/bar.git": "git.Clone", - // Non-existing local path (it is not git repository, so it is file - // download) - "file://foo/bar": "error", - "/foo/bar": "error", + git.MustParse("git://github.com/bar"): "git.Clone", + git.MustParse("https://github.com/bar"): "git.Clone", + git.MustParse("git@github.com:foo/bar.git"): "git.Clone", // Local directory with valid Git repository - gitLocalDir: "git.Clone", - "file://" + gitLocalDir: "git.Clone", + git.MustParse(gitLocalDir): "git.Clone", + git.MustParse("file:///" + filepath.ToSlash(gitLocalDir)): "git.Clone", // Local directory that exists but it is not Git repository - localDir: "file.File", - "file://" + localDir: "file.File", - "foo://github.com/bar": "error", - "./foo": "error", + git.MustParse(localDir): "file.File", + git.MustParse("file:///" + filepath.ToSlash(localDir)): "file.File", // Empty source string - "": "empty.Noop", + nil: "empty.Noop", } for s, expected := range tc { - r, filePathUpdate, err := DownloaderForSource(fs.NewFileSystem(), s, false) + r, err := DownloaderForSource(fs.NewFileSystem(), s, false) if err != nil { - if expected != "error" { - t.Errorf("Unexpected error %q for %q, expected %q", err, s, expected) - } + t.Errorf("Unexpected error %q for %q, expected %q", err, s, expected) continue } - if s == gitLocalDir || s == localDir { - if !strings.HasPrefix(filePathUpdate, "file://") { - t.Errorf("input %s should have produced a file path update starting with file:// but produced: %s", s, filePathUpdate) - } - } - expected = "*" + expected if reflect.TypeOf(r).String() != expected { t.Errorf("Expected %q for %q, got %q", expected, s, reflect.TypeOf(r).String()) @@ -61,32 +50,32 @@ func TestDownloaderForSource(t *testing.T) { } func TestDownloaderForSourceOnRelativeGit(t *testing.T) { - gitLocalDir := git.CreateLocalGitDirectory(t) + gitLocalDir, err := git.CreateLocalGitDirectory() + if err != nil { + t.Fatal(err) + } defer os.RemoveAll(gitLocalDir) os.Chdir(gitLocalDir) - r, s, err := DownloaderForSource(fs.NewFileSystem(), ".", false) + r, err := DownloaderForSource(fs.NewFileSystem(), git.MustParse("."), false) if err != nil { t.Errorf("Unexpected error %q for %q, expected %q", err, ".", "git.Clone") } - if !strings.HasPrefix(s, "file://") { - t.Errorf("Expected source to have 'file://' prefix, it is %q", s) - } if reflect.TypeOf(r).String() != "*git.Clone" { t.Errorf("Expected %q for %q, got %q", "*git.Clone", ".", reflect.TypeOf(r).String()) } } func TestDownloaderForceCopy(t *testing.T) { - gitLocalDir := git.CreateLocalGitDirectory(t) + gitLocalDir, err := git.CreateLocalGitDirectory() + if err != nil { + t.Fatal(err) + } defer os.RemoveAll(gitLocalDir) os.Chdir(gitLocalDir) - r, s, err := DownloaderForSource(fs.NewFileSystem(), ".", true) + r, err := DownloaderForSource(fs.NewFileSystem(), git.MustParse("."), true) if err != nil { t.Errorf("Unexpected error %q for %q, expected %q", err, ".", "*file.File") } - if !strings.HasPrefix(s, "file://") { - t.Errorf("Expected source to have 'file://' prefix, it is %q", s) - } if reflect.TypeOf(r).String() != "*file.File" { t.Errorf("Expected %q for %q, got %q", "*file.File", ".", reflect.TypeOf(r).String()) } diff --git a/pkg/test/git.go b/pkg/test/git.go index ee014c0be..bde3971e1 100644 --- a/pkg/test/git.go +++ b/pkg/test/git.go @@ -1,7 +1,6 @@ package test import ( - "net/url" "os" "github.com/openshift/source-to-image/pkg/scm/git" @@ -9,10 +8,7 @@ import ( // FakeGit provides a fake Git type FakeGit struct { - ValidCloneSpecSource string - ValidCloneSpecResult bool - - CloneSource string + CloneSource *git.URL CloneTarget string CloneError error @@ -29,26 +25,8 @@ type FakeGit struct { SubmoduleUpdateError error } -// ValidCloneSpec returns a valid Git clone specification -func (f *FakeGit) ValidCloneSpec(source string) (bool, error) { - f.ValidCloneSpecSource = source - return f.ValidCloneSpecResult, nil -} - -//ValidCloneSpecRemoteOnly returns a valid Git clone specification -func (f *FakeGit) ValidCloneSpecRemoteOnly(source string) bool { - f.ValidCloneSpecSource = source - return f.ValidCloneSpecResult -} - -//MungeNoProtocolURL returns a valid no protocol Git URL -func (f *FakeGit) MungeNoProtocolURL(source string, url *url.URL) error { - f.ValidCloneSpecSource = source - return nil -} - // Clone clones the fake source Git repository to target directory -func (f *FakeGit) Clone(source, target string, c git.CloneConfig) error { +func (f *FakeGit) Clone(source *git.URL, target string, c git.CloneConfig) error { f.CloneSource = source f.CloneTarget = target return f.CloneError diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index 1a43be25c..3a4bc4d90 100644 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -24,6 +24,7 @@ import ( "github.com/openshift/source-to-image/pkg/build/strategies" "github.com/openshift/source-to-image/pkg/docker" dockerpkg "github.com/openshift/source-to-image/pkg/docker" + "github.com/openshift/source-to-image/pkg/scm/git" "github.com/openshift/source-to-image/pkg/tar" "github.com/openshift/source-to-image/pkg/util" "github.com/openshift/source-to-image/pkg/util/fs" @@ -261,7 +262,7 @@ func (i *integrationTest) exerciseCleanAllowedUIDsBuild(tag, imageName string, e DockerConfig: docker.GetDefaultDockerConfig(), BuilderImage: imageName, BuilderPullPolicy: api.DefaultBuilderPullPolicy, - Source: TestSource, + Source: git.MustParse(TestSource), Tag: tag, Incremental: false, ScriptsURL: "", @@ -318,7 +319,7 @@ func (i *integrationTest) exerciseCleanBuild(tag string, verifyCallback bool, im DockerConfig: docker.GetDefaultDockerConfig(), BuilderImage: imageName, BuilderPullPolicy: api.DefaultBuilderPullPolicy, - Source: TestSource, + Source: git.MustParse(TestSource), Tag: buildTag, Incremental: false, CallbackURL: callbackURL, @@ -405,7 +406,7 @@ func (i *integrationTest) exerciseInjectionBuild(tag, imageName string, injectio DockerConfig: docker.GetDefaultDockerConfig(), BuilderImage: imageName, BuilderPullPolicy: api.DefaultBuilderPullPolicy, - Source: TestSource, + Source: git.MustParse(TestSource), Tag: tag, Injections: injectionList, ExcludeRegExp: tar.DefaultExclusionPattern.String(), @@ -448,7 +449,7 @@ func (i *integrationTest) exerciseIncrementalBuild(tag, imageName string, remove DockerConfig: docker.GetDefaultDockerConfig(), BuilderImage: imageName, BuilderPullPolicy: api.DefaultBuilderPullPolicy, - Source: TestSource, + Source: git.MustParse(TestSource), Tag: tag, Incremental: false, RemovePreviousImage: removePreviousImage, @@ -472,7 +473,7 @@ func (i *integrationTest) exerciseIncrementalBuild(tag, imageName string, remove DockerConfig: docker.GetDefaultDockerConfig(), BuilderImage: imageName, BuilderPullPolicy: api.DefaultBuilderPullPolicy, - Source: TestSource, + Source: git.MustParse(TestSource), Tag: tag, Incremental: true, RemovePreviousImage: removePreviousImage,