Skip to content

Commit 4425d88

Browse files
briandealwisbriandealwis
authored
Move context validation to build phase so as to not interfere with deploy (#4657)
* Revert #4492 * Error in build when encountering an invalid artifact context * gofmt * Add tests * Hide util.Copy; fix up stray local testing bogons * revert to origin/master * gofmt
1 parent 72560a3 commit 4425d88

File tree

6 files changed

+178
-96
lines changed

6 files changed

+178
-96
lines changed

‎integration/deploy_test.go

+100
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,17 @@ limitations under the License.
1717
package integration
1818

1919
import (
20+
"errors"
21+
"io"
22+
"os"
23+
"path/filepath"
2024
"strings"
2125
"testing"
2226

2327
"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
2428
"github.com/GoogleContainerTools/skaffold/integration/skaffold"
29+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
30+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
2531
"github.com/GoogleContainerTools/skaffold/testutil"
2632
)
2733

@@ -105,3 +111,97 @@ func TestDeployWithInCorrectConfig(t *testing.T) {
105111
t.Errorf("failed without saying the reason: %s", output)
106112
}
107113
}
114+
115+
// Verify that we can deploy without artifact details (https://github.com/GoogleContainerTools/skaffold/issues/4616)
116+
func TestDeployWithoutWorkspaces(t *testing.T) {
117+
MarkIntegrationTest(t, NeedsGcp)
118+
119+
ns, _ := SetupNamespace(t)
120+
121+
outputBytes := skaffold.Build("--quiet").InDir("examples/nodejs").InNs(ns.Name).RunOrFailOutput(t)
122+
// Parse the Build Output
123+
buildArtifacts, err := flags.ParseBuildOutput(outputBytes)
124+
failNowIfError(t, err)
125+
if len(buildArtifacts.Builds) != 1 {
126+
t.Fatalf("expected 1 artifact to be built, but found %d", len(buildArtifacts.Builds))
127+
}
128+
129+
tmpDir := testutil.NewTempDir(t)
130+
buildOutputFile := tmpDir.Path("build.out")
131+
tmpDir.Write("build.out", string(outputBytes))
132+
copyFiles(tmpDir.Root(), "examples/nodejs/skaffold.yaml")
133+
copyFiles(tmpDir.Root(), "examples/nodejs/k8s")
134+
135+
// Run Deploy using the build output
136+
// See https://github.com/GoogleContainerTools/skaffold/issues/2372 on why status-check=false
137+
skaffold.Deploy("--build-artifacts", buildOutputFile, "--status-check=false").InDir(tmpDir.Root()).InNs(ns.Name).RunOrFail(t)
138+
}
139+
140+
// Copies a file or directory tree. There are 2x3 cases:
141+
// 1. If _src_ is a file,
142+
// 1. and _dst_ exists and is a file then _src_ is copied into _dst_
143+
// 2. and _dst_ exists and is a directory, then _src_ is copied as _dst/$(basename src)_
144+
// 3. and _dst_ does not exist, then _src_ is copied as _dst_.
145+
// 2. If _src_ is a directory,
146+
// 1. and _dst_ exists and is a file, then return an error
147+
// 2. and _dst_ exists and is a directory, then src is copied as _dst/$(basename src)_
148+
// 3. and _dst_ does not exist, then src is copied as _dst/src[1:]_.
149+
func copyFiles(dst, src string) error {
150+
if util.IsFile(src) {
151+
switch {
152+
case util.IsFile(dst): // copy _src_ to _dst_
153+
case util.IsDir(dst): // copy _src_ to _dst/src[-1]
154+
dst = filepath.Join(dst, filepath.Base(src))
155+
default: // copy _src_ to _dst_
156+
if err := os.MkdirAll(filepath.Dir(dst), os.ModePerm); err != nil {
157+
return err
158+
}
159+
}
160+
in, err := os.Open(src)
161+
if err != nil {
162+
return err
163+
}
164+
out, err := os.Create(dst)
165+
if err != nil {
166+
return err
167+
}
168+
_, err = io.Copy(out, in)
169+
return err
170+
} else if !util.IsDir(src) {
171+
return errors.New("src does not exist")
172+
}
173+
// so src is a directory
174+
if util.IsFile(dst) {
175+
return errors.New("cannot copy directory into file")
176+
}
177+
srcPrefix := src
178+
if util.IsDir(dst) { // src is copied to _dst/$(basename src)
179+
srcPrefix = filepath.Dir(src)
180+
} else if err := os.MkdirAll(filepath.Dir(dst), os.ModePerm); err != nil {
181+
return err
182+
}
183+
return walk.From(src).Unsorted().WhenIsFile().Do(func(path string, _ walk.Dirent) error {
184+
rel, err := filepath.Rel(srcPrefix, path)
185+
if err != nil {
186+
return err
187+
}
188+
in, err := os.Open(path)
189+
if err != nil {
190+
return err
191+
}
192+
defer in.Close()
193+
194+
destFile := filepath.Join(dst, rel)
195+
if err := os.MkdirAll(filepath.Dir(destFile), os.ModePerm); err != nil {
196+
return err
197+
}
198+
199+
out, err := os.Create(destFile)
200+
if err != nil {
201+
return err
202+
}
203+
204+
_, err = io.Copy(out, in)
205+
return err
206+
})
207+
}

