Skip to content
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.

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

Add go module support #60

Merged
merged 4 commits into from
Jul 13, 2019
Merged

Add go module support #60

merged 4 commits into from
Jul 13, 2019

Conversation

jonjohnsonjr
Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr commented Jul 12, 2019

This is based on my comment here.

Fixes #7

This uses go list -mod=readonly -m -json to determine if we should use go modules (and fetch required info). For modules, we use the Dir as the source directory root (instead of GOPATH) and the Path to make IsSupportedReference much more efficient for modules.

E.g. I've cloned https://github.com/xmlking/micro-starter-kit to ~/hub/micro-starter-kit:

$ go list -mod=readonly -m -json
{
	"Path": "github.com/xmlking/micro-starter-kit",
	"Main": true,
	"Dir": "/home/jonjohnson/hub/micro-starter-kit",
	"GoMod": "/home/jonjohnson/hub/micro-starter-kit/go.mod",
	"GoVersion": "1.12"
}

We call that once per invocation to determine if we're in a project using go modules, then reuse that info in order to find packages with matching import paths. If we don't find one, we fall back to GOPATH (old behavior).

This also uses the packages package to handle qualifying local imports (in order to make them work with go modules).

@jonjohnsonjr jonjohnsonjr requested a review from imjasonh July 12, 2019 21:45
@jonjohnsonjr
Copy link
Collaborator Author

cc @paivagustavo

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

This looks great! 🎉

Just a few tiny comments, otherwise lgtm

pkg/build/gobuild.go Outdated Show resolved Hide resolved
pkg/build/gobuild.go Outdated Show resolved Hide resolved
pkg/build/gobuild_test.go Show resolved Hide resolved
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Either way, lgtm, go modules gooooo 🚀

@jonjohnsonjr
Copy link
Collaborator Author

Honest question because I don't know what users would expect: Do we want to fallthrough to checking the importpath from the gopath here?

Good question... I'm not sure. I can imagine someone wanting to do this, even though ideally all your source would be within your project. Should we stop people from building external dependencies?

@imjasonh
Copy link
Member

Yeah I'm just not sure.

My instinct says we should disallow it until someone asks for it, because that's easier than locking it down in the future. But how this would manifest is by silently passing the reference through to the output YAML, instead of raising an error. :-/

@paivagustavo
Copy link

This looks great @jonjohnsonjr. The only caveat about this solution is with multi-modules, if we run ko from inside a sub-module it would not be able to resolve others modules. Take this for example:

├── cmd1
│   ├── main.go
│   └── go.mod // github.com/foo/bar/cmd1
├── cmd2
│   ├── main.go
│   ├── files
│   ├── ─── deploy.yaml  <- reference to // github.com/foo/bar/cmd1
│   └── go.mod // github.com/foo/bar/cmd2
└── go.mod // github.com/foo/bar

Running ko resolve -f files inside the cmd2 it is not going to resolve the github.com/foo/bar/cmd1 since the go list -mod=readonly -m is going to report github.com/foo/bar/cmd2 as the current module. But anyway, I don't see this being an issue here since even the go modules itself doesn't handle this type of dependency very well, just wanted to make you aware of it if you didn't know already.

@jonjohnsonjr
Copy link
Collaborator Author

But anyway, I don't see this being an issue here since even the go modules itself doesn't handle this type of dependency very well, just wanted to make you aware of it if you didn't know already.

Yeah that's really frustrating... I was trying to figure out a way around this, but I'm not sure that it's possible to cover 100% of cases we care about. If you can think of a clean way to do this, we can fix it in a separate PR.

What's worse is that if you have this:

├── cmd1
│   ├── main.go
│   └── go.mod // github.com/foo/bar/cmd1
├── cmd2
│   ├── main.go
│   ├── files
│   └── go.mod // github.com/foo/bar/cmd2
├── deploy.yaml  <- reference to // github.com/foo/bar/cmd1
└── go.mod // github.com/foo/bar

it won't work either unless you rm cmd1/go.mod. We can probably fix this case with some heuristics, e.g. if we match a prefix but we can't import it, we could walk down the filesystem trying each directory between them, but that would only work by convention (child go.mod files with prefixes of parents would have matching directory structures). If people complain about this not working we can try that, too.

@jonjohnsonjr
Copy link
Collaborator Author

My instinct says we should disallow it until someone asks for it, because that's easier than locking it down in the future. But how this would manifest is by silently passing the reference through to the output YAML, instead of raising an error. :-/

Agree with the :-/ but I think you're right. Let's see if anyone actually does this. Updated.

@jonjohnsonjr
Copy link
Collaborator Author

Let's merge this and see how people feel about it.

@jonjohnsonjr jonjohnsonjr merged commit 3566d3f into ko-build:master Jul 13, 2019
@jonjohnsonjr jonjohnsonjr deleted the modules branch July 13, 2019 05:54
@mattysweeps
Copy link

@jonjohnsonjr I've ran into a need for submodules to be ko-applyable. Do you think this is possible?

Unfortunately what I have is your worst case scenario:

├── cmd1
│   ├── main.go
│   └── go.mod // github.com/foo/bar/cmd1
├── cmd2
│   ├── main.go
│   ├── files
│   └── go.mod // github.com/foo/bar/cmd2
├── deploy.yaml  <- reference to // github.com/foo/bar/cmd1
└── go.mod // github.com/foo/bar

@jonjohnsonjr
Copy link
Collaborator Author

what I have is your worst case scenario

:(

I might be mistaken, but I would think that scenario actually works, since the directory structure matches the go module path structure (handled here where we just match the prefix).

Can you point me to a repro of this? I can take a look when I get back from kubecon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ko does not work with Go modules
5 participants