code review comments

GoogleContainerTools · Aug 13, 2019 · e03a889 · e03a889
1 parent 7ae27c7
commit e03a889
Show file tree

Hide file tree

Showing 6 changed files with 11 additions and 16 deletions.
diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go
@@ -99,11 +99,9 @@ func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, builds []build
 	labelDeployResults(labels, dRes)
 
 	// Collect namespaces in a string
-	namespaces := make([]string, len(nsMap))
-	i := 0
-	for k := range nsMap {
-		namespaces[i] = k
-		i++
+	namespaces := make([]string, 0, len(nsMap))
+	for ns := range nsMap {
+		namespaces = append(namespaces, ns)
 	}
 
 	return NewDeploySuccessResult(namespaces)

diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go
@@ -109,7 +109,7 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu
 
 	namespaces, err := manifests.CollectNamespaces()
 	if err != nil {
-		event.DeployInfoEvent(errors.Wrap(err, "could not fetch deployed resource namespace."+
+		event.DeployInfoEvent(errors.Wrap(err, "could not fetch deployed resource namespace. "+
 			"This might cause port-forward and deploy health-check to fail."))
 	}
 

diff --git a/pkg/skaffold/deploy/kubectl/namespaces.go b/pkg/skaffold/deploy/kubectl/namespaces.go
@@ -30,11 +30,10 @@ func (l *ManifestList) CollectNamespaces() ([]string, error) {
 	if _, err := l.Visit(replacer); err != nil {
 		return nil, errors.Wrap(err, "collecting namespaces")
 	}
-	namespaces := make([]string, len(replacer.namespaces))
-	i := 0
+
+	namespaces := make([]string, 0, len(replacer.namespaces))
 	for ns := range replacer.namespaces {
-		namespaces[i] = ns
-		i++
+		namespaces = append(namespaces, ns)
 	}
 	sort.Strings(namespaces)
 	return namespaces, nil

diff --git a/pkg/skaffold/event/event.go b/pkg/skaffold/event/event.go
@@ -150,7 +150,7 @@ func DeployInProgress() {
 	handler.handleDeployEvent(&proto.DeployEvent{Status: InProgress})
 }
 
-// DeployFailed notifies that a deployment has failed.
+// DeployFailed notifies that non-fatal errors were encountered during a deployment.
 func DeployFailed(err error) {
 	handler.handleDeployEvent(&proto.DeployEvent{Status: Failed, Err: err.Error()})
 }

diff --git a/pkg/skaffold/runner/notification.go b/pkg/skaffold/runner/notification.go
@@ -43,7 +43,7 @@ type withNotification struct {
 
 func (w withNotification) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []deploy.Labeller) *deploy.Result {
 	dr := w.Deployer.Deploy(ctx, out, builds, labellers)
-	if isErr, _ := dr.IsError(); isErr {
+	if isErr, _ := dr.IsError(); !isErr {
 		fmt.Fprint(out, terminalBell)
 	}
 	return dr

diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go
@@ -98,11 +98,9 @@ func (r *RunContext) UpdateNamespaces(ns []string) {
 	}
 
 	// Update RunContext Namespace
-	updated := make([]string, len(nsMap))
-	i := 0
+	updated := make([]string, 0, len(nsMap))
 	for k := range nsMap {
-		updated[i] = k
-		i++
+		updated = append(updated, k)
 	}
 	sort.Strings(updated)
 	r.Namespaces = updated