Fix code review comments

GoogleContainerTools · Jul 11, 2019 · 30e7d83 · 30e7d83
1 parent 23774e0
commit 30e7d83
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 95 deletions.
diff --git a/docs/content/en/docs/references/cli/_index.md b/docs/content/en/docs/references/cli/_index.md
@@ -275,38 +275,9 @@ Options:
       --toot=false: Emit a terminal beep after the deploy is complete
 
 Usage:
-<<<<<<< HEAD
   skaffold debug [options]
 
 Use "skaffold debug options" for a list of global command-line options (applies to all commands).
-=======
-  skaffold debug
-
-Flags:
-      --cache-artifacts             Set to true to enable caching of artifacts
-      --cache-file string           Specify the location of the cache file (default $HOME/.skaffold/cache)
-      --cleanup                     Delete deployments after dev or debug mode is interrupted (default true)
-  -d, --default-repo string         Default repository value (overrides global config)
-      --enable-rpc skaffold dev     Enable gRPC for exposing Skaffold events (true by default for skaffold dev)
-  -f, --filename string             Filename or URL to the pipeline file (default "skaffold.yaml")
-      --force                       Recreate kubernetes resources if necessary for deployment (warning: might cause downtime!) (default true)
-      --insecure-registry strings   Target registries for built images which are not secure
-  -l, --label strings               Add custom labels to deployed objects. Set multiple times for multiple labels
-  -n, --namespace string            Run deployments in the specified namespace
-      --no-prune                    Skip removing images and containers built by Skaffold
-      --no-prune-children           Skip removing layers reused by Skaffold
-      --port-forward                Port-forward exposed container ports within pods
-  -p, --profile strings             Activate profiles by name
-      --rpc-http-port int           tcp port to expose event REST API over HTTP (default 50052)
-      --rpc-port int                tcp port to expose event API (default 50051)
-      --skip-tests                  Whether to skip the tests after building
-      --tail                        Stream logs from deployed objects (default true)
-      --toot                        Emit a terminal beep after the deploy is complete
-
-Global Flags:
-      --color int          Specify the default output color in ANSI escape codes (default 34)
-  -v, --verbosity string   Log level (debug, info, warn, error, fatal, panic) (default "warning")
->>>>>>> fix linter
 
 
 ```
@@ -383,33 +354,9 @@ E.g. build.out created by running skaffold build --quiet {{json .}} > build.out
       --toot=false: Emit a terminal beep after the deploy is complete
 
 Usage:
-<<<<<<< HEAD
   skaffold deploy [options]
 
 Use "skaffold deploy options" for a list of global command-line options (applies to all commands).
-=======
-  skaffold deploy
-
-Flags:
-  -a, --build-artifacts *flags.BuildOutputFileFlag   Filepath containing build output.
-                                                     E.g. build.out created by running skaffold build --quiet {{json .}} > build.out
-  -d, --default-repo string                          Default repository value (overrides global config)
-      --enable-rpc skaffold dev                      Enable gRPC for exposing Skaffold events (true by default for skaffold dev)
-  -f, --filename string                              Filename or URL to the pipeline file (default "skaffold.yaml")
-      --force                                        Recreate kubernetes resources if necessary for deployment (default false, warning: might cause downtime!)
-  -i, --images *flags.Images                         A list of pre-built images to deploy
-  -l, --label strings                                Add custom labels to deployed objects. Set multiple times for multiple labels
-  -n, --namespace string                             Run deployments in the specified namespace
-  -p, --profile strings                              Activate profiles by name
-      --rpc-http-port int                            tcp port to expose event REST API over HTTP (default 50052)
-      --rpc-port int                                 tcp port to expose event API (default 50051)
-      --tail                                         Stream logs from deployed objects (default false)
-      --toot                                         Emit a terminal beep after the deploy is complete
-
-Global Flags:
-      --color int          Specify the default output color in ANSI escape codes (default 34)
-  -v, --verbosity string   Log level (debug, info, warn, error, fatal, panic) (default "warning")
->>>>>>> fix linter
 
 
 ```
@@ -461,41 +408,9 @@ Options:
   -i, --watch-poll-interval=1000: Interval (in ms) between two checks for file changes
 
 Usage:
-<<<<<<< HEAD
   skaffold dev [options]
 
 Use "skaffold dev options" for a list of global command-line options (applies to all commands).
-=======
-  skaffold dev
-
-Flags:
-      --cache-artifacts             Set to true to enable caching of artifacts
-      --cache-file string           Specify the location of the cache file (default $HOME/.skaffold/cache)
-      --cleanup                     Delete deployments after dev or debug mode is interrupted (default true)
-  -d, --default-repo string         Default repository value (overrides global config)
-      --enable-rpc skaffold dev     Enable gRPC for exposing Skaffold events (true by default for skaffold dev)
-  -f, --filename string             Filename or URL to the pipeline file (default "skaffold.yaml")
-      --force                       Recreate kubernetes resources if necessary for deployment (warning: might cause downtime!) (default true)
-      --insecure-registry strings   Target registries for built images which are not secure
-  -l, --label strings               Add custom labels to deployed objects. Set multiple times for multiple labels
-  -n, --namespace string            Run deployments in the specified namespace
-      --no-prune                    Skip removing images and containers built by Skaffold
-      --no-prune-children           Skip removing layers reused by Skaffold
-      --port-forward                Port-forward exposed container ports within pods
-  -p, --profile strings             Activate profiles by name
-      --rpc-http-port int           tcp port to expose event REST API over HTTP (default 50052)
-      --rpc-port int                tcp port to expose event API (default 50051)
-      --skip-tests                  Whether to skip the tests after building
-      --tail                        Stream logs from deployed objects (default true)
-      --toot                        Emit a terminal beep after the deploy is complete
-      --trigger string              How are changes detected? (polling, manual or notify) (default "polling")
-  -w, --watch-image strings         Choose which artifacts to watch. Artifacts with image names that contain the expression will be watched only. Default is to watch sources for all artifacts
-  -i, --watch-poll-interval int     Interval (in ms) between two checks for file changes (default 1000)
-
-Global Flags:
-      --color int          Specify the default output color in ANSI escape codes (default 34)
-  -v, --verbosity string   Log level (debug, info, warn, error, fatal, panic) (default "warning")
->>>>>>> fix linter
 
 
 ```

diff --git a/integration/util.go b/integration/util.go
@@ -150,4 +150,3 @@ func (k *NSKubernetesClient) printDiskFreeSpace() {
 	out, _ := cmd.CombinedOutput()
 	fmt.Println(string(out))
 }
-
diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go
@@ -91,7 +91,7 @@ func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller)
 	for _, d := range deps.Items {
 		var deadline int32
 		if d.Spec.ProgressDeadlineSeconds == nil {
-			logrus.Warnf("no progressDeadlineSeconds config found for deployment %s. Setting deadline to %d seconds", d.Name, defaultStatusCheckDeadlineInSeconds)
+			logrus.Debugf("no progressDeadlineSeconds config found for deployment %s. Setting deadline to %d seconds", d.Name, defaultStatusCheckDeadlineInSeconds)
 			deadline = defaultStatusCheckDeadlineInSeconds
 		} else {
 			deadline = *d.Spec.ProgressDeadlineSeconds
@@ -120,7 +120,7 @@ func pollDeploymentRolloutStatus(ctx context.Context, k *kubectl.CLI, dName stri
 				return
 			}
 			if strings.Contains(status, "successfully rolled out") {
-				syncMap.Store(dName, status)
+				syncMap.Store(dName, nil)
 				return
 			}
 		}
@@ -145,8 +145,5 @@ func getSkaffoldDeployStatus(m *sync.Map) error {
 func getRollOutStatus(ctx context.Context, k *kubectl.CLI, dName string) (string, error) {
 	b, err := k.RunOut(ctx, nil, "rollout", []string{"status", "deployment", dName},
 		"--watch=false")
-	if err != nil {
-		return "", err
-	}
-	return string(b), nil
+	return string(b), err
 }
diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go
@@ -203,6 +203,7 @@ func TestPollDeploymentRolloutStatus(t *testing.T) {
 		duration    int
 		exactCalls  int
 		shouldErr   bool
+		timedOut    bool
 	}{
 		{
 			description: "rollout returns success",
@@ -229,7 +230,7 @@ func TestPollDeploymentRolloutStatus(t *testing.T) {
 					"Waiting for rollout to finish: 0 of 1 updated replicas are available...",
 					"deployment.apps/dep successfully rolled out"},
 			},
-			duration:   500,
+			duration:   800,
 			exactCalls: 3,
 		},
 		{
@@ -242,6 +243,7 @@ func TestPollDeploymentRolloutStatus(t *testing.T) {
 			},
 			duration:  1000,
 			shouldErr: true,
+			timedOut:  true,
 		},
 	}
 	originalPollingPeriod := defaultPollPeriodInMilliseconds
@@ -264,7 +266,8 @@ func TestPollDeploymentRolloutStatus(t *testing.T) {
 			}
 			err := getSkaffoldDeployStatus(actual)
 			t.CheckError(test.shouldErr, err)
-			if test.exactCalls > 0 {
+			// Check number of calls only if command did not timeout since there could be n-1 or n or n+1 calls when command timed out
+			if !test.timedOut {
 				t.CheckDeepEqual(test.exactCalls, mock.called)
 			}
 		})

diff --git a/pkg/skaffold/runner/deploy_test.go b/pkg/skaffold/runner/deploy_test.go
@@ -24,7 +24,10 @@ import (
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
+	runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/context"
 	"github.com/GoogleContainerTools/skaffold/testutil"
+	"k8s.io/client-go/tools/clientcmd/api"
 )
 
 func TestDeploy(t *testing.T) {
@@ -53,10 +56,20 @@ func TestDeploy(t *testing.T) {
 			testBench:   &TestBench{deployErrors: []error{errors.New("deploy error")}},
 		},
 	}
-
+
+	dummyStatusCheck := func(ctx context.Context, l *deploy.DefaultLabeller, runCtx *runcontext.RunContext) error {
+		return nil
+	}
+	originalStatusCheck := deploy.StatusCheck
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 
+			t.SetupFakeKubernetesContext(api.Config{CurrentContext: "cluster1"})
+			// Figure out why i can't use t.Override.
+			// Using t.Override throws an error "reflect: call of reflect.Value.Elem on func Value"
+			statusCheck = dummyStatusCheck
+			defer func() { statusCheck = originalStatusCheck }()
+
 			runner := createRunner(t, test.testBench, nil)
 			runner.runCtx.Opts.StatusCheck = test.statusCheck
 			out := new(bytes.Buffer)

diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go
@@ -81,6 +81,7 @@ type SkaffoldRunner struct {
 	imageList            *kubernetes.ImageList
 }
 
+// for testing
 var (
 	statusCheck = deploy.StatusCheck
 )
Original file line number	Diff line number	Diff line change
Expand Up		@@ -150,4 +150,3 @@ func (k *NSKubernetesClient) printDiskFreeSpace() {
		out, _ := cmd.CombinedOutput()
		fmt.Println(string(out))
		}
-Original file line number
+Diff line change
@@ Expand Up / @@ -81,6 +81,7 @@ type SkaffoldRunner struct { @@
     	imageList            *kubernetes.ImageList
     }
+    // for testing
     var (
     	statusCheck = deploy.StatusCheck
     )
@@ Expand Down @@