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

Refactor skaffold init for more flexible builder detection #2274

Merged
merged 12 commits into from
Jun 25, 2019
89 changes: 89 additions & 0 deletions pkg/skaffold/docker/docker_init.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
Copyright 2019 The Skaffold Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package docker

import (
"fmt"
"os"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/moby/buildkit/frontend/dockerfile/command"
"github.com/moby/buildkit/frontend/dockerfile/parser"
"github.com/sirupsen/logrus"
)

// Dockerfile is the path to a dockerfile. Implements the InitBuilder interface.
type Dockerfile string
TadCordle marked this conversation as resolved.
Show resolved Hide resolved
TadCordle marked this conversation as resolved.
Show resolved Hide resolved

// Describe returns the initBuilder's string representation, used when prompting the user to choose a builder.
func (d Dockerfile) Describe() string {
return fmt.Sprintf("Docker (%s)", d)
}

// GetArtifact creates an Artifact to be included in the generated Build Config
func (d Dockerfile) GetArtifact(manifestImage string) *latest.Artifact {
path := string(d)
workspace := filepath.Dir(path)
a := &latest.Artifact{ImageName: manifestImage}
if workspace != "." {
a.Workspace = workspace
}
if filepath.Base(path) != constants.DefaultDockerfilePath {
a.ArtifactType = latest.ArtifactType{
DockerArtifact: &latest.DockerArtifact{DockerfilePath: path},
}
}

return a
}

// GetConfiguredImage returns the target image configured by the builder, or an empty string if no image is configured
func (d Dockerfile) GetConfiguredImage() string {
// Target image is not configured in dockerfiles
return ""
}

// GetPath returns the path to the dockerfile
func (d Dockerfile) GetPath() string {
return string(d)
}

// ValidateDockerfile makes sure the given Dockerfile is existing and valid.
var ValidateDockerfile = func(path string) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Curious: why the var name = func() syntax instead of directly defining a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is another thing that happens in the followup, I end up mocking this function in a test.

TadCordle marked this conversation as resolved.
Show resolved Hide resolved
f, err := os.Open(path)
if err != nil {
logrus.Warnf("opening file %s: %s", path, err.Error())
return false
}

res, err := parser.Parse(f)
if err != nil || res == nil || len(res.AST.Children) == 0 {
return false
}

// validate each node contains valid dockerfile directive
for _, child := range res.AST.Children {
_, ok := command.Commands[child.Value]
if !ok {
return false
}
}

return true
}
135 changes: 135 additions & 0 deletions pkg/skaffold/docker/docker_init_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
Copyright 2019 The Skaffold Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package docker

import (
"path/filepath"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/testutil"
)

func TestValidateDockerfile(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: moved from docker/validate_test.go.

var tests = []struct {
description string
content string
fileToValidate string
expectedValid bool
}{
{
description: "valid",
content: "FROM scratch",
fileToValidate: "Dockerfile",
expectedValid: true,
},
{
description: "invalid command",
content: "GARBAGE",
fileToValidate: "Dockerfile",
expectedValid: false,
},
{
description: "not found",
fileToValidate: "Unknown",
expectedValid: false,
},
{
description: "invalid file",
content: "#escape",
fileToValidate: "Dockerfile",
expectedValid: false,
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
tmp := t.NewTempDir().
Write("Dockerfile", test.content)

valid := ValidateDockerfile(tmp.Path(test.fileToValidate))

t.CheckDeepEqual(test.expectedValid, valid)
})
}
}

func TestDescribe(t *testing.T) {
var tests = []struct {
description string
dockerfile Dockerfile
expectedPrompt string
}{
{
description: "Dockerfile prompt",
dockerfile: Dockerfile("path/to/Dockerfile"),
expectedPrompt: "Docker (path/to/Dockerfile)",
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.CheckDeepEqual(test.expectedPrompt, test.dockerfile.Describe())
})
}
}

func TestGetArtifact(t *testing.T) {
var tests = []struct {
description string
dockerfile Dockerfile
manifestImage string
expectedArtifact latest.Artifact
expectedImage string
}{
{
description: "default filename",
dockerfile: Dockerfile(filepath.Join("path", "to", "Dockerfile")),
manifestImage: "image",
expectedArtifact: latest.Artifact{
ImageName: "image",
Workspace: filepath.Join("path", "to"),
ArtifactType: latest.ArtifactType{},
},
},
{
description: "non-default filename",
dockerfile: Dockerfile(filepath.Join("path", "to", "Dockerfile1")),
manifestImage: "image",
expectedArtifact: latest.Artifact{
ImageName: "image",
Workspace: filepath.Join("path", "to"),
ArtifactType: latest.ArtifactType{
DockerArtifact: &latest.DockerArtifact{DockerfilePath: filepath.Join("path", "to", "Dockerfile1")},
},
},
},
{
description: "ignore workspace",
dockerfile: Dockerfile("Dockerfile"),
manifestImage: "image",
expectedArtifact: latest.Artifact{
ImageName: "image",
ArtifactType: latest.ArtifactType{},
},
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
artifact := test.dockerfile.GetArtifact(test.manifestImage)
t.CheckDeepEqual(test.expectedArtifact, *artifact)
})
}
}
50 changes: 0 additions & 50 deletions pkg/skaffold/docker/validate.go

This file was deleted.

66 changes: 0 additions & 66 deletions pkg/skaffold/docker/validate_test.go

This file was deleted.

Loading