Refactor skaffold init for more flexible builder detection #2274
Conversation
TadCordle
requested review from
balopat,
dgageot,
nkubala,
priyawadhwa and
tejal29
as code owners
pkg/skaffold/docker/docker_init.go
Outdated
| } | ||
|
|
||
| // ValidateDockerfile makes sure the given Dockerfile is existing and valid. | ||
| var ValidateDockerfile = func(path string) bool { |
There was a problem hiding this comment.
Note: this function was moved from docker/validate.go, to keep all the init-related docker stuff in one file. If it makes more sense to leave the InitBuilder implementation by itself, I can bring back the original file and move this back.
There was a problem hiding this comment.
Curious: why the var name = func() syntax instead of directly defining a function?
There was a problem hiding this comment.
Ah, this is another thing that happens in the followup, I end up mocking this function in a test.
| "github.com/GoogleContainerTools/skaffold/testutil" | ||
| ) | ||
|
|
||
| func TestValidateDockerfile(t *testing.T) { |
There was a problem hiding this comment.
Note: moved from docker/validate_test.go.
pkg/skaffold/initializer/init.go
Outdated
| // autoSelectBuilders takes a list of builders and images, checks if any of the builders' configured target | ||
| // images match an image in the image list, and returns a list of the matching builder/image pairs. Also | ||
| // returns the images from the original image list that didn't match any build configurations. | ||
| func autoSelectBuilders(buildConfigs []InitBuilder, images []string) ([]BuilderImagePair, []string) { |
There was a problem hiding this comment.
Note: This function is only really useful for builders that can configure a target image themselves, so there will be a test for this function in the Jib follow-up PR, since Jib makes better use of this than Dockerfiles.
pkg/skaffold/docker/docker_init.go
Outdated
| } | ||
|
|
||
| // ValidateDockerfile makes sure the given Dockerfile is existing and valid. | ||
| var ValidateDockerfile = func(path string) bool { |
There was a problem hiding this comment.
Curious: why the var name = func() syntax instead of directly defining a function?
pkg/skaffold/initializer/init.go
Outdated
| func printAnalyzeJSON(out io.Writer, skipBuild bool, dockerfiles, images []string) error { | ||
| if !skipBuild && len(dockerfiles) == 0 { | ||
| return errors.New("one or more valid Dockerfiles must be present to build images with skaffold; please provide at least one Dockerfile and try again or run `skaffold init --skip-build`") | ||
| func printAnalyzeJSON(out io.Writer, skipBuild bool, buildConfigs []InitBuilder, images []string) error { |
There was a problem hiding this comment.
Is this only for printing debugging output? Does changing the JSON output format not matter?
There was a problem hiding this comment.
Hmm that's a good point, I'm not sure. It's not used from anywhere within skaffold, but might be used externally.
There was a problem hiding this comment.
I changed this a bit so the new output is more consistent across builders/more easily digested by IDEs, but I'm going to double check with the vs code team since I think they use this.
|
The logic looks good to me (except for the breaking schema change we talked about.) Others with Go expertise will have a chance to give feedback on style, naming, effective code, etc. |
Codecov Report
|
|
I think I'm going to revert the |
|
The last commit to revert --analyze looks OK. That can be done later. Other code remains same, so LGTM for the overall logic. |
This PR introduces an
InitBuilderinterface so that skaffold init can more easily detect non-Docker builders in the future. The interface allows us to:Name(): Get the name of the builder that was detectedDescribe(): Use a specialized prompt string in the interactive CLI while choosing builder/image pairsCreateArtifact(): Generate the corresponding build config artifact for the builderGetConfiguredImage(): Detect a builder's configured target image to skip choosing it from the CLIGetPath(): Get the path to the builder's config fileFollow-up for updating
--analyze: #2327Follow-up for Jib support: #2276