feat: kfn build uses ko as default #644
Conversation
justinsb
left a comment
justinsb
left a comment
There was a problem hiding this comment.
A few suggestions for the future, but lgtm
| r.Command.Flags().StringVarP(&r.BuilderType, "builder", "b", Ko, | ||
| "the image builder. `ko` is the default builder, which requires `go build`; `docker` is accepted, and "+ | ||
| " requires you to have docker installed and running") | ||
| r.Command.Flags().StringVarP(&r.Docker.Image, "image", "i", Image, |
There was a problem hiding this comment.
Nit: does r.Docker.Image this mean that this can't configure the ko image?
There was a problem hiding this comment.
Yeah, I had a todo to better align the -image and -tag flags. It would require several corner case handling which are not quite the focus of this PR.
| r.Command.Flags().StringVarP(&r.Ko.Tag, "tag", "t", "latest", | ||
| "the ko image tag") | ||
| // TODO: Docker CLI uses `--tag` flag to refer to "image:tag", which could be confusing but broadly accepted. | ||
| // We should better guide users on how to use "tag" and "image" flags for kfn. |
There was a problem hiding this comment.
Yeah, I think this is OK for now but I should be able to specify the image name (with tag) and have it work for both docker and ko. I guess we also want consistent behaviour if we specify only the tag or image, but that's less important.
There was a problem hiding this comment.
Totally agree.
I want to make it more user friendly so would like to support cases if image contains tag, or tag contains image (which docker build does).
go/cli/commands/build.go
Outdated
| } | ||
| color.Green(out.String()) | ||
| color.Green("Image %v builds successfully. Now you can publish the image", r.Tag) | ||
| fmt.Println(out) |
There was a problem hiding this comment.
If you're going to print it anyway, might be better to stream it (by setting cmd.Stdout = os.Stdout and cmd.Stderr = os.Stderr) ... TBD.
There was a problem hiding this comment.
Using cmd.Stdout = os.Stdout will printout all the logs, while the out only shows the last line (empty in many cases). I'm sort of hesitating between concise user messages and debugging modes with detailed logs.
changed to stdout.
go/cli/commands/build.go
Outdated
| } | ||
|
|
||
| func (r *BuildRunner) dockerfileExist() bool { | ||
| func (r *DockerBuilder) fileExist() bool { |
There was a problem hiding this comment.
Thanks! changed back to dockerfileExists.
Because I changed the object from BuildRunner to DockerBuilder, I thought it is verbose to repeat docker. similar to docker build --file.
| Image = "function:latest" | ||
| DockerfilePath = "Dockerfile" | ||
| builtinDockerfilePath = "embed/Dockerfile" | ||
| BuiltinDockerfilePath = "embed/Dockerfile" |
There was a problem hiding this comment.
Try the go:embed stuff ... it's really cool
There was a problem hiding this comment.
yeah, I'm using go:embed. https://github.com/GoogleContainerTools/kpt-functions-sdk/blob/master/go/cli/commands/build.go#L31
this is just to give the embedded dockerfile a const var.
yuwenma
force-pushed
the
kfn-ko-build
branch
2 times, most recently
from
5d8cfb0 to
c19f84a
Compare
2 participants
This PR extends the
kfn buildto usekoas the default builder.It now supports the following cases
kfn build:if ko is not installed,
kfn buildwill rungo install github.com/google/ko@latestto installko; default repo isko.local.user can specify two flags
--repofor KO_DOCKER_REPO and--tagto tag the ko imagekfn build --builder docker:if docker is not installed, this execution will fail with user message
kfn requires that "docker" is installed and on the PATHusers can specify to use docker as builder via
--builder=dockerflagusers can specify two flags to control the docker build
--dockerfile=<PATH TO DOCKERFILE>and--image=<image:tag>which is the same asdocker build--tagflag.I have a separate PR to hide the Dockerfile unless user requests it to be exposed. #643
Unittest coverage on
kfn build.