Skip to content

Conversation

joelanford
Copy link
Member

Description of the change:
Fixes logic that extracts repo portion of path from $GOPATH when CWD is $GOPATH/src

Motivation for the change:
Closes #1423

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 15, 2019
@estroz
Copy link
Member

estroz commented May 15, 2019

Does this fix the issue? strings.TrimPrefix(currPkg, fsep) should remove fsep from currPkg regardless of whether it is replaced above.

@joelanford
Copy link
Member Author

Does this fix the issue? strings.TrimPrefix(currPkg, fsep) should remove fsep from currPkg regardless of whether it is replaced above.

It does. Here's a line-by-line explanation:

Old way:

gopath := MustSetGopath(MustGetGopath())          // gopath = "/home/user/go"
goSrc := filepath.Join(gopath, SrcDir)            // goSrc = "/home/user/go/src"
wd := MustGetwd()                                 // wd = "/home/user/go/src"
currPkg := strings.Replace(wd, goSrc+fsep, "", 1) // currPkg = "/home/user/go/src"
// strip any "/" prefix from the repo path.
return strings.TrimPrefix(currPkg, fsep)          // return "home/user/go/src"

New way:

gopath := MustSetGopath(MustGetGopath())     // gopath = "/home/user/go"
goSrc := filepath.Join(gopath, SrcDir)       // goSrc = "/home/user/go/src"
wd := MustGetwd()                            // wd = "/home/user/go/src"
currPkg := strings.Replace(wd, goSrc, "", 1) // currPkg = ""
// strip any "/" prefix from the repo path.
return strings.TrimPrefix(currPkg, fsep)     // return ""

In the old way, the extra fsep in strings.Replace causes the prefix not to be matched when wd and goSrc are equal, resulting in no replacement.

@estroz
Copy link
Member

estroz commented May 15, 2019

Ah so if we're in the first child directory of $GOPATH/src this won't work, but does if we're in a grandchild directory. Nice catch.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2019
@joelanford joelanford merged commit fb10d7d into operator-framework:master May 16, 2019
@joelanford joelanford deleted the fix-repo-lookup branch May 16, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid memory adding api following guide

3 participants