Support for go modules #19
Conversation
|
@jonjohnsonjr since you were the one who did the PoC and the first attempt, could you please do an initial review? |
|
I may have broken the |
|
@jonjohnsonjr I've cleaned up the code and it's working for both modules and non modules projects. It still missing some test, I can't figure out how to do it. Maybe create another "ko test" command but with modules and force the Maybe even create a go module project on the I would love some opinions on this. |
I think by default
Which test and what's the issue? |
I guess There is two points I can see for this:
I actually prefer to use
I feel that current tests doesn't assure that ko actually works with modules. |
That seems fine since we already shell out to
Ah, got it. Creating a module inside a |
|
@jonjohnsonjr I've added some tests for it. I had to add an extra abstraction to set the working directory of the build process, so I can fake that I'm running ko from the go module project that is on the I have duplicated some test code just to validate the idea. If you feel like this is enough i'll refactor to be easier to add more test later. |
pkg/build/gobuild.go
Outdated
| return "", errors.New("could not find specified import path: " + importPath) | ||
| } | ||
|
|
||
| output := outputs[0] |
There was a problem hiding this comment.
Will this ever return more than 0 or 1 results?
pkg/build/gobuild_test.go
Outdated
| } | ||
| os.Mkdir(filepath.Join(tmpDir, "kodata"), 0777) | ||
|
|
||
| writeFile(t, filepath.Join(tmpDir, "main.go"), ` |
There was a problem hiding this comment.
Is it possible to pull these out into some checked-in files, and just copy them to a tmpdir? It's kind of ugly with them inlined here. Is this just replicating these files? You could just copy that directory to tmpDir and then write the go.mod file (and maybe fix up symlinks? not sure).
pkg/build/options.go
Outdated
|
|
||
| // withWd is a functional option for overriding the working directory | ||
| // where ko is going run. This is exposed for testing. | ||
| func withWd(wd string) Option { |
There was a problem hiding this comment.
Can you avoid adding this by just doing os.Chdir in the test? (You probably would want to defer a Chdir back to the result of Getwd to avoid messing up other tests).
|
@paivagustavo any chance of picking this back up? |
|
Hi @jonjohnsonjr I totally forgot about this. I'll try to finish finish this week :) |
|
Just replying here to say that I'll finish this tomorrow :) |
|
git is hard 😂 I have done a wrong push 😭 and so this PR was automatically closed. Moving to #50 |
I took some inspiration based on the @jonjohnsonjr PoC google/go-containerregistry/pull/329
And take it futher using the new https://godoc.org/golang.org/x/tools/go/packages package to work with both modules and normal go projects.
I'm not yet satisfied how the codes look but from my first tests it seems to being working. Will try to implement tests to this, don't know how to actually test this.
Fixes #7