Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions cmd/skaffold/app/cmd/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package cmd

import (
"context"
"fmt"
"io"

"github.com/spf13/cobra"
Expand All @@ -43,11 +44,24 @@ func NewCmdVerify() *cobra.Command {

func doVerify(ctx context.Context, out io.Writer) error {
return withRunner(ctx, out, func(r runner.Runner, configs []util.VersionedConfig) error {
buildArtifacts, err := getBuildArtifactsAndSetTagsForVerify(configs, r.ApplyDefaultRepo)
if err != nil {
tips.PrintUseRunVsDeploy(out)
return err
var buildArtifacts []graph.Artifact
var err error

// If pre-built artifacts are provided via --build-artifacts flag, use them; otherwise build
if fromBuildOutputFile.String() != "" {
buildArtifacts, err = getBuildArtifactsAndSetTagsForVerify(configs, r.ApplyDefaultRepo)
if err != nil {
tips.PrintUseRunVsDeploy(out)
return err
}
} else {
// Build artifacts that are referenced in verify test cases
buildArtifacts, err = r.Build(ctx, out, targetArtifactsForVerify(configs))
if err != nil {
return fmt.Errorf("failed to build: %w", err)
}
}

defer func() {
if err := r.Cleanup(context.Background(), out, false, manifest.NewManifestListByConfig(), opts.Command); err != nil {
log.Entry(ctx).Warn("verifier cleanup:", err)
Expand Down Expand Up @@ -81,3 +95,23 @@ func getVerifyImgs(configs []util.VersionedConfig) map[string]bool {

return imgs
}

// targetArtifactsForVerify returns the build artifacts that are referenced by verify test cases.
// Only artifacts whose image names are used in verify containers will be built.
func targetArtifactsForVerify(configs []util.VersionedConfig) []*latest.Artifact {
verifyImgs := getVerifyImgs(configs)
if len(verifyImgs) == 0 {
return nil
}

artifacts := make([]*latest.Artifact, 0, len(verifyImgs))
for _, cfg := range configs {
for _, artifact := range cfg.(*latest.SkaffoldConfig).Build.Artifacts {
// Only include artifacts that are referenced in verify test cases
if verifyImgs[artifact.ImageName] {
artifacts = append(artifacts, artifact)
}
}
}
return artifacts
}
Comment on lines +101 to +117
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This function can be made more robust and slightly more efficient with a couple of changes:

  1. Early exit: If getVerifyImgs returns no images, we can exit early instead of iterating through all artifacts.
  2. Safe type assertion: The type assertion cfg.(*latest.SkaffoldConfig) will panic if cfg is not of the expected type. Using the _, ok form is safer.
  3. Pre-allocation: We can pre-allocate the artifacts slice with a capacity based on the number of verify images to avoid potential reallocations.
func targetArtifactsForVerify(configs []util.VersionedConfig) []*latest.Artifact {
	verifyImgs := getVerifyImgs(configs)
	if len(verifyImgs) == 0 {
		return nil
	}

	artifacts := make([]*latest.Artifact, 0, len(verifyImgs))
	for _, cfg := range configs {
		skaffoldConfig, ok := cfg.(*latest.SkaffoldConfig)
		if !ok {
			continue
		}
		for _, artifact := range skaffoldConfig.Build.Artifacts {
			// Only include artifacts that are referenced in verify test cases
			if verifyImgs[artifact.ImageName] {
				artifacts = append(artifacts, artifact)
			}
		}
	}
	return artifacts
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! I addressed 2 of the 3 suggestions:

Early exit - Applied

Pre-allocation - Applied

Safe type assertion - Not applied. The existing code in cmd/skaffold/app/cmd/ consistently uses direct type assertions (cfg.(*latest.SkaffoldConfig)) without the safe form - see dev.go:69, exec.go:99, build.go:101, filter.go:159,163,180, deploy.go:48. The safe form is only used in pkg/skaffold/. To maintain consistency within this package, I kept the direct assertion.

204 changes: 204 additions & 0 deletions cmd/skaffold/app/cmd/verify_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
/*
Copyright 2024 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 cmd

import (
"testing"

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

func TestGetVerifyImgs(t *testing.T) {
tests := []struct {
description string
configs []util.VersionedConfig
expected map[string]bool
}{
{
description: "no verify config",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{},
},
expected: map[string]bool{},
},
{
description: "single verify test case",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
},
},
},
},
expected: map[string]bool{"image1": true},
},
{
description: "multiple verify test cases",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
{Container: latest.VerifyContainer{Image: "image2"}},
},
},
},
},
expected: map[string]bool{"image1": true, "image2": true},
},
{
description: "multiple configs",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
},
},
},
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image2"}},
},
},
},
},
expected: map[string]bool{"image1": true, "image2": true},
},
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
result := getVerifyImgs(test.configs)
t.CheckDeepEqual(test.expected, result)
})
}
}

func TestTargetArtifactsForVerify(t *testing.T) {
tests := []struct {
description string
configs []util.VersionedConfig
expected []*latest.Artifact
}{
{
description: "no artifacts or verify config",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{},
},
expected: nil,
},
{
description: "build artifact matches verify image",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Build: latest.BuildConfig{
Artifacts: []*latest.Artifact{
{ImageName: "image1"},
},
},
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
},
},
},
},
expected: []*latest.Artifact{{ImageName: "image1"}},
},
{
description: "build artifact not used in verify",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Build: latest.BuildConfig{
Artifacts: []*latest.Artifact{
{ImageName: "image1"},
{ImageName: "image2"},
},
},
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
},
},
},
},
expected: []*latest.Artifact{{ImageName: "image1"}},
},
{
description: "verify image not in build artifacts (external image)",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Build: latest.BuildConfig{
Artifacts: []*latest.Artifact{
{ImageName: "image1"},
},
},
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "alpine:latest"}},
},
},
},
},
expected: []*latest.Artifact{},
},
{
description: "multiple configs with mixed artifacts",
configs: []util.VersionedConfig{
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Build: latest.BuildConfig{
Artifacts: []*latest.Artifact{
{ImageName: "image1"},
{ImageName: "unused"},
},
},
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image1"}},
},
},
},
&latest.SkaffoldConfig{
Pipeline: latest.Pipeline{
Build: latest.BuildConfig{
Artifacts: []*latest.Artifact{
{ImageName: "image2"},
},
},
Verify: []*latest.VerifyTestCase{
{Container: latest.VerifyContainer{Image: "image2"}},
},
},
},
},
expected: []*latest.Artifact{{ImageName: "image1"}, {ImageName: "image2"}},
},
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
result := targetArtifactsForVerify(test.configs)
t.CheckDeepEqual(test.expected, result)
})
}
}