Skip to content

fix repo/path/tag detection to work with https repos #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
serchaos wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix repo/path/tag detection to work with https repos #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions stevedore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ func FindImagesInCwd(filter cmd.FilterFunc) ([]Image, error) {
}

func findImages(filter cmd.FilterFunc, wd string) (images []Image, err error) {
repo, path, tag := detectRepoPathAndTag(wd)
repo, err := runCmdAndGetOutput("git", "config", "--get", "remote.origin.url")
if err != nil {
log.Fatal("error detecting git repo", err)
}
repo, path, tag := detectRepoPathAndTag(repo, wd)
dockerfiles := findDockerfiles()
for dockerfile, repos := range mapDockerfileToRepos(repo, path, tag, dockerfiles...) {
if !filter(dockerfile) {
Expand Down Expand Up @@ -57,21 +61,30 @@ func (i Image) Push() (err error) {
return runCmdAndPipeOutput(cmd.Output, "docker", "push", i.Url)
}

func detectRepoPathAndTag(wd string) (repo, path, tag string) {
repo, err := runCmdAndGetOutput("git", "config", "--get", "remote.origin.url")
if err != nil {
log.Fatal("error detecting git repo", err)
}

if index := strings.LastIndex(repo, ":"); index != -1 {
repo = repo[index+1:]
func extractRepo(repo string) string {
if strings.HasPrefix(repo, "git") {

@smreed smreed May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer tests where we include the "://" part as well. You'd have to add a condition for both "http" and "https" that way but it seems safer and a bit more clear.

@smreed smreed May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: return an error when the repo starts w/ a prefix we don't expect to be able to handle

@smreed smreed May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait after actually thinking about this, even the "git@..." repos are not guaranteed to have a particular prefix. So we should probably only look for "http://" and "https://" prefixes, then fall back. Maybe add some validation at the end to make sure whatever we end up with doesn't look like it has unexpected bits in it such as '/'.

@serchaos serchaos May 24, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good on all counts, thanks for catching the prefix bit - I'll also pull in some more exhaustive test cases from whatever the remote spec allows. On some level we don't really care about prefix, we just want consistent fragment extraction.

@smreed smreed May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

On Tue, May 24, 2016 at 12:57 PM Kyle notifications@github.com wrote:

In stevedore.go
#12 (comment):

@@ -57,21 +61,30 @@ func (i Image) Push() (err error) {
return runCmdAndPipeOutput(cmd.Output, "docker", "push", i.Url)
}

-func detectRepoPathAndTag(wd string) (repo, path, tag string) {

  • repo, err := runCmdAndGetOutput("git", "config", "--get", "remote.origin.url")
  • if err != nil {
  •   log.Fatal("error detecting git repo", err)

- }

  • if index := strings.LastIndex(repo, ":"); index != -1 {
  •   repo = repo[index+1:]
    +func extractRepo(repo string) string {
  • if strings.HasPrefix(repo, "git") {

Sounds good on all counts, thanks for catching the prefix bit - I'll also
pull in some more exhaustive test cases from whatever the remote spec
allows. On some level we don't really care about prefix, we just want
consistent fragment extraction.

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
https://github.com/zulily/stevedore/pull/12/files/ae1789827b1a04be04b58d81fbbb8ab3ea3c08cc#r64458062

if index := strings.LastIndex(repo, ":"); index != -1 {
repo = repo[index+1:]
}
} else if strings.HasPrefix(repo, "http") {
if last := strings.LastIndex(repo, "/"); last != -1 {
tmp := repo[:last]
if first := strings.LastIndex(tmp, "/"); first != -1 {
repo = repo[first+1:]
}
}
}

if strings.HasSuffix(repo, ".git") {
repo = repo[:len(repo)-4]
}

path, err = runCmdAndGetOutput("git", "rev-parse", "--show-toplevel")
return repo
}

func detectRepoPathAndTag(gitRemote, wd string) (repo, path, tag string) {
repo = extractRepo(gitRemote)
path, err := runCmdAndGetOutput("git", "rev-parse", "--show-toplevel")
switch {
case wd == path:
path = ""
Expand All @@ -81,7 +94,7 @@ func detectRepoPathAndTag(wd string) (repo, path, tag string) {
default:
log.Fatal("Current directory is not child of top level", wd, path)
}

if cmd.Tag == "" {

tag, err = runCmdAndGetOutput("git", "rev-parse", "HEAD")
Expand All @@ -92,7 +105,6 @@ func detectRepoPathAndTag(wd string) (repo, path, tag string) {
tag = cmd.Tag
}


if len(tag) > 7 {
tag = tag[:7]
}
Expand Down
29 changes: 29 additions & 0 deletions stevedore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,32 @@ func TestGenerateRepoNames(t *testing.T) {
}
}
}

type detectPathTagCase struct {
repo string
expected string
}

var (
testDetectPathTagCases = []detectPathTagCase{
detectPathTagCase{
repo: "git@github.com:foo/bar.git",
expected: "foo/bar",
},
detectPathTagCase{
repo: "https://github.com/foo/bar.git",
expected: "foo/bar",
},
}
)

func TestDetectRepoPathAndTag(t *testing.T) {
for _, testCase := range testDetectPathTagCases {
actual := extractRepo(testCase.repo)

if actual != testCase.expected {
t.Errorf("Expected (%q), got (%q)", testCase.expected, actual)

@smreed smreed May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super minor nit but since you're quoting the values with %q, there's not much need for the parens. I think the test error would be readable enough w/o them.

t.FailNow()
}
}
}