‎pkg/skaffold/runner/build_deploy.go

+22
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"fmt"
2222
"io"
23+
"os"
2324
"time"
2425

2526
"github.com/sirupsen/logrus"
@@ -38,6 +39,10 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
3839
return []build.Artifact{}, nil
3940
}
4041

42+
if err := checkWorkspaces(artifacts); err != nil {
43+
return nil, err
44+
}
45+
4146
tags, err := r.imageTags(ctx, out, artifacts)
4247
if err != nil {
4348
return nil, err
@@ -201,3 +206,20 @@ func (r *SkaffoldRunner) imageTags(ctx context.Context, out io.Writer, artifacts
201206
logrus.Infoln("Tags generated in", time.Since(start))
202207
return imageTags, nil
203208
}
209+
210+
func checkWorkspaces(artifacts []*latest.Artifact) error {
211+
for _, a := range artifacts {
212+
if a.Workspace != "" {
213+
if info, err := os.Stat(a.Workspace); err != nil {
214+
// err could be permission-related
215+
if os.IsNotExist(err) {
216+
return fmt.Errorf("image %q context %q does not exist", a.ImageName, a.Workspace)
217+
}
218+
return fmt.Errorf("image %q context %q: %w", a.ImageName, a.Workspace, err)
219+
} else if !info.IsDir() {
220+
return fmt.Errorf("image %q context %q is not a directory", a.ImageName, a.Workspace)
221+
}
222+
}
223+
}
224+
return nil
225+
}

‎pkg/skaffold/runner/build_deploy_test.go

+55
Original file line numberDiff line numberDiff line change
@@ -128,3 +128,58 @@ func TestBuildAndTestSkipBuild(t *testing.T) {
128128
t.CheckDeepEqual([]Actions{{}}, testBench.Actions())
129129
})
130130
}
131+
132+
func TestCheckWorkspaces(t *testing.T) {
133+
tmpDir := testutil.NewTempDir(t).Touch("file")
134+
tmpFile := tmpDir.Path("file")
135+
136+
tests := []struct {
137+
description string
138+
artifacts []*latest.Artifact
139+
shouldErr bool
140+
}{
141+
{
142+
description: "no workspace",
143+
artifacts: []*latest.Artifact{
144+
{
145+
ImageName: "image",
146+
},
147+
},
148+
},
149+
{
150+
description: "directory that exists",
151+
artifacts: []*latest.Artifact{
152+
{
153+
ImageName: "image",
154+
Workspace: tmpDir.Root(),
155+
},
156+
},
157+
},
158+
{
159+
description: "error on non-existent location",
160+
artifacts: []*latest.Artifact{
161+
{
162+
ImageName: "image",
163+
Workspace: "doesnotexist",
164+
},
165+
},
166+
shouldErr: true,
167+
},
168+
{
169+
description: "error on file",
170+
artifacts: []*latest.Artifact{
171+
{
172+
ImageName: "image",
173+
Workspace: tmpFile,
174+
},
175+
},
176+
shouldErr: true,
177+
},
178+
}
179+
for _, test := range tests {
180+
testutil.Run(t, test.description, func(t *testutil.T) {
181+
err := checkWorkspaces(test.artifacts)
182+
t.CheckError(test.shouldErr, err)
183+
})
184+
}
185+
}

‎pkg/skaffold/schema/samples_test.go

+1-9
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,7 @@ func TestParseSamples(t *testing.T) {
7474
}
7575

7676
func checkSkaffoldConfig(t *testutil.T, yaml []byte) {
77-
root := t.NewTempDir()
78-
configFile := root.Path("skaffold.yaml")
79-
root.Write("skaffold.yaml", string(yaml))
80-
// create workspace directories referenced in these samples
81-
for _, d := range []string{"app", "node", "python", "leeroy-web", "leeroy-app", "backend", "base-service", "world-service"} {
82-
root.Mkdir(d)
83-
}
84-
root.Chdir()
85-
77+
configFile := t.TempFile("skaffold.yaml", yaml)
8678
cfg, err := ParseConfigAndUpgrade(configFile, latest.Version)
8779
t.CheckNoError(err)
8880

‎pkg/skaffold/schema/validation/validation.go

-21
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package validation
1818

1919
import (
2020
"fmt"
21-
"os"
2221
"reflect"
2322
"strings"
2423

@@ -37,7 +36,6 @@ var (
3736
// Process checks if the Skaffold pipeline is valid and returns all encountered errors as a concatenated string
3837
func Process(config *latest.SkaffoldConfig) error {
3938
errs := visitStructs(config, validateYamltags)
40-
errs = append(errs, validateWorkspaces(config.Build.Artifacts)...)
4139
errs = append(errs, validateImageNames(config.Build.Artifacts)...)
4240
errs = append(errs, validateDockerNetworkMode(config.Build.Artifacts)...)
4341
errs = append(errs, validateCustomDependencies(config.Build.Artifacts)...)
@@ -58,25 +56,6 @@ func Process(config *latest.SkaffoldConfig) error {
5856
return fmt.Errorf(strings.Join(messages, " | "))
5957
}
6058

61-
// validateWorkspaces makes sure the artifact workspaces are valid directories.
62-
func validateWorkspaces(artifacts []*latest.Artifact) (errs []error) {
63-
for _, a := range artifacts {
64-
if a.Workspace != "" {
65-
if info, err := os.Stat(a.Workspace); err != nil {
66-
// err could be permission-related
67-
if os.IsNotExist(err) {
68-
errs = append(errs, fmt.Errorf("image %q context %q does not exist", a.ImageName, a.Workspace))
69-
} else {
70-
errs = append(errs, fmt.Errorf("image %q context %q: %w", a.ImageName, a.Workspace, err))
71-
}
72-
} else if !info.IsDir() {
73-
errs = append(errs, fmt.Errorf("image %q context %q is not a directory", a.ImageName, a.Workspace))
74-
}
75-
}
76-
}
77-
return
78-
}
79-
8059
// validateImageNames makes sure the artifact image names are valid base names,
8160
// without tags nor digests.
8261
func validateImageNames(artifacts []*latest.Artifact) (errs []error) {

‎pkg/skaffold/schema/validation/validation_test.go

-66
Original file line numberDiff line numberDiff line change
@@ -717,72 +717,6 @@ func TestValidateJibPluginType(t *testing.T) {
717717
}
718718
}
719719

720-
func TestValidateWorkspaces(t *testing.T) {
721-
tmpDir := testutil.NewTempDir(t).Touch("file")
722-
tmpFile := tmpDir.Path("file")
723-
724-
tests := []struct {
725-
description string
726-
artifacts []*latest.Artifact
727-
shouldErr bool
728-
}{
729-
{
730-
description: "no workspace",
731-
artifacts: []*latest.Artifact{
732-
{
733-
ImageName: "image",
734-
},
735-
},
736-
},
737-
{
738-
description: "directory that exists",
739-
artifacts: []*latest.Artifact{
740-
{
741-
ImageName: "image",
742-
Workspace: tmpDir.Root(),
743-
},
744-
},
745-
},
746-
{
747-
description: "error on non-existent location",
748-
artifacts: []*latest.Artifact{
749-
{
750-
ImageName: "image",
751-
Workspace: "doesnotexist",
752-
},
753-
},
754-
shouldErr: true,
755-
},
756-
{
757-
description: "error on file",
758-
artifacts: []*latest.Artifact{
759-
{
760-
ImageName: "image",
761-
Workspace: tmpFile,
762-
},
763-
},
764-
shouldErr: true,
765-
},
766-
}
767-
for _, test := range tests {
768-
testutil.Run(t, test.description, func(t *testutil.T) {
769-
// disable yamltags validation
770-
t.Override(&validateYamltags, func(interface{}) error { return nil })
771-
772-
err := Process(
773-
&latest.SkaffoldConfig{
774-
Pipeline: latest.Pipeline{
775-
Build: latest.BuildConfig{
776-
Artifacts: test.artifacts,
777-
},
778-
},
779-
})
780-
781-
t.CheckError(test.shouldErr, err)
782-
})
783-
}
784-
}
785-
786720
func TestValidateLogsConfig(t *testing.T) {
787721
tests := []struct {
788722
prefix string

0 commit comments

Comments
 (0)