Skip to content

Commit 2b47821

Browse files
tejal29tejal29
committed
address code reivew comments
1 parent 505ee97 commit 2b47821

10 files changed

+56
-37
lines changed

‎cmd/skaffold/app/cmd/build_test.go

+16-11
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
2828
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
2929
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
30+
sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
3031
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
3132
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
3233
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -49,9 +50,13 @@ func (r *mockRunner) Stop() error {
4950
return nil
5051
}
5152

53+
func (r *mockRunner) ErrConfig() sErrors.Config {
54+
return &runcontext.RunContext{}
55+
}
56+
5257
func TestTagFlag(t *testing.T) {
53-
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
54-
return &mockRunner{}, &runcontext.RunContext{}, []*latest.SkaffoldConfig{{}}, nil
58+
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
59+
return &mockRunner{}, []*latest.SkaffoldConfig{{}}, nil
5560
}
5661

5762
testutil.Run(t, "override tag with argument", func(t *testutil.T) {
@@ -69,8 +74,8 @@ func TestTagFlag(t *testing.T) {
6974
}
7075

7176
func TestQuietFlag(t *testing.T) {
72-
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
73-
return &mockRunner{}, nil, []*latest.SkaffoldConfig{{}}, nil
77+
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
78+
return &mockRunner{}, []*latest.SkaffoldConfig{{}}, nil
7479
}
7580

7681
tests := []struct {
@@ -115,8 +120,8 @@ func TestQuietFlag(t *testing.T) {
115120
}
116121

117122
func TestFileOutputFlag(t *testing.T) {
118-
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
119-
return &mockRunner{}, nil, []*latest.SkaffoldConfig{{}}, nil
123+
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
124+
return &mockRunner{}, []*latest.SkaffoldConfig{{}}, nil
120125
}
121126

122127
tests := []struct {
@@ -178,16 +183,16 @@ func TestFileOutputFlag(t *testing.T) {
178183
}
179184

180185
func TestRunBuild(t *testing.T) {
181-
errRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
182-
return nil, nil, nil, errors.New("some error")
186+
errRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
187+
return nil, nil, errors.New("some error")
183188
}
184-
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
185-
return &mockRunner{}, nil, []*latest.SkaffoldConfig{{}}, nil
189+
mockCreateRunner := func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
190+
return &mockRunner{}, []*latest.SkaffoldConfig{{}}, nil
186191
}
187192

188193
tests := []struct {
189194
description string
190-
mock func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error)
195+
mock func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error)
191196
shouldErr bool
192197
}{
193198
{

‎cmd/skaffold/app/cmd/debug_test.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323

2424
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
2525
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
26-
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
2726
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
2827
"github.com/GoogleContainerTools/skaffold/testutil"
2928
)
@@ -49,8 +48,8 @@ func TestNewCmdDebug(t *testing.T) {
4948
func TestDebugIndependentFromDev(t *testing.T) {
5049
mockRunner := &mockDevRunner{}
5150
testutil.Run(t, "DevDebug", func(t *testutil.T) {
52-
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
53-
return mockRunner, nil, []*latest.SkaffoldConfig{{}}, nil
51+
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
52+
return mockRunner, []*latest.SkaffoldConfig{{}}, nil
5453
})
5554
t.Override(&opts, config.SkaffoldOptions{})
5655
t.Override(&doDev, func(context.Context, io.Writer) error {

‎cmd/skaffold/app/cmd/dev_test.go

+13-4
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"testing"
2424

2525
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
26+
sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
2627
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
2728
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
2829
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -42,6 +43,10 @@ func (r *mockDevRunner) Dev(context.Context, io.Writer, []*latest.Artifact) erro
4243
return r.errDev
4344
}
4445

46+
func (r *mockDevRunner) ErrConfig() sErrors.Config {
47+
return &runcontext.RunContext{}
48+
}
49+
4550
func (r *mockDevRunner) HasBuilt() bool {
4651
r.calls = append(r.calls, "HasBuilt")
4752
return r.hasBuilt
@@ -96,8 +101,8 @@ func TestDoDev(t *testing.T) {
96101
hasDeployed: test.hasDeployed,
97102
errDev: context.Canceled,
98103
}
99-
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
100-
return mockRunner, &runcontext.RunContext{}, []*latest.SkaffoldConfig{{}}, nil
104+
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
105+
return mockRunner, []*latest.SkaffoldConfig{{}}, nil
101106
})
102107
t.Override(&opts, config.SkaffoldOptions{
103108
Cleanup: true,
@@ -134,6 +139,10 @@ func (m *mockConfigChangeRunner) HasDeployed() bool {
134139
return true
135140
}
136141

142+
func (m *mockConfigChangeRunner) ErrConfig() sErrors.Config {
143+
return &runcontext.RunContext{}
144+
}
145+
137146
func (m *mockConfigChangeRunner) Prune(context.Context, io.Writer) error {
138147
return nil
139148
}
@@ -146,8 +155,8 @@ func TestDevConfigChange(t *testing.T) {
146155
testutil.Run(t, "test config change", func(t *testutil.T) {
147156
mockRunner := &mockConfigChangeRunner{}
148157

149-
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
150-
return mockRunner, nil, []*latest.SkaffoldConfig{{}}, nil
158+
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
159+
return mockRunner, []*latest.SkaffoldConfig{{}}, nil
151160
})
152161
t.Override(&opts, config.SkaffoldOptions{
153162
Cleanup: true,

‎cmd/skaffold/app/cmd/run_test.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
2626
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
2727
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
28-
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
2928
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
3029
"github.com/GoogleContainerTools/skaffold/testutil"
3130
)
@@ -70,8 +69,8 @@ func (r *mockRunRunner) DeployAndLog(context.Context, io.Writer, []build.Artifac
7069
func TestBuildImageFlag(t *testing.T) {
7170
testutil.Run(t, "", func(t *testutil.T) {
7271
mockRunner := &mockRunRunner{}
73-
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
74-
return mockRunner, nil, []*latest.SkaffoldConfig{{
72+
t.Override(&createRunner, func(io.Writer, config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
73+
return mockRunner, []*latest.SkaffoldConfig{{
7574
Pipeline: latest.Pipeline{
7675
Build: latest.BuildConfig{
7776
Artifacts: []*latest.Artifact{

‎cmd/skaffold/app/cmd/runner.go

+9-8
Original file line numberDiff line numberDiff line change
@@ -46,30 +46,31 @@ import (
4646
var createRunner = createNewRunner
4747

4848
func withRunner(ctx context.Context, out io.Writer, action func(runner.Runner, []*latest.SkaffoldConfig) error) error {
49-
runner, runCtx, config, err := createRunner(out, opts)
49+
runner, config, err := createRunner(out, opts)
5050
if err != nil {
5151
return err
5252
}
5353

54-
err = action(runner, config)
55-
56-
return alwaysSucceedWhenCancelled(ctx, runCtx, err)
54+
if err := action(runner, config); err != nil {
55+
return alwaysSucceedWhenCancelled(ctx, runner.ErrConfig(), err)
56+
}
57+
return nil
5758
}
5859

5960
// createNewRunner creates a Runner and returns the SkaffoldConfig associated with it.
60-
func createNewRunner(out io.Writer, opts config.SkaffoldOptions) (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {
61+
func createNewRunner(out io.Writer, opts config.SkaffoldOptions) (runner.Runner, []*latest.SkaffoldConfig, error) {
6162
runCtx, configs, err := runContext(out, opts)
6263
if err != nil {
63-
return nil, nil, nil, err
64+
return nil, nil, err
6465
}
6566
instrumentation.InitMeterFromConfig(configs)
6667
runner, err := runner.NewForConfig(runCtx)
6768
if err != nil {
6869
event.InititializationFailed(err)
69-
return nil, nil, nil, fmt.Errorf("creating runner: %w", err)
70+
return nil, nil, fmt.Errorf("creating runner: %w", err)
7071
}
7172

72-
return runner, runCtx, configs, nil
73+
return runner, configs, nil
7374
}
7475

7576
func runContext(out io.Writer, opts config.SkaffoldOptions) (*runcontext.RunContext, []*latest.SkaffoldConfig, error) {

‎cmd/skaffold/app/cmd/runner_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func TestCreateNewRunner(t *testing.T) {
9999
Write("skaffold.yaml", fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, test.config)).
100100
Chdir()
101101

102-
_, _, _, err := createNewRunner(ioutil.Discard, test.options)
102+
_, _, err := createNewRunner(ioutil.Discard, test.options)
103103

104104
t.CheckError(test.shouldErr, err)
105105
if test.expectedError != "" {

‎pkg/skaffold/errors/deploy_problems.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@ func suggestDeployFailedAction(cfg Config) []*proto.Suggestion {
3636
}
3737
return []*proto.Suggestion{{
3838
SuggestionCode: proto.SuggestionCode_CHECK_MINIKUBE_STATUS,
39-
Action: fmt.Sprintf("Check if minikube is running using `%s` cmd and try again.", command),
39+
Action: fmt.Sprintf("Check if minikube is running using %q and try again.", command),
4040
}}
4141
}
4242

4343
return []*proto.Suggestion{{
4444
SuggestionCode: proto.SuggestionCode_CHECK_CLUSTER_CONNECTION,
45-
Action: fmt.Sprintf("Check your connection for %s cluster", cfg.GetKubeContext()),
45+
Action: fmt.Sprintf("Check your connection to the %q cluster", cfg.GetKubeContext()),
4646
}}
4747
}

‎pkg/skaffold/errors/deploy_problems_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestSuggestDeployFailedAction(t *testing.T) {
3636
isMinikube: true,
3737
expected: []*proto.Suggestion{{
3838
SuggestionCode: proto.SuggestionCode_CHECK_MINIKUBE_STATUS,
39-
Action: "Check if minikube is running using `minikube status` cmd and try again.",
39+
Action: "Check if minikube is running using \"minikube status\" and try again.",
4040
}},
4141
},
4242
{
@@ -45,7 +45,7 @@ func TestSuggestDeployFailedAction(t *testing.T) {
4545
isMinikube: true,
4646
expected: []*proto.Suggestion{{
4747
SuggestionCode: proto.SuggestionCode_CHECK_MINIKUBE_STATUS,
48-
Action: "Check if minikube is running using `minikube status -p test_cluster` cmd and try again.",
48+
Action: "Check if minikube is running using \"minikube status -p test_cluster\" and try again.",
4949
}},
5050
},
5151
{
@@ -54,7 +54,7 @@ func TestSuggestDeployFailedAction(t *testing.T) {
5454
isMinikube: false,
5555
expected: []*proto.Suggestion{{
5656
SuggestionCode: proto.SuggestionCode_CHECK_CLUSTER_CONNECTION,
57-
Action: "Check your connection for gke_test cluster",
57+
Action: "Check your connection to the \"gke_test\" cluster",
5858
}},
5959
},
6060
}

‎pkg/skaffold/errors/errors_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -247,13 +247,13 @@ func TestShowAIError(t *testing.T) {
247247
cfg: dummyConfig{kubectx: "gke_test"},
248248
phase: Deploy,
249249
err: fmt.Errorf(`exiting dev mode because first deploy failed: unable to connect to Kubernetes: Get "https://192.168.64.3:8443/version?timeout=32s": net/http: TLS handshake timeout`),
250-
expected: "Deploy Failed. Could not connect to cluster due to \"https://192.168.64.3:8443/version?timeout=32s\": net/http: TLS handshake timeout. Check your connection for gke_test cluster.",
250+
expected: "Deploy Failed. Could not connect to cluster due to \"https://192.168.64.3:8443/version?timeout=32s\": net/http: TLS handshake timeout. Check your connection to the \"gke_test\" cluster.",
251251
expectedAE: &proto.ActionableErr{
252252
ErrCode: proto.StatusCode_DEPLOY_CLUSTER_CONNECTION_ERR,
253253
Message: "exiting dev mode because first deploy failed: unable to connect to Kubernetes: Get \"https://192.168.64.3:8443/version?timeout=32s\": net/http: TLS handshake timeout",
254254
Suggestions: []*proto.Suggestion{{
255255
SuggestionCode: proto.SuggestionCode_CHECK_CLUSTER_CONNECTION,
256-
Action: "Check your connection for gke_test cluster",
256+
Action: "Check your connection to the \"gke_test\" cluster",
257257
}},
258258
},
259259
},

‎pkg/skaffold/runner/runner.go

+6
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
2727
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/label"
2828
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/status"
29+
sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
2930
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
3031
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubectl"
3132
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
@@ -54,6 +55,7 @@ type Runner interface {
5455
Prune(context.Context, io.Writer) error
5556
HasDeployed() bool
5657
HasBuilt() bool
58+
ErrConfig() sErrors.Config
5759
}
5860

5961
// SkaffoldRunner is responsible for running the skaffold build, test and deploy config.
@@ -97,3 +99,7 @@ func (r *SkaffoldRunner) HasDeployed() bool {
9799
func (r *SkaffoldRunner) HasBuilt() bool {
98100
return r.hasBuilt
99101
}
102+
103+
func (r *SkaffoldRunner) ErrConfig() sErrors.Config {
104+
return r.runCtx
105+
}

0 commit comments

Comments
 (0)