Small changes suggested by reviewers for comments and rebase

pkg/kubectl/cmd/taint.go

@@ -41,6 +41,12 @@ import (
 	utiltaints "k8s.io/kubernetes/pkg/util/taints"
 )
 
+const (
+	MODIFIED  = "modified"
+	TAINTED   = "tainted"
+	UNTAINTED = "untainted"
+)
+
 // TaintOptions have the data required to perform the taint operation
 type TaintOptions struct {
 	resources      []string
@@ -114,42 +120,54 @@ func NewCmdTaint(f cmdutil.Factory, out io.Writer) *cobra.Command {
 
 // reorganizeTaints returns the updated set of taints, taking into account old taints that were not updated,
 // old taints that were updated, old taints that were deleted, and new taints.
-func reorganizeTaints(obj runtime.Object, overwrite bool, taintsToAdd []v1.Taint, taintsToRemove []v1.Taint) ([]v1.Taint, error) {
+func reorganizeTaints(node *v1.Node, overwrite bool, taintsToAdd []v1.Taint, taintsToRemove []v1.Taint) (string, []v1.Taint, error) {
-	node, ok := obj.(*v1.Node)
-	if !ok {
-		return nil, fmt.Errorf("unexpected type %T, expected Node", obj)
-	}
-
 	newTaints := append([]v1.Taint{}, taintsToAdd...)
-
 	oldTaints := node.Spec.Taints
 	// add taints that already existing but not updated to newTaints
+	added := addTaints(oldTaints, &newTaints)
+	allErrs, deleted := deleteTaints(taintsToRemove, &newTaints)
+	if (added && deleted) || overwrite {
+		return MODIFIED, newTaints, utilerrors.NewAggregate(allErrs)
+	} else if added {
+		return TAINTED, newTaints, utilerrors.NewAggregate(allErrs)
+	}
+	return UNTAINTED, newTaints, utilerrors.NewAggregate(allErrs)
+}
+
+// deleteTaints deletes the given taints from the node's taintlist.
+func deleteTaints(taintsToRemove []v1.Taint, newTaints *[]v1.Taint) ([]error, bool) {
+	allErrs := []error{}
+	var removed bool
+	for _, taintToRemove := range taintsToRemove {
+		removed = false
+		if len(taintToRemove.Effect) > 0 {
+			*newTaints, removed = v1helper.DeleteTaint(*newTaints, &taintToRemove)
+		} else {
+			*newTaints, removed = v1helper.DeleteTaintsByKey(*newTaints, taintToRemove.Key)
+		}
+		if !removed {
+			allErrs = append(allErrs, fmt.Errorf("taint %q not found", taintToRemove.ToString()))
+		}
+	}
+	return allErrs, removed
+}
+
+// addTaints adds the newTaints list to existing ones and updates the newTaints List.
+// TODO: This needs a rewrite to take only the new values instead of appended newTaints list to be consistent.
+func addTaints(oldTaints []v1.Taint, newTaints *[]v1.Taint) bool {
 	for _, oldTaint := range oldTaints {
 		existsInNew := false
-		for _, taint := range newTaints {
+		for _, taint := range *newTaints {
 			if taint.MatchTaint(&oldTaint) {
 				existsInNew = true
 				break
 			}
 		}
 		if !existsInNew {
-			newTaints = append(newTaints, oldTaint)
+			*newTaints = append(*newTaints, oldTaint)
 		}
 	}
-
+	return len(oldTaints) != len(*newTaints)
-	allErrs := []error{}
-	for _, taintToRemove := range taintsToRemove {
-		removed := false
-		if len(taintToRemove.Effect) > 0 {
-			newTaints, removed = v1helper.DeleteTaint(newTaints, &taintToRemove)
-		} else {
-			newTaints, removed = v1helper.DeleteTaintsByKey(newTaints, taintToRemove.Key)
-		}
-		if !removed {
-			allErrs = append(allErrs, fmt.Errorf("taint %q not found", taintToRemove.ToString()))
-		}
-	}
-	return newTaints, utilerrors.NewAggregate(allErrs)
 }
 
 func parseTaints(spec []string) ([]v1.Taint, []v1.Taint, error) {
@@ -304,8 +322,8 @@ func (o TaintOptions) RunTaint() error {
 		if err != nil {
 			return err
 		}
-
+		operation, err := o.updateTaints(obj)
-		if err := o.updateTaints(obj); err != nil {
+		if err != nil {
 			return err
 		}
 		newData, err := json.Marshal(obj)
@@ -341,18 +359,13 @@ func (o TaintOptions) RunTaint() error {
 			return o.f.PrintObject(o.cmd, mapper, outputObj, o.out)
 		}
 
-		cmdutil.PrintSuccess(mapper, false, o.out, info.Mapping.Resource, info.Name, false, "tainted")
+		cmdutil.PrintSuccess(mapper, false, o.out, info.Mapping.Resource, info.Name, false, operation)
 		return nil
 	})
 }
 
 // validateNoTaintOverwrites validates that when overwrite is false, to-be-updated taints don't exist in the node taint list (yet)
-func validateNoTaintOverwrites(obj runtime.Object, taints []v1.Taint) error {
+func validateNoTaintOverwrites(node *v1.Node, taints []v1.Taint) error {
-	node, ok := obj.(*v1.Node)
-	if !ok {
-		return fmt.Errorf("unexpected type %T, expected Node", obj)
-	}
-
 	allErrs := []error{}
 	oldTaints := node.Spec.Taints
 	for _, taint := range taints {
@@ -366,24 +379,21 @@ func validateNoTaintOverwrites(obj runtime.Object, taints []v1.Taint) error {
 	return utilerrors.NewAggregate(allErrs)
 }
 
-// updateTaints updates taints of obj
+// updateTaints applies a taint option(o) to a node in cluster after computing the net effect of operation(i.e. does it result in an overwrite?), it reports back the end result in a way that user can easily interpret.
-func (o TaintOptions) updateTaints(obj runtime.Object) error {
+func (o TaintOptions) updateTaints(obj runtime.Object) (string, error) {
-	if !o.overwrite {
-		if err := validateNoTaintOverwrites(obj, o.taintsToAdd); err != nil {
-			return err
-		}
-	}
-
-	newTaints, err := reorganizeTaints(obj, o.overwrite, o.taintsToAdd, o.taintsToRemove)
-	if err != nil {
-		return err
-	}
-
 	node, ok := obj.(*v1.Node)
 	if !ok {
-		return fmt.Errorf("unexpected type %T, expected Node", obj)
+		return "", fmt.Errorf("unexpected type %T, expected Node", obj)
+	}
+	if !o.overwrite {
+		if err := validateNoTaintOverwrites(node, o.taintsToAdd); err != nil {
+			return "", err
+		}
+	}
+	operation, newTaints, err := reorganizeTaints(node, o.overwrite, o.taintsToAdd, o.taintsToRemove)
+	if err != nil {
+		return "", err
 	}
 	node.Spec.Taints = newTaints
-
+	return operation, nil
-	return nil
 